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

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: <4d88cd641c25cc231fcbad62c19cae60749bb171.camel@redhat.com>
Date: Tue, 19 Sep 2023 12:41:42 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Taehee Yoo <ap420073@...il.com>, davem@...emloft.net, kuba@...nel.org, 
	edumazet@...gle.com, netdev@...r.kernel.org, 
	syzbot+9bbbacfbf1e04d5221f7@...kaller.appspotmail.com, 
	syzbot+1c71587a1a09de7fbde3@...kaller.appspotmail.com
Subject: Re: [PATCH net v2] net: team: get rid of team->lock in team module

On Tue, 2023-09-19 at 12:32 +0200, Jiri Pirko wrote:
> Tue, Sep 19, 2023 at 09:40:53AM CEST, pabeni@...hat.com wrote:
> > On Sat, 2023-09-16 at 18:47 +0200, Jiri Pirko wrote:
> > > Sat, Sep 16, 2023 at 03:11:15PM CEST, ap420073@...il.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.

Well, let's see if Taehee can came up with something addressing that. I
think that if such problem proves to be too hard, we should consider
other options.

Cheers,

Paolo