Messages in this thread Patch in this message | | | Date | Fri, 11 Mar 2022 13:53:44 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb_del() |
| |
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.
> 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; ...
> @@ -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?
> 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
| |