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
> > kfree_skb(skb);
> > return NET_RX_DROP;
> > }
