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

[Nikolay Aleksandrov <razor@blackwall.org>]

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.