netdev - Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <877dmjqokp.fsf@waldekranz.com>
Date:   Sun, 07 Mar 2021 10:51:18 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net 2/2] net: dsa: Always react to global bridge attribute changes

On Sun, Mar 07, 2021 at 02:58, Vladimir Oltean <olteanv@...il.com> wrote:
> On Sat, Mar 06, 2021 at 07:17:09PM +0100, Tobias Waldekranz wrote:
>> On Sat, Mar 06, 2021 at 16:04, Vladimir Oltean <olteanv@...il.com> wrote:
>> > On Sat, Mar 06, 2021 at 04:00:33PM +0200, Vladimir Oltean wrote:
>> >> Hi Tobias,
>> >>
>> >> On Sat, Mar 06, 2021 at 01:24:55AM +0100, Tobias Waldekranz wrote:
>> >> > This is the second attempt to provide a fix for the issue described in
>> >> > 99b8202b179f, which was reverted in the previous commit.
>> >> >
>> >> > When a change is made to some global bridge attribute, such as VLAN
>> >> > filtering, accept events where orig_dev is the bridge master netdev.
>> >> >
>> >> > Separate the validation of orig_dev based on whether the attribute in
>> >> > question is global or per-port.
>> >> >
>> >> > Fixes: 5696c8aedfcc ("net: dsa: Don't offload port attributes on standalone ports")
>> >> > Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
>> >> > ---
>> >>
>> >> What do you think about this alternative?
>> >
>> > Ah, wait, this won't work when offloading objects/attributes on a LAG.
>> > Let me actually test your patch.
>> 
>> Right. But you made me realize that my v1 is also flawed, because it
>> does not guard against trying to apply attributes to non-offloaded
>> ports. ...the original issue :facepalm:
>> 
>> I have a version ready which reuses the exact predicate that you
>> previously added to dsa_port_offloads_netdev:
>> 
>> -               if (netif_is_bridge_master(attr->orig_dev))
>> +               if (dp->bridge_dev == attr->orig_dev)
>> 
>> Do you think anything else needs to be changed, or should I send that as
>> v2?
>
> Sorry, I just get a blank stare when I look at that blob of code you've
> added at the beginning of dsa_slave_port_attr_set, it might as well be
> correct but I'm not smart enough to process it and say "yes it is".
>
> What do you think about this one? At least for me it's easier to
> understand what's going on, and would leave a lot more room for further
> fixups if needed.

I like the approach of having to explicitly state the supported orig_dev
per attribute or object. I think we should go with your fix.