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

[Nikolay Aleksandrov <razor@blackwall.org>]

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