Hello,

On Mon, 14 Mar 2022 14:02:56 +0300, Dan Carpen wrote:

quoted

quoted

But even here, my instinct is that if the refcounting is were done in
the correct place we would not need any additional variables.  Is there
no simpler solution?

I think there is a simpler solution instead of using any additional variables,
which is shown below:

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 6bd09718077..0886109421a 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c

@@ -98,8 +98,10 @@ static void ax25_kill_by_device(struct net_device *dev)
 			spin_unlock_bh(&ax25_list_lock);
 			lock_sock(sk);
 			s->ax25_dev = NULL;
-			dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
-			ax25_dev_put(ax25_dev);
+			if (sk->sk_wq) {
+				dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+				ax25_dev_put(ax25_dev);
+			}
 			ax25_disconnect(s, ENETUNREACH);
 			release_sock(sk);
 			spin_lock_bh(&ax25_list_lock);

@@ -979,14 +981,20 @@ static int ax25_release(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
 	ax25_cb *ax25;
+	ax25_dev *ax25_dev;
 
 	if (sk == NULL)
 		return 0;
 
 	sock_hold(sk);
-	sock_orphan(sk);
 	lock_sock(sk);
+	sock_orphan(sk);
 	ax25 = sk_to_ax25(sk);
+	ax25_dev = ax25->ax25_dev;
+	if (ax25_dev) {
+		dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+		ax25_dev_put(ax25_dev);
+	}
 
 	if (sk->sk_type == SOCK_SEQPACKET) {
 		switch (ax25->state) {

we add decrements of refcounts in ax25_release(), and use lock_sock() to do synchronization. 
If refcounts decrease in ax25_release(), the decrements of refcounts in ax25_kill_by_device() 
will not be executed and vice versa. 

1. If we decrease the refcounts in ax25_release(), the decrements of refcounts in 
ax25_kill_by_device() will not execute. Because we set NULL to sk->sk_wq in sock_orphan()
and we check whether sk->sk_wq is NULL in ax25_kill_by_device(). Only positions (3) and (4)
could execute.

     (Thread 1)                      |      
ax25_bind()                          |
 ...                                 |
 ax25_addr_ax25dev()                 |
  ax25_dev_hold()   //(1)            |
  ...                                |
 dev_hold_track()   //(2)            |     (Thread 2)
 ...                                 | ax25_release()
                                     |  ...
                                     |   lock_sock(sk)
                                     |   sock_orphan(sk)
                                     |     sk->sk_wq  = NULL //set NULL
                                     |   ...
                                     |   if (ax25_dev) {
                                     |    dev_put_track() //(3)
                                     |    ax25_dev_put() //(4)
                                     |
     (thread 3)                      |
ax25_kill_by_device()                |
 ...                                 |
 lock_sock(sk)                       |
 s->ax25_dev = NULL                  |
 if (sk->sk_wq) { //check            |
  dev_put_track() //(5)              |
  ax25_dev_put() //(6)               |


2. If we decrease refcounts in ax25_kill_by_device(), the decrements of refcounts 
in ax25_release() will not execute. Because we set NULL to s->ax25_dev in ax25_kill_by_device()
and we check whether ax25_dev is NULL in ax25_release(). Only positions (3) and (4)
could execute.

     (Thread 1)                      | 
ax25_bind()                          |
 ...                                 |
 ax25_addr_ax25dev()                 |
  ax25_dev_hold()   //(1)            |
  ...                                |
 dev_hold_track()   //(2)            |      (Thread 2)
 ...                                 | ax25_kill_by_device()
                                     |  ...
                                     |   lock_sock(sk)
                                     |   s->ax25_dev = NULL //set NULL
                                     |   if (sk->sk_wq) {
                                     |    dev_put_track() //(3)
                                     |    ax25_dev_put() //(4)
                                     |    ...
                                     |
     (thread 3)                      |
ax25_release()                       |
 ...                                 |
 lock_sock(sk);                      |
 sock_orphan(sk);                    |
 ...                                 |
 if (ax25_dev) { //check             |
  dev_put_track() //(5)              |
  ax25_dev_put() //(6)               |

quoted

I sent "[PATCH net V2 1/2] ax25: Fix refcount leaks caused by ax25_cb_del()"
on On Fri, Mar 11, 2022. Could this patch solve your question?

I had a bunch of questions...  You just ignored them, and sent a patch
called v2 instead of v4 so I was puzzled and confused. I guess the
answer is no, could you please answer the questions?

I hope my answer could solve your questions. If you still have any questions
welcome to send email to me. I will send "[PATCH net V4 1/2] ax25: Fix
refcount leaks caused by ax25_cb_del()" as soon as possible. 

What`s more, I found NPD bugs in ax25 timers, I will send
"[PATCH net V4 2/2] ax25: Fix NULL pointer dereferences in ax25 timers" together.

Best wishes,
Duoming Zhou