netdev - Re: [PATCH net v2] dpaa2-eth: free already allocated channels on probe defer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191112145714.ohlnx6pmpkqxs5qs@netronome.com>
Date:   Tue, 12 Nov 2019 15:57:15 +0100
From:   Simon Horman <simon.horman@...ronome.com>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net v2] dpaa2-eth: free already allocated channels on
 probe defer

On Tue, Nov 12, 2019 at 04:24:53PM +0200, Ioana Ciornei wrote:
> The setup_dpio() function tries to allocate a number of channels equal
> to the number of CPUs online. When there are not enough DPCON objects
> already probed, the function will return EPROBE_DEFER. When this
> happens, the already allocated channels are not freed. This results in
> the incapacity of properly probing the next time around.
> Fix this by freeing the channels on the error path.
> 
> Fixes: d7f5a9d89a55 ("dpaa2-eth: defer probe on object allocate")

Its not clear to me that this clean-up problem was added by
the defer change. But rather, looking at the git logs, it seems
likely to have been present since the driver was added by:

6e2387e8f19e ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver")

> Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> ---
> Changes in v2:
>  - add the proper Fixes tag
> 
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 19379bae0144..22e9519f65bb 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -2232,6 +2232,14 @@ static int setup_dpio(struct dpaa2_eth_priv *priv)
>  err_service_reg:
>  	free_channel(priv, channel);
>  err_alloc_ch:
> +	for (i = 0; i < priv->num_channels; i++) {
> +		channel = priv->channel[i];
> +		nctx = &channel->nctx;
> +		dpaa2_io_service_deregister(channel->dpio, nctx, dev);
> +		free_channel(priv, channel);
> +	}
> +	priv->num_channels = 0;
> +
>  	if (err == -EPROBE_DEFER)
>  		return err;

This function goes on to return 0 unless cpumask_empty(&priv->dpio_cpumask)
is zero. Given this is an errorr path and the clean-up above, is that correct?

>  
> -- 
> 1.9.1
>