netdev - Re: [PATCH net-next 2/4] net: dsa: Link aggregation support

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 26 Nov 2020 23:36:17 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, vivien.didelot@...il.com,
        olteanv@...il.com, j.vosburgh@...il.com, vfalico@...il.com,
        andy@...yhouse.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] net: dsa: Link aggregation support

On Fri, Nov 20, 2020 at 14:30, Andrew Lunn <andrew@...n.ch> wrote:
> On Thu, Nov 19, 2020 at 06:43:38PM -0800, Florian Fainelli wrote:
>> > 
>> > Hi Tobias
>> > 
>> > My comment last time was to statically allocated them at probe
>> > time. Worse case scenario is each port is alone in a LAG. Pointless,
>> > but somebody could configure it. In dsa_tree_setup_switches() you can
>> > count the number of ports and then allocate an array, or while setting
>> > up a port, add one more lag to the list of lags.

Right, yes I forgot about that, sorry.

Permit me a final plea for the current implementation. If it is a hard
no, say the word and I will re-spin a v2 with your suggestion.

>> The allocation is allowed to sleep (have not checked the calling context
>> of dsa_lag_get() whether this is OK) so what would be the upside of

These events always originate from a sendmsg (netlink message), AFAIK
there is no other way for an interface to move to a new upper, so I do
not see any issues there.

>> doing upfront dsa_lag allocation which could be wasteful?
>
> Hi Florian
>
> It fits the pattern for the rest of the DSA core. We never allocate
> anything at runtime. That keeps the error handling simple, we don't
> need to deal with ENOMEM errors, undoing whatever we might of done,
> implementing transactions etc.

I understand that argument in principle. The reason that I think it
carries less weight in this case has to do with the dynamic nature of
the LAGs themselves.

In the cases of `struct dsa_port`s or `struct dsa_switch`es there is no
way for them to be dynamically removed, so it makes a lot of sense to
statically allocate everything.

Contrast that with a LAG that dynamically created and dynamically
modified, i.e. more ports can join/leave the LAG during its
lifecycle. This means you get all the headache of figuring out if the
LAG is in use or not anyway, i.e. you need to refcount them.

So essentially we _will_ be dynamically allocating/freeing them. We just
get to choose if we do it through the SLAB allocator, or through the
static array.

If you go with the static array, you theoretically can not get the
equivalent of an ENOMEM. Practically though you have to iterate through
the array and look for a free entry, but you still have to put a return
statement at the bottom of that function, right? Or panic I suppose. My
guess is you end up with:

    struct dsa_lag *dsa_lag_get(dst)
    {
        for (lag in dst->lag_array) {
            if (lag->dev == NULL)
                return lag;
        }

        return NULL;
    }

So now we have just traded dealing with an ENOMEM for a NULL pointer;
pretty much the same thing.