[PATCH] bridge: use promisc arg instead of skb flags

[PATCH] bridge: use promisc arg instead of skb flags

[Amedeo Baragiola]

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@gmail.com>
---
 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;
 	}
-- 
2.46.2

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Nikolay Aleksandrov]

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@gmail.com>
> ---
>  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.

Cheers,
 Nik

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Amedeo Baragiola]

I agree, just patch actually changes the behaviour when a BR_FDB_LOCAL
dst is found and drops the traffic because promisc is *always* set to
false when a BR_FDB_LOCAL dst is found in br_handle_frame_finish().
I guess the problem I was trying to solve was that since the
introduction of the promisc flag we still use brdev->flags &
IFF_PROMISC in br_pass_frame_up() which is essentially the value of
promisc (except in the BR_FDB_LOCAL case above) instead of promisc
itself.

Amedeo


On Sat, Oct 5, 2024 at 7:06 AM Nikolay Aleksandrov <razor@blackwall.org> 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@gmail.com>
> > ---
> >  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.
>
> Cheers,
>  Nik



-- 
Thanks,
Amedeo

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Nikolay Aleksandrov]

On 06/10/2024 20:24, Amedeo Baragiola wrote:
> I agree, just patch actually changes the behaviour when a BR_FDB_LOCAL
> dst is found and drops the traffic because promisc is *always* set to
> false when a BR_FDB_LOCAL dst is found in br_handle_frame_finish().
> I guess the problem I was trying to solve was that since the
> introduction of the promisc flag we still use brdev->flags &
> IFF_PROMISC in br_pass_frame_up() which is essentially the value of
> promisc (except in the BR_FDB_LOCAL case above) instead of promisc
> itself.
> 
> Amedeo
> 
> 
[snip]

Please don't top post on netdev@. 
The current code works correctly, my question to Pablo was more about if the warn
can still be triggered by adding a BR_FDB_LOCAL fdb and setting bridge
promisc on, then we'll hit that codepath with promisc == false and it's
kind of correct because traffic would've been passed up anyway, but the
promisc flag can be actually set on the device..

Cheers,
 Nik

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Pablo Neira Ayuso]

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@gmail.com>
> > ---
> >  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

> >  		kfree_skb(skb);
> >  		return NET_RX_DROP;
> >  	}

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Nikolay Aleksandrov]

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@gmail.com>
>>> ---
>>>  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.

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Pablo Neira Ayuso]

On Tue, Oct 08, 2024 at 05:45:44PM +0300, Nikolay Aleksandrov wrote:
> 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@gmail.com>
> >>> ---
> >>>  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.

I see, but does this defeat the purpose of the vlan bridge filtering
for BR_FDB_LOCAL dst while IFF_PROMISC is on?

> 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 agree this proposed patch does not improve the situation.

> 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.

Packets with BR_FDB_LOCAL dst are unicast packets but
skb->pkt_type != PACKET_HOST?

Re: [PATCH] bridge: use promisc arg instead of skb flags

[Nikolay Aleksandrov]

On 08/10/2024 18:44, Pablo Neira Ayuso wrote:
> On Tue, Oct 08, 2024 at 05:45:44PM +0300, Nikolay Aleksandrov wrote:
>> 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@gmail.com>
>>>>> ---
>>>>>  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.
> 
> I see, but does this defeat the purpose of the vlan bridge filtering
> for BR_FDB_LOCAL dst while IFF_PROMISC is on?
> 

Yes, it does, but it is expected behaviour with promisc on.

>> 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 agree this proposed patch does not improve the situation.
> 
>> 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.
> 
> Packets with BR_FDB_LOCAL dst are unicast packets but
> skb->pkt_type != PACKET_HOST?

BR_FDB_LOCAL just marks the skb to be passed up the stack (terminated
locally) with the bridge device set in skb->dev, it may or may not be PACKET_HOST.