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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201126225753.GP2075216@lunn.ch>
Date:   Thu, 26 Nov 2020 23:57:53 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, 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

> 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;
>     }

I would put a WARN() in here, and return the NULL pointer. That will
then likely opps soon afterwards and kill of the user space tool
configuring the LAG, maybe leaving rtnl locked, and so all network
configuration dies. But that is all fine, since you cannot have more
LAGs than ports. This can never happen. If it does happen, something
is badly wrong and we want to know about it. And so a very obvious
explosion is good.

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

ENOMEM you have to handle correctly, unwind everything and leaving the
stack in the same state as before. Being out of memory is not a reason
to explode. Have you tested this? It is the sort of thing which does
not happen very often, so i expect is broken.

   Andrew