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] [day] [month] [year] [list]

Message-ID: <VI1PR0402MB280082F2CFC49BAB2E4CAB23E0770@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Date:   Tue, 12 Nov 2019 15:18:59 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Simon Horman <simon.horman@...ronome.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <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")
> 

The problem is not present in the initial driver.
The logic before the d7f5a9d89a55 ("dpaa2-eth: defer probe on object allocate") patch
was that if there are not enough channels available it will go ahead with less than the optimal
value. 

> > 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?

You're right, cleaning up the channels should be done just on the EPROBE_DEFER path,
in the if statement, and not for the entire code path. 
I'll rework this.

Thanks for pointing this out.

Ioana

> 
> >
> > --
> > 1.9.1
> >