On Fri, Apr 01, 2016 at 06:12:49AM +0200, Hannes Frederic Sowa wrote:

On 01.04.2016 06:04, Alexei Starovoitov wrote:

quoted

On Thu, Mar 31, 2016 at 08:03:38PM -0700, Eric Dumazet wrote:

quoted

On Thu, 2016-03-31 at 18:45 -0700, Alexei Starovoitov wrote:

quoted

Eric, what's your take on Hannes's patch 2 ?
Is it more accurate to ask lockdep to check for actual lock
or lockdep can rely on owned flag?
Potentially there could be races between setting the flag and
actual lock... but that code is contained, so unlikely.
Will we find the real issues with this 'stronger' check or
just spend a ton of time adapting to new model like your other
patch for release_sock and whatever may need to come next...

More precise lockdep checks are certainly good, I only objected to 4/4
trying to work around another bug.

But why do we rush for 'net' tree ?

This looks net-next material to me.

Locking changes are often subtle, lets take the time to do them
properly.

completely agree. I think only first patch belongs in net.
Everything else is net-next material.

Problem with first patch is that it uses lock_sock_fast, thus the current
sock_owned_by_user check doesn't get rid the lockdep warning. :/

Thus we would need to go with the two first patches. Do you think it is
acceptable? I actually didn't see a problem and testing showed no problems
so far.

I see. right. the patch 1 only makes sense when coupled with 2.
but now I'm not so sure that lockdep_is_held(&sk->sk_lock.slock)
is a valid check, since current sock_owned_by_user() is equivalent
to lockdep_is_held(&sk->sk_lock) only.
I would go with Daniel's approach. Much simpler to reason about.