| Message ID | 20170506052945.2639-1-christophe.jaillet@wanadoo.fr |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 JAILLET <christophe.jaillet@wanadoo.fr> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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 JAILLET <christophe.jaillet@wanadoo.fr> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); > + if (!ps) > + return -ENOMEM; > + > ps->netdev = dev_get_by_name(&init_net, pdata->netdev); > if (!ps->netdev) > return -EPROBE_DEFER; On the face if it this code leaks like a sieve. David
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); > > + if (!ps) > > + return -ENOMEM; > > + > > ps->netdev = dev_get_by_name(&init_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
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); > > > + if (!ps) > > > + return -ENOMEM; > > > + > > > ps->netdev = dev_get_by_name(&init_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(&devmdev->dev, ps); devm_kfree(&mdiodev->dev, ds); return -EPROBE_DEFER; }
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Date: 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 <christophe.jaillet@wanadoo.fr> Please do not separate "Fixes: " tags from signoffs and acks with empty lines in the future, thank you. Applied and queued up for -stable.
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); > > > > + if (!ps) > > > > + return -ENOMEM; > > > > + > > > > ps->netdev = dev_get_by_name(&init_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(&devmdev->dev, ps); > devm_kfree(&mdiodev->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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); >>>>> + if (!ps) >>>>> + return -ENOMEM; >>>>> + >>>>> ps->netdev = dev_get_by_name(&init_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(&devmdev->dev, ps); >> devm_kfree(&mdiodev->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()...
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); > >>>>> + if (!ps) > >>>>> + return -ENOMEM; > >>>>> + > >>>>> ps->netdev = dev_get_by_name(&init_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(&devmdev->dev, ps); > >> devm_kfree(&mdiodev->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
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(&devmdev->dev, ps); > > > devm_kfree(&mdiodev->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.
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(&mdiodev->dev, sizeof(*ps), GFP_KERNEL); + if (!ps) + return -ENOMEM; + ps->netdev = dev_get_by_name(&init_net, pdata->netdev); if (!ps->netdev) return -EPROBE_DEFER;
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 <christophe.jaillet@wanadoo.fr> --- drivers/net/dsa/dsa_loop.c | 3 +++ 1 file changed, 3 insertions(+)