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

[Jiri Pirko <jiri@resnulli.us>]

Tue, Sep 19, 2023 at 09:40:53AM CEST, pabeni@redhat.com wrote:
>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:
>> > 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.

But it would solve this false warnings not only here but for many
others.


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

I see no such usecase. However, if someone is using it this way now, we
should not break him.


>
>Cheers,
>
>Paolo
>