On 08/10/2024 17:30, Pablo Neira Ayuso wrote:
> Hi Nikolay,
>
> On Sat, Oct 05, 2024 at 05:06:56PM +0300, Nikolay Aleksandrov wrote:
>> On 05/10/2024 04:44, Amedeo Baragiola wrote:
>>> Since commit 751de2012eaf ("netfilter: br_netfilter: skip conntrack input hook for promisc packets")
>>> a second argument (promisc) has been added to br_pass_frame_up which
>>> represents whether the interface is in promiscuous mode. However,
>>> internally - in one remaining case - br_pass_frame_up checks the device
>>> flags derived from skb instead of the argument being passed in.
>>> This one-line changes addresses this inconsistency.
>>>
>>> Signed-off-by: Amedeo Baragiola <ingamedeo@xxxxxxxxx>
>>> ---
>>> net/bridge/br_input.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>>> index ceaa5a89b947..156c18f42fa3 100644
>>> --- a/net/bridge/br_input.c
>>> +++ b/net/bridge/br_input.c
>>> @@ -50,8 +50,7 @@ static int br_pass_frame_up(struct sk_buff *skb, bool promisc)
>>> * packet is allowed except in promisc mode when someone
>>> * may be running packet capture.
>>> */
>>> - if (!(brdev->flags & IFF_PROMISC) &&
>>> - !br_allowed_egress(vg, skb)) {
>>> + if (!promisc && !br_allowed_egress(vg, skb)) {
>>> kfree_skb(skb);
>>> return NET_RX_DROP;
>>> }
>>
>> This is subtle, but it does change behaviour when a BR_FDB_LOCAL dst
>> is found it will always drop the traffic after this patch (w/ promisc) if it
>> doesn't pass br_allowed_egress(). It would've been allowed before, but current
>> situation does make the patch promisc bit inconsistent, i.e. we get
>> there because of BR_FDB_LOCAL regardless of the promisc flag.
>>
>> Because we can have a BR_FDB_LOCAL dst and still pass up such skb because of
>> the flag instead of local_rcv (see br_br_handle_frame_finish()).
>>
>> CCing also Pablo for a second pair of eyes and as the original patch
>> author. :)
>>
>> Pablo WDYT?
>>
>> Just FYI we definitely want to see all traffic if promisc is set, so
>> this patch is a no-go.
>
> promisc is always _false_ for BR_FDB_LOCAL dst:
>
> if (dst) {
> unsigned long now = jiffies;
>
> if (test_bit(BR_FDB_LOCAL, &dst->flags))
> return br_pass_frame_up(skb, false);
>
> ...
> }
>
> if (local_rcv)
> return br_pass_frame_up(skb, promisc);
>
>>> - if (!(brdev->flags & IFF_PROMISC) &&
>>> - !br_allowed_egress(vg, skb)) {
>>> + if (!promisc && !br_allowed_egress(vg, skb)) {
>
> Then, this is not equivalent.
>
> But, why is br_allowed_egress() skipped depending on brdev->flags & IFF_PROMISC?
>
> I mean, how does this combination work?
>
> BR_FDB_LOCAL dst AND (brdev->flags & IFF_PROMISC) AND BR_INPUT_SKB_CB(skb)->vlan_filtered
The bridge should see all packets come up if promisc flag is set, regardless if the
vlan exists or not, so br_allowed_egress() is skipped entirely. As I commented
separately the patch changes that behaviour and suddenly these packets
(BR_FDB_LOCAL fdb + promisc bit set on the bridge dev) won't be sent up to
the bridge. I think the current code should stay as-is, but wanted to get
your opinion if we can still hit the warning that was fixed because we can
still hit that code with a BR_FDB_LOCAL dst with promisc flag set and
the promisc flag will be == false in that case.
