Re: [PATCH net v2] net: team: get rid of team->lock in team module

On Sat, 2023-09-16 at 18:47 +0200, Jiri Pirko wrote:

Sat, Sep 16, 2023 at 03:11:15PM CEST, ap420073@gmail.com wrote:

quoted

The purpose of team->lock is to protect the private data of the team
interface. But RTNL already protects it all well.
The precise purpose of the team->lock is to reduce contention of
RTNL due to GENL operations such as getting the team port list, and
configuration dump.

team interface has used a dynamic lockdep key to avoid false-positive
lockdep deadlock detection. Virtual interfaces such as team usually
have their own lock for protecting private data.
These interfaces can be nested.
team0
 |
team1

Each interface's lock is actually different(team0->lock and team1->lock).
So,
mutex_lock(&team0->lock);
mutex_lock(&team1->lock);
mutex_unlock(&team1->lock);
mutex_unlock(&team0->lock);
The above case is absolutely safe. But lockdep warns about deadlock.
Because the lockdep understands these two locks are same. This is a
false-positive lockdep warning.

So, in order to avoid this problem, the team interfaces started to use
dynamic lockdep key. The false-positive problem was fixed, but it
introduced a new problem.

When the new team virtual interface is created, it registers a dynamic
lockdep key(creates dynamic lockdep key) and uses it. But there is the
limitation of the number of lockdep keys.
So, If so many team interfaces are created, it consumes all lockdep keys.
Then, the lockdep stops to work and warns about it.

What about fixing the lockdep instead? I bet this is not the only
occurence of this problem.

I think/fear that solving the max key lockdep problem could be
problematic hard and/or requiring an invasive change.

Is there any real use-case requiring team devices being nested one to
each other? If not, can we simply prevent such nesting in
team_port_add()? I'm guessing that syzkaller can find more ways to
exploit such complex setup.

Cheers,

Paolo