From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753579AbdEIAfU (ORCPT ); Mon, 8 May 2017 20:35:20 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:36761 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751087AbdEIAfR (ORCPT ); Mon, 8 May 2017 20:35:17 -0400 Subject: Re: [PATCH] net: dsa: loop: Check for memory allocation failure To: Julia Lawall , Joe Perches References: <20170506052945.2639-1-christophe.jaillet@wanadoo.fr> <063D6719AE5E284EB5DD2968C1650D6DCFFE715E@AcuExch.aculab.com> <1494265443.31950.62.camel@perches.com> Cc: David Laight , "'Christophe JAILLET'" , "andrew@lunn.ch" , "vivien.didelot@savoirfairelinux.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kernel-janitors@vger.kernel.org" From: Florian Fainelli Message-ID: Date: Mon, 8 May 2017 17:35:09 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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()... -- Florian From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH] net: dsa: loop: Check for memory allocation failure Date: Mon, 8 May 2017 17:35:09 -0700 Message-ID: References: <20170506052945.2639-1-christophe.jaillet@wanadoo.fr> <063D6719AE5E284EB5DD2968C1650D6DCFFE715E@AcuExch.aculab.com> <1494265443.31950.62.camel@perches.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: David Laight , 'Christophe JAILLET' , "andrew@lunn.ch" , "vivien.didelot@savoirfairelinux.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kernel-janitors@vger.kernel.org" To: Julia Lawall , Joe Perches Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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()... -- Florian From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Date: Tue, 09 May 2017 00:35:09 +0000 Subject: Re: [PATCH] net: dsa: loop: Check for memory allocation failure Message-Id: List-Id: References: <20170506052945.2639-1-christophe.jaillet@wanadoo.fr> <063D6719AE5E284EB5DD2968C1650D6DCFFE715E@AcuExch.aculab.com> <1494265443.31950.62.camel@perches.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Julia Lawall , Joe Perches Cc: David Laight , 'Christophe JAILLET' , "andrew@lunn.ch" , "vivien.didelot@savoirfairelinux.com" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kernel-janitors@vger.kernel.org" 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()... -- Florian