diff mbox series

[net-next] net: add annotation for sock_{lock,unlock}_fast

Message ID 95cf587fe96127884e555f695fe519d50e63cc17.1605522868.git.pabeni@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show
Series [net-next] net: add annotation for sock_{lock,unlock}_fast | expand

Commit Message

Paolo Abeni Nov. 16, 2020, 10:36 a.m. UTC
The static checker is fooled by the non-static locking scheme
implemented by the mentioned helpers.
Let's make its life easier adding some unconditional annotation
so that the helpers are now interpreted as a plain spinlock from
sparse.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sock.h | 9 ++++++---
 net/core/sock.c    | 3 ++-
 2 files changed, 8 insertions(+), 4 deletions(-)

Comments

Luc Van Oostenryck Nov. 16, 2020, 10:27 p.m. UTC | #1
On Mon, Nov 16, 2020 at 11:36:39AM +0100, Paolo Abeni wrote:
> The static checker is fooled by the non-static locking scheme
> implemented by the mentioned helpers.
> Let's make its life easier adding some unconditional annotation
> so that the helpers are now interpreted as a plain spinlock from
> sparse.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/net/sock.h | 9 ++++++---
>  net/core/sock.c    | 3 ++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 1d29aeae74fd..60d321c6b5a5 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1595,7 +1595,8 @@ void release_sock(struct sock *sk);
>  				SINGLE_DEPTH_NESTING)
>  #define bh_unlock_sock(__sk)	spin_unlock(&((__sk)->sk_lock.slock))
>  
> -bool lock_sock_fast(struct sock *sk);
> +bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock);
> +

Good.

>  /**
>   * unlock_sock_fast - complement of lock_sock_fast
>   * @sk: socket
> @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
>   */
>  static inline void unlock_sock_fast(struct sock *sk, bool slow)
>  {
> -	if (slow)
> +	if (slow) {
>  		release_sock(sk);
> -	else
> +		__release(&sk->sk_lock.slock);

The correct solution would be to annotate the declaration of
release_sock() with '__releases(&sk->sk_lock.slock)'.

>  /* Used by processes to "lock" a socket state, so that
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 727ea1cc633c..9badbe7bb4e4 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3078,7 +3078,7 @@ EXPORT_SYMBOL(release_sock);
>   *
>   *   sk_lock.slock unlocked, owned = 1, BH enabled
>   */
> -bool lock_sock_fast(struct sock *sk)
> +bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock)
>  {
>  	might_sleep();
>  	spin_lock_bh(&sk->sk_lock.slock);
> @@ -3096,6 +3096,7 @@ bool lock_sock_fast(struct sock *sk)
>  	 * The sk_lock has mutex_lock() semantics here:
>  	 */
>  	mutex_acquire(&sk->sk_lock.dep_map, 0, 0, _RET_IP_);
> +	__acquire(&sk->sk_lock.slock);

OK, given that the mutexes are not annotated.

-- Luc
Paolo Abeni Nov. 17, 2020, 8:38 a.m. UTC | #2
Hello,

Thank you for the feedback!

On Mon, 2020-11-16 at 23:27 +0100, Luc Van Oostenryck wrote:
> > @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> >   */
> >  static inline void unlock_sock_fast(struct sock *sk, bool slow)
> >  {
> > -	if (slow)
> > +	if (slow) {
> >  		release_sock(sk);
> > -	else
> > +		__release(&sk->sk_lock.slock);
> 
> The correct solution would be to annotate the declaration of
> release_sock() with '__releases(&sk->sk_lock.slock)'.

If I add such annotation to release_sock(), I'll get several sparse
warnings for context imbalance (on each lock_sock()/release_sock()
pair), unless I also add an '__acquires()' annotation to lock_sock(). 

The above does not look correct to me ?!? When release_sock() completes
the socket spin lock is not held. The annotation added above is
somewhat an artifact to let unlock_sock_fast() matches lock_sock_fast()
from sparse perspective. I intentionally avoided changing
the release_sock() annotation to avoid introducing more artifacts.

The proposed schema is not 100% accurate, as it will also allow e.g. a
really-not-fitting bh_lock_sock()/unlock_sock_fast() pair, but I could
not come-up with anything better.

Can we go with the schema I proposed?

Thanks,

Paolo
Luc Van Oostenryck Nov. 17, 2020, 4:58 p.m. UTC | #3
On Tue, Nov 17, 2020 at 09:38:45AM +0100, Paolo Abeni wrote:
> Hello,
> 
> Thank you for the feedback!
> 
> On Mon, 2020-11-16 at 23:27 +0100, Luc Van Oostenryck wrote:
> > > @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> > >   */
> > >  static inline void unlock_sock_fast(struct sock *sk, bool slow)
> > >  {
> > > -	if (slow)
> > > +	if (slow) {
> > >  		release_sock(sk);
> > > -	else
> > > +		__release(&sk->sk_lock.slock);
> > 
> > The correct solution would be to annotate the declaration of
> > release_sock() with '__releases(&sk->sk_lock.slock)'.
> 
> If I add such annotation to release_sock(), I'll get several sparse
> warnings for context imbalance (on each lock_sock()/release_sock()
> pair), unless I also add an '__acquires()' annotation to lock_sock(). 
> 
> The above does not look correct to me ?!? When release_sock() completes
> the socket spin lock is not held.

Yes, that's fine, but I suppose it somehow releases the mutex that
is taken in lock_sock_fast() when returning true, right?

> The annotation added above is
> somewhat an artifact to let unlock_sock_fast() matches lock_sock_fast()
> from sparse perspective. I intentionally avoided changing
> the release_sock() annotation to avoid introducing more artifacts.
> 
> The proposed schema is not 100% accurate, as it will also allow e.g. a
> really-not-fitting bh_lock_sock()/unlock_sock_fast() pair, but I could
> not come-up with anything better.
> 
> Can we go with the schema I proposed?

Well, I suppose it's a first step.
But can you then add a '__releases(...)' to unlock_sock_fast()?
It's not needed by sparse because it's an inline function and sparse
can then deduce it but it will help to see the pairing with
lock_sock_fast() is OK.

-- Luc
Paolo Abeni Nov. 17, 2020, 5:36 p.m. UTC | #4
Hello,

On Tue, 2020-11-17 at 17:58 +0100, Luc Van Oostenryck wrote:
> On Tue, Nov 17, 2020 at 09:38:45AM +0100, Paolo Abeni wrote:
> > Hello,
> > 
> > Thank you for the feedback!
> > 
> > On Mon, 2020-11-16 at 23:27 +0100, Luc Van Oostenryck wrote:
> > > > @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> > > >   */
> > > >  static inline void unlock_sock_fast(struct sock *sk, bool slow)
> > > >  {
> > > > -	if (slow)
> > > > +	if (slow) {
> > > >  		release_sock(sk);
> > > > -	else
> > > > +		__release(&sk->sk_lock.slock);
> > > 
> > > The correct solution would be to annotate the declaration of
> > > release_sock() with '__releases(&sk->sk_lock.slock)'.
> > 
> > If I add such annotation to release_sock(), I'll get several sparse
> > warnings for context imbalance (on each lock_sock()/release_sock()
> > pair), unless I also add an '__acquires()' annotation to lock_sock(). 
> > 
> > The above does not look correct to me ?!? When release_sock() completes
> > the socket spin lock is not held.
> 
> Yes, that's fine, but I suppose it somehow releases the mutex that
> is taken in lock_sock_fast() when returning true, right?

Well, it has mutex semantics, but does not really acquire any mutex.

> > The annotation added above is
> > somewhat an artifact to let unlock_sock_fast() matches lock_sock_fast()
> > from sparse perspective. I intentionally avoided changing
> > the release_sock() annotation to avoid introducing more artifacts.
> > 
> > The proposed schema is not 100% accurate, as it will also allow e.g. a
> > really-not-fitting bh_lock_sock()/unlock_sock_fast() pair, but I could
> > not come-up with anything better.
> > 
> > Can we go with the schema I proposed?
> 
> Well, I suppose it's a first step.
> But can you then add a '__releases(...)' to unlock_sock_fast()?
> It's not needed by sparse because it's an inline function and sparse
> can then deduce it but it will help to see the pairing with
> lock_sock_fast() is OK.

Ok, I'll send a v2 with such annotation.

Thanks!

Paolo
diff mbox series

Patch

diff --git a/include/net/sock.h b/include/net/sock.h
index 1d29aeae74fd..60d321c6b5a5 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1595,7 +1595,8 @@  void release_sock(struct sock *sk);
 				SINGLE_DEPTH_NESTING)
 #define bh_unlock_sock(__sk)	spin_unlock(&((__sk)->sk_lock.slock))
 
-bool lock_sock_fast(struct sock *sk);
+bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock);
+
 /**
  * unlock_sock_fast - complement of lock_sock_fast
  * @sk: socket
@@ -1606,10 +1607,12 @@  bool lock_sock_fast(struct sock *sk);
  */
 static inline void unlock_sock_fast(struct sock *sk, bool slow)
 {
-	if (slow)
+	if (slow) {
 		release_sock(sk);
-	else
+		__release(&sk->sk_lock.slock);
+	} else {
 		spin_unlock_bh(&sk->sk_lock.slock);
+	}
 }
 
 /* Used by processes to "lock" a socket state, so that
diff --git a/net/core/sock.c b/net/core/sock.c
index 727ea1cc633c..9badbe7bb4e4 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3078,7 +3078,7 @@  EXPORT_SYMBOL(release_sock);
  *
  *   sk_lock.slock unlocked, owned = 1, BH enabled
  */
-bool lock_sock_fast(struct sock *sk)
+bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock)
 {
 	might_sleep();
 	spin_lock_bh(&sk->sk_lock.slock);
@@ -3096,6 +3096,7 @@  bool lock_sock_fast(struct sock *sk)
 	 * The sk_lock has mutex_lock() semantics here:
 	 */
 	mutex_acquire(&sk->sk_lock.dep_map, 0, 0, _RET_IP_);
+	__acquire(&sk->sk_lock.slock);
 	local_bh_enable();
 	return true;
 }