Re: [PATCH v1 2/2] hamradio: defer 6pack kfree after unregister_netdev
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-11-11 02:05:30
Also in:
lkml
On Mon, 8 Nov 2021 18:37:59 +0800 Lin Ma wrote:
There is a possible race condition (use-after-free) like below
(USE) | (FREE)
dev_queue_xmit |
__dev_queue_xmit |
__dev_xmit_skb |
sch_direct_xmit | ...
xmit_one |
netdev_start_xmit | tty_ldisc_kill
__netdev_start_xmit | 6pack_close
sp_xmit | kfree
sp_encaps |
|
According to the patch "defer ax25 kfree after unregister_netdev", thisThis is the previous patch of the series? Maybe call it "previous patch"?
patch reorder the kfree after the unregister_netdev to avoid the possible UAF as the unregister_netdev() is well synchronized and won't return if there is a running routine. Signed-off-by: Lin Ma <redacted>
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c index 49f10053a794..bfdf89e54752 100644 --- a/drivers/net/hamradio/6pack.c +++ b/drivers/net/hamradio/6pack.c@@ -672,11 +672,13 @@ static void sixpack_close(struct tty_struct *tty) del_timer_sync(&sp->tx_t); del_timer_sync(&sp->resync_t); - /* Free all 6pack frame buffers. */ + unregister_netdev(sp->dev); + + /* Free all 6pack frame buffers after unreg. */ kfree(sp->rbuff); kfree(sp->xbuff); - unregister_netdev(sp->dev); + free_netdev(sp->dev);
You should mention in the commit message why you think it's safe to add free_netdev() which wasn't here before... This driver seems to be setting: dev->needs_free_netdev = true; so unregister_netdev() will free the netdev automatically. That said I don't see a reason why this driver needs the automatic cleanup. You can either remove that setting and then you can call free_netdev() like you do, or you need to move the cleanup to dev->priv_destructor.
} /* Perform I/O control on an active 6pack channel. */