Re: [PATCH] net: dsa: loop: Check for memory allocation failure
On Mon, 2017-05-08 at 17:35 -0700, Florian Fainelli wrote: > On 05/08/2017 04:46 PM, Julia Lawall wrote: > > On Mon, 8 May 2017, Joe Perches wrote: > > > Each time -EPROBE_DEFER occurs, another set of calls to > > > dsa_switch_alloc and dev_kzalloc also occurs. > > > > > > Perhaps it'd be better to do: > > > > > > if (ps->netdev) { > > > devm_kfree(>dev, ps); > > > devm_kfree(>dev, ds); > > > return -EPROBE_DEFER; > > > } > > > > Is EPROBE_DEFER handled differently than other kinds of errors? > > In the core device driver model, yes, EPROBE_DEFER is treated > differently than other errors because it puts the driver on a retry queue. > > EPROBE_DEFER is already a slow and exceptional path, and this is a > mock-up driver, so I am not sure what value there is in trying to > balance devm_kzalloc() with corresponding devm_kfree()... Example code should be as correct as possible.
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
On Mon, 8 May 2017, Florian Fainelli wrote: > On 05/08/2017 04:46 PM, Julia Lawall wrote: > > > > > > On Mon, 8 May 2017, Joe Perches wrote: > > > >> On Mon, 2017-05-08 at 20:32 +0800, Julia Lawall wrote: > >>> > >>> On Mon, 8 May 2017, David Laight wrote: > >>> > From: Christophe JAILLET > > Sent: 06 May 2017 06:30 > > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > > Return -ENOMEM instead, as done for some other memory allocation just a > > few lines above. > > ... > > --- a/drivers/net/dsa/dsa_loop.c > > +++ b/drivers/net/dsa/dsa_loop.c > > @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device > > *mdiodev) > > return -ENOMEM; > > > > ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); > > + if (!ps) > > + return -ENOMEM; > > + > > ps->netdev = dev_get_by_name(_net, pdata->netdev); > > if (!ps->netdev) > > return -EPROBE_DEFER; > > On the face if it this code leaks like a sieve. > >>> > >>> I don't think so. The allocations (dsa_switch_alloc and devm_kzalloc) use > >>> devm functions. > >> > >> It's at least wasteful. > >> > >> Each time -EPROBE_DEFER occurs, another set of calls to > >> dsa_switch_alloc and dev_kzalloc also occurs. > >> > >> Perhaps it'd be better to do: > >> > >>if (ps->netdev) { > >>devm_kfree(>dev, ps); > >>devm_kfree(>dev, ds); > >>return -EPROBE_DEFER; > >>} > > > > Is EPROBE_DEFER handled differently than other kinds of errors? > > In the core device driver model, yes, EPROBE_DEFER is treated > differently than other errors because it puts the driver on a retry queue. > > EPROBE_DEFER is already a slow and exceptional path, and this is a > mock-up driver, so I am not sure what value there is in trying to > balance devm_kzalloc() with corresponding devm_kfree()... OK, thanks for the explanation. julia
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
On 05/08/2017 04:46 PM, Julia Lawall wrote: > > > On Mon, 8 May 2017, Joe Perches wrote: > >> On Mon, 2017-05-08 at 20:32 +0800, Julia Lawall wrote: >>> >>> On Mon, 8 May 2017, David Laight wrote: >>> From: Christophe JAILLET > Sent: 06 May 2017 06:30 > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for some other memory allocation just a > few lines above. ... > --- a/drivers/net/dsa/dsa_loop.c > +++ b/drivers/net/dsa/dsa_loop.c > @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device > *mdiodev) > return -ENOMEM; > > ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); > + if (!ps) > + return -ENOMEM; > + > ps->netdev = dev_get_by_name(_net, pdata->netdev); > if (!ps->netdev) > return -EPROBE_DEFER; On the face if it this code leaks like a sieve. >>> >>> I don't think so. The allocations (dsa_switch_alloc and devm_kzalloc) use >>> devm functions. >> >> It's at least wasteful. >> >> Each time -EPROBE_DEFER occurs, another set of calls to >> dsa_switch_alloc and dev_kzalloc also occurs. >> >> Perhaps it'd be better to do: >> >> if (ps->netdev) { >> devm_kfree(>dev, ps); >> devm_kfree(>dev, ds); >> return -EPROBE_DEFER; >> } > > Is EPROBE_DEFER handled differently than other kinds of errors? In the core device driver model, yes, EPROBE_DEFER is treated differently than other errors because it puts the driver on a retry queue. EPROBE_DEFER is already a slow and exceptional path, and this is a mock-up driver, so I am not sure what value there is in trying to balance devm_kzalloc() with corresponding devm_kfree()... -- Florian
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
On Mon, 8 May 2017, Joe Perches wrote: > On Mon, 2017-05-08 at 20:32 +0800, Julia Lawall wrote: > > > > On Mon, 8 May 2017, David Laight wrote: > > > > > From: Christophe JAILLET > > > > Sent: 06 May 2017 06:30 > > > > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > > > > Return -ENOMEM instead, as done for some other memory allocation just a > > > > few lines above. > > > > > > ... > > > > --- a/drivers/net/dsa/dsa_loop.c > > > > +++ b/drivers/net/dsa/dsa_loop.c > > > > @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device > > > > *mdiodev) > > > > return -ENOMEM; > > > > > > > > ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); > > > > + if (!ps) > > > > + return -ENOMEM; > > > > + > > > > ps->netdev = dev_get_by_name(_net, pdata->netdev); > > > > if (!ps->netdev) > > > > return -EPROBE_DEFER; > > > > > > On the face if it this code leaks like a sieve. > > > > I don't think so. The allocations (dsa_switch_alloc and devm_kzalloc) use > > devm functions. > > It's at least wasteful. > > Each time -EPROBE_DEFER occurs, another set of calls to > dsa_switch_alloc and dev_kzalloc also occurs. > > Perhaps it'd be better to do: > > if (ps->netdev) { > devm_kfree(>dev, ps); > devm_kfree(>dev, ds); > return -EPROBE_DEFER; > } Is EPROBE_DEFER handled differently than other kinds of errors? julia > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
On Mon, 2017-05-08 at 20:32 +0800, Julia Lawall wrote: > > On Mon, 8 May 2017, David Laight wrote: > > > From: Christophe JAILLET > > > Sent: 06 May 2017 06:30 > > > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > > > Return -ENOMEM instead, as done for some other memory allocation just a > > > few lines above. > > > > ... > > > --- a/drivers/net/dsa/dsa_loop.c > > > +++ b/drivers/net/dsa/dsa_loop.c > > > @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device > > > *mdiodev) > > > return -ENOMEM; > > > > > > ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); > > > + if (!ps) > > > + return -ENOMEM; > > > + > > > ps->netdev = dev_get_by_name(_net, pdata->netdev); > > > if (!ps->netdev) > > > return -EPROBE_DEFER; > > > > On the face if it this code leaks like a sieve. > > I don't think so. The allocations (dsa_switch_alloc and devm_kzalloc) use > devm functions. It's at least wasteful. Each time -EPROBE_DEFER occurs, another set of calls to dsa_switch_alloc and dev_kzalloc also occurs. Perhaps it'd be better to do: if (ps->netdev) { devm_kfree(>dev, ps); devm_kfree(>dev, ds); return -EPROBE_DEFER; }
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
From: Christophe JAILLETDate: Sat, 6 May 2017 07:29:45 +0200 > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for some other memory allocation just a > few lines above. > > Fixes: 98cd1552ea27 ("net: dsa: Mock-up driver") > > Signed-off-by: Christophe JAILLET Please do not separate "Fixes: " tags from signoffs and acks with empty lines in the future, thank you. Applied and queued up for -stable.
RE: [PATCH] net: dsa: loop: Check for memory allocation failure
On Mon, 8 May 2017, David Laight wrote: > From: Christophe JAILLET > > Sent: 06 May 2017 06:30 > > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > > Return -ENOMEM instead, as done for some other memory allocation just a > > few lines above. > ... > > --- a/drivers/net/dsa/dsa_loop.c > > +++ b/drivers/net/dsa/dsa_loop.c > > @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device > > *mdiodev) > > return -ENOMEM; > > > > ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); > > + if (!ps) > > + return -ENOMEM; > > + > > ps->netdev = dev_get_by_name(_net, pdata->netdev); > > if (!ps->netdev) > > return -EPROBE_DEFER; > > On the face if it this code leaks like a sieve. I don't think so. The allocations (dsa_switch_alloc and devm_kzalloc) use devm functions. julia
RE: [PATCH] net: dsa: loop: Check for memory allocation failure
From: Christophe JAILLET > Sent: 06 May 2017 06:30 > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for some other memory allocation just a > few lines above. ... > --- a/drivers/net/dsa/dsa_loop.c > +++ b/drivers/net/dsa/dsa_loop.c > @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev) > return -ENOMEM; > > ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); > + if (!ps) > + return -ENOMEM; > + > ps->netdev = dev_get_by_name(_net, pdata->netdev); > if (!ps->netdev) > return -EPROBE_DEFER; On the face if it this code leaks like a sieve. David
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
Le 05/05/17 à 22:29, Christophe JAILLET a écrit : > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for some other memory allocation just a > few lines above. > > Fixes: 98cd1552ea27 ("net: dsa: Mock-up driver") > > Signed-off-by: Christophe JAILLETAcked-by: Florian Fainelli -- Florian
Re: [PATCH] net: dsa: loop: Check for memory allocation failure
On Sat, May 06, 2017 at 07:29:45AM +0200, Christophe JAILLET wrote: > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for some other memory allocation just a > few lines above. > > Fixes: 98cd1552ea27 ("net: dsa: Mock-up driver") > > Signed-off-by: Christophe JAILLETReviewed-by: Andrew Lunn Andrew
[PATCH] net: dsa: loop: Check for memory allocation failure
If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. Return -ENOMEM instead, as done for some other memory allocation just a few lines above. Fixes: 98cd1552ea27 ("net: dsa: Mock-up driver") Signed-off-by: Christophe JAILLET--- drivers/net/dsa/dsa_loop.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c index f0fc4de4fc9a..a19e1781e9bb 100644 --- a/drivers/net/dsa/dsa_loop.c +++ b/drivers/net/dsa/dsa_loop.c @@ -256,6 +256,9 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev) return -ENOMEM; ps = devm_kzalloc(>dev, sizeof(*ps), GFP_KERNEL); + if (!ps) + return -ENOMEM; + ps->netdev = dev_get_by_name(_net, pdata->netdev); if (!ps->netdev) return -EPROBE_DEFER; -- 2.11.0