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: <87r1of88dp.fsf@waldekranz.com>
Date:   Fri, 27 Nov 2020 10:19:14 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Andrew Lunn <andrew@...n.ch>
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

On Thu, Nov 26, 2020 at 23:57, Andrew Lunn <andrew@...n.ch> wrote:
>> 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

This is a digression, but I really do not get this shift from using
BUG()s to WARN()s in the kernel when you detect a violated invariant. It
smells of "On Error Resume Next" to me.

> 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.

Indeed. That is why I think it should be BUG(). Leaving the system to
limp along can easily go undetected.

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

We have to handle EWHATEVER correctly, no? I do not get what is so
special about ENOMEM. If we hit some other error after the allocation we
have to remember to free the memory of course, but that is just normal
cleanup. Is this what you mean by "unwind everything"? In that case we
need to also "free" the element we allocated from the array. Again, it
is fundamentally the same problem.

How would a call to kmalloc have any impact on the stack? (Barring
exotic bugs in the allocator that would allow the heap to intrude on
stack memory) Or am I misunderstanding what you mean by "the stack"?

> to explode. Have you tested this? It is the sort of thing which does
> not happen very often, so i expect is broken.

I have not run my system under memory pressure or done any error
injection testing. Is that customary to do whenever using kmalloc?  If
it would make a difference I could look into setting that up.

I have certainly tried my best to audit all the possible error paths to
make sure that no memory is ever leaked, and I feel confident in that
analysis.