netdev - Re: [PATCH v3 net-next] net: dsa: reference count the host mdb addresses

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]

Date:   Sat, 12 Dec 2020 22:18:59 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [PATCH v3 net-next] net: dsa: reference count the host mdb
 addresses

On Sat, Dec 12, 2020 at 11:06:41PM +0100, Andrew Lunn wrote:
> > +	/* Complication created by the fact that addition has two phases, but
> > +	 * deletion only has one phase, and we need reference counting.
> > +	 * The strategy is to do the memory allocation in the prepare phase,
> > +	 * but initialize the refcount in the commit phase.
> > +	 *
> > +	 * Have mdb	| mdb has refcount > 0	| Commit phase	| Resolution
> > +	 * -------------+-----------------------+---------------+---------------
> > +	 * no		| -			| no		| Alloc & proceed
>
> This does not look right.
>
> The point of the prepare phase is to allow all the different layers
> involved to allocate whatever they need and to validate they can do
> the requested action. Any layer can say, No, stop, i cannot do
> this. The commit phase will then not happen. But that also means the
> prepare phase should not do any state changes. So you should not be
> proceeding here, just allocating.

Are you commenting based on the code, or just based on the comment?
If just based on the comment, then yeah, sorry. I was limited to 80
characters, and I couldn't specify "proceed to what". It's just "proceed
to call the prepare phase of the driver". Which is... normal and
expected, and does not contradict what you said above.

> And you need some way to cleanup the allocated memory when the commit
> never happens because some other layer has said No!

So this would be a fatal problem with the switchdev transactional model
if I am not misunderstanding it. On one hand there's this nice, bubbly
idea that you should preallocate memory in the prepare phase, so that
there's one reason less to fail at commit time. But on the other hand,
if "the commit phase might never happen" is even a remove possibility,
all of that goes to trash - how are you even supposed to free the
preallocated memory.

Sorry, I don't think that there's any possibility for the commit phase
to not happen as long as everybody is in agreement that the preparation
phase went ok. If you look at the code, I even allocated the memory at
preparation time _before_ calling into the driver, to ensure that we're
not giving the driver the false impression that it gave switchdev the
green light but the commit never came. If our allocation in the DSA core
fails during the prepare phase, the prepare phase of the driver is not
even called.

That being said, please let me know if you spot bugs in the actual code.
I tested it and it appeared to work ok (I also put debugging prints to
make sure that the refcounting works ok and the entries are removed
after all of them expire).