Re: [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb_del()
From: Dan Carpenter <hidden>
Date: 2022-03-11 10:54:36
Also in:
linux-hams, lkml
On Fri, Mar 11, 2022 at 09:46:24AM +0800, Duoming Zhou wrote:
This patch adds a flag in ax25_dev in order to prevent reference count leaks. If the above condition happens, the "test_bit" condition check in ax25_kill_by_device() could pass and the refcounts could be decreased properly.
Why are you using test_bit()? Just use booleans. I am not a networking person but this whole thing feel like adding on layers of cruft. If refcounting needs a lot of additional tracking information then probably the reference counts are not being done in the correct location.
quoted hunk ↗ jump to hunk
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c index 6bd09718077..fc564b87acc 100644 --- a/net/ax25/af_ax25.c +++ b/net/ax25/af_ax25.c@@ -86,6 +86,7 @@ static void ax25_kill_by_device(struct net_device *dev) again: ax25_for_each(s, &ax25_list) { if (s->ax25_dev == ax25_dev) { + set_bit(AX25_DEV_KILL, &ax25_dev->flag); sk = s->sk; if (!sk) { spin_unlock_bh(&ax25_list_lock);
Why can this not be a local variable.
bool found = false;
ax25_for_each(s, &ax25_list) {
if (s->ax25_dev == ax25_dev) {
found = true;
...
quoted hunk ↗ jump to hunk
@@ -115,6 +116,10 @@ static void ax25_kill_by_device(struct net_device *dev) } } spin_unlock_bh(&ax25_list_lock); + if (!test_bit(AX25_DEV_KILL, &ax25_dev->flag) && test_bit(AX25_DEV_BIND, &ax25_dev->flag)) {
The comments for ax25_kill_by_device() say:
/*
* Kill all bound sockets on a dropped device.
*/
So how can test_bit(AX25_DEV_BIND, &ax25_dev->flag) ever be false at
this location?
if (!found) {
dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
ax25_dev_put(ax25_dev);
}
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?
quoted hunk ↗ jump to hunk
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c index d2a244e1c26..9b04d74a1be 100644 --- a/net/ax25/ax25_dev.c +++ b/net/ax25/ax25_dev.c@@ -77,6 +77,7 @@ void ax25_dev_device_up(struct net_device *dev) ax25_dev->values[AX25_VALUES_PACLEN] = AX25_DEF_PACLEN; ax25_dev->values[AX25_VALUES_PROTOCOL] = AX25_DEF_PROTOCOL; ax25_dev->values[AX25_VALUES_DS_TIMEOUT]= AX25_DEF_DS_TIMEOUT; + ax25_dev->flag = AX25_DEV_INIT;
There is no need for this, it's allocated with kzalloc().
#if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER) ax25_ds_setup_timer(ax25_dev);
regards, dan carpenter