netdev - RE: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

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]

Date:   Fri, 9 Nov 2018 14:01:56 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>
Subject: RE: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

> > The fsl_mc_object_allocate function can fail because not all
> > allocatable objects are probed by the fsl_mc_allocator at the call
> > time. Defer the dpaa2-eth probe when this happens.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30
> > +++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > index 88f7acc..71f5cd4 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > @@ -1434,8 +1434,11 @@ static struct fsl_mc_device *setup_dpcon(struct
> dpaa2_eth_priv *priv)
> >  	err = fsl_mc_object_allocate(to_fsl_mc_device(dev),
> >  				     FSL_MC_POOL_DPCON, &dpcon);
> >  	if (err) {
> > -		dev_info(dev, "Not enough DPCONs, will go on as-is\n");
> > -		return NULL;
> > +		if (err == -ENXIO)
> > +			err = -EPROBE_DEFER;
> > +		else
> > +			dev_info(dev, "Not enough DPCONs, will go on as-
> is\n");
> > +		return ERR_PTR(err);
> >  	}
> >
> >  	err = dpcon_open(priv->mc_io, 0, dpcon->obj_desc.id,
> > &dpcon->mc_handle); @@ -1493,8 +1496,10 @@ static void
> free_dpcon(struct dpaa2_eth_priv *priv,
> >  		return NULL;
> >
> >  	channel->dpcon = setup_dpcon(priv);
> > -	if (!channel->dpcon)
> > +	if (IS_ERR_OR_NULL(channel->dpcon)) {
> > +		err = PTR_ERR(channel->dpcon);
> >  		goto err_setup;
> > +	}
> 
> Hi Ioana
> 
> You need to be careful with IS_ERR_OR_NULL(). If it is a NULL,
> PTR_ERR() is going to return 0. You then jump to the error cleanup code, but
> return 0, meaning everything is O.K.
> 

Hi Andrew,

I took a closer look at the code path and it seems that if channel->dpcon in the snippet above is NULL,
then indeed PTR_ERR will be 0 but in the error cleanup code, in this case the err_setup label, 
a reverse ERR_PTR (NULL in this case) will be returned.
Continuing on the code path, alloc_channel then returns NULL and is handled by the following snippet.

+		if (IS_ERR_OR_NULL(channel)) {
+			err = PTR_ERR(channel);
+			if (err != -EPROBE_DEFER)
+				dev_info(dev,
+					 "No affine channel for cpu %d and above\n", i);
 			goto err_alloc_ch;
 		}
  
In case channel is NULL, then the dev_info will be called and the jump to the cleanup is made.

err_alloc_ch:
+	if (err == -EPROBE_DEFER)
+		return err;
+
 	if (cpumask_empty(&priv->dpio_cpumask)) {
 		dev_err(dev, "No cpu with an affine DPIO/DPCON\n");
 		return err;

Here err is 0 so in case the cpumask is empty, 0 will be returned, which is not the intended use.
I will send a v2 changing the return value to -ENODEV in case no cpus with an affine DPIO is found.

Thanks,
Ioana