Re: [PATCH V3] ax25: Fix refcount leaks caused by ax25_cb_del()

[Dan Carpenter <dan.carpenter@oracle.com>]

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