Received-SPF: pass (google.com: domain of linux-kernel+bounces-139857-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 11 Apr 2024 09:31:31 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Sam Sun <samsun1006219@gmail.com>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	linux-hams@vger.kernel.org, pabeni@redhat.com, kuba@kernel.org,
	jreuter@yaina.de, davem@davemloft.net,
	Eric Dumazet <edumazet@google.com>, syzkaller-bugs@googlegroups.com,
	xrivendell7@gmail.com
Subject: Re: [Linux kernel bug] WARNING in ax25_dev_device_down
Message-ID: <26760fb7-a2d8-4cb5-9c47-3f91016a9a7a@moroto.mountain>
References: <CAEkJfYNYe5tSMfAn9K_zJ1O_Vu7jxJDZv_uYCgTW=NZiUzcAuw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEkJfYNYe5tSMfAn9K_zJ1O_Vu7jxJDZv_uYCgTW=NZiUzcAuw@mail.gmail.com>

On Tue, Apr 09, 2024 at 10:02:49PM +0800, Sam Sun wrote:
> Dear developers and maintainers,
> 
> We encountered a kernel warning in function ax25_dev_device_down
> during testing using our modified syzkaller. It is tested against the
> latest upstream linux (6.9-rc3). C repro and kernel config are
> attached to this email. Kernel log is listed below.
> ```
> WARNING: CPU: 0 PID: 8121 at lib/ref_tracker.c:255
> ref_tracker_free+0x610/0x830 lib/ref_tracker.c:255
> Modules linked in:
> CPU: 0 PID: 8121 Comm: syz-executor329 Not tainted 6.7.0-rc7 #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:ref_tracker_free+0x610/0x830 lib/ref_tracker.c:255
> Code: 00 00 44 8b 73 18 31 ff 44 89 f6 e8 7a 58 fe fc 45 85 f6 0f 85
> a7 00 00 00 e8 4c 5c fe fc 4c 89 ee 48 89 ef e8 11 0a e5 05 90 <0f> 0b
> 90 41 bd ea ff ff ff e9 51 fd ff ff e8 2d 5c fe fc 4c 8d 75
> RSP: 0018:ffffc900029bf8b8 EFLAGS: 00010286
> RAX: 0000000080000000 RBX: ffff888106185480 RCX: 0000000000000000
> RDX: 0000000000000001 RSI: 0000000000000001 RDI: 0000000000000001
> RBP: ffff8881136c85b8 R08: 0000000000000001 R09: fffffbfff23e8bd1
> R10: 0000000000000001 R11: 0000000000000001 R12: 1ffff92000537f19
> R13: 0000000000000292 R14: 00000000067a01d1 R15: ffff888106185498
> FS:  0000555556e663c0(0000) GS:ffff888063a00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fc148041ba8 CR3: 0000000016374000 CR4: 0000000000750ef0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  netdev_tracker_free include/linux/netdevice.h:4127 [inline]
>  netdev_put include/linux/netdevice.h:4144 [inline]
>  netdev_put include/linux/netdevice.h:4140 [inline]
>  ax25_dev_device_down+0x2bc/0x420 net/ax25/ax25_dev.c:140
                                                       ^^^^

The locking in ax25_dev_device_down() seems pretty suspect.

It takes:
	ax25_dev = ax25_dev_ax25dev()
then it takes the &ax25_dev_lock, then it drops the lock, then on line
140 mentioned in this stack trace it calls:

	netdev_put(dev, &ax25_dev->dev_tracker);

That can race with ax25_dev_free() which has proper locking.  The
temptation is to do something like this, but I don't know this code well
and can't test it.

regards,
dan carpenter

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index 282ec581c072..68c2945d6051 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -97,13 +97,15 @@ void ax25_dev_device_down(struct net_device *dev)
 {
 	ax25_dev *s, *ax25_dev;
 
-	if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL)
+	spin_lock_bh(&ax25_dev_lock);
+
+	if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL) {
+		spin_unlock_bh(&ax25_dev_lock);
 		return;
+	}
 
 	ax25_unregister_dev_sysctl(ax25_dev);
 
-	spin_lock_bh(&ax25_dev_lock);
-
 #ifdef CONFIG_AX25_DAMA_SLAVE
 	timer_shutdown_sync(&ax25_dev->dama.slave_timer);
 #endif
@@ -128,17 +130,17 @@ void ax25_dev_device_down(struct net_device *dev)
 
 		s = s->next;
 	}
-	spin_unlock_bh(&ax25_dev_lock);
 	dev->ax25_ptr = NULL;
 	ax25_dev_put(ax25_dev);
+	spin_unlock_bh(&ax25_dev_lock);
 	return;
 
 unlock_put:
-	spin_unlock_bh(&ax25_dev_lock);
 	ax25_dev_put(ax25_dev);
 	dev->ax25_ptr = NULL;
 	netdev_put(dev, &ax25_dev->dev_tracker);
 	ax25_dev_put(ax25_dev);
+	spin_unlock_bh(&ax25_dev_lock);
 }
 
 int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd)