From mboxrd@z Thu Jan 1 00:00:00 1970 From: "John W. Linville" Subject: Re: [PATCH v2] ethtool: do not vzalloc(0) on registers dump Date: Thu, 2 Feb 2017 09:27:18 -0500 Message-ID: <20170202142717.GA22745@tuxdriver.com> References: <1486038730-9469-1-git-send-email-sgruszka@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, Ben Hutchings To: Stanislaw Gruszka Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:53439 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751734AbdBBOaa (ORCPT ); Thu, 2 Feb 2017 09:30:30 -0500 Content-Disposition: inline In-Reply-To: <1486038730-9469-1-git-send-email-sgruszka@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Feb 02, 2017 at 01:32:10PM +0100, Stanislaw Gruszka wrote: > If ->get_regs_len() callback return 0, we allocate 0 bytes of memory, > what print ugly warning in dmesg, which can be found further below. > > This happen on mac80211 devices where ieee80211_get_regs_len() just > return 0 and driver only fills ethtool_regs structure and actually > do not provide any dump. However I assume this can happen on other > drivers i.e. when for some devices driver provide regs dump and for > others do not. Hence preventing to to print warning in ethtool code > seems to be reasonable. > > ethtool: vmalloc: allocation failure: 0 bytes, mode:0x24080c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_ZERO) > > Call Trace: > [] dump_stack+0x63/0x8c > [] warn_alloc+0x13f/0x170 > [] __vmalloc_node_range+0x1e6/0x2c0 > [] vzalloc+0x54/0x60 > [] dev_ethtool+0xb4c/0x1b30 > [] dev_ioctl+0x181/0x520 > [] sock_do_ioctl+0x42/0x50 > > Mem-Info: > active_anon:435809 inactive_anon:173951 isolated_anon:0 > active_file:835822 inactive_file:196932 isolated_file:0 > unevictable:0 dirty:8 writeback:0 unstable:0 > slab_reclaimable:157732 slab_unreclaimable:10022 > mapped:83042 shmem:306356 pagetables:9507 bounce:0 > free:130041 free_pcp:1080 free_cma:0 > Node 0 active_anon:1743236kB inactive_anon:695804kB active_file:3343288kB inactive_file:787728kB unevictable:0kB isolated(anon):0kB isolated(file):0kB mapped:332168kB dirty:32kB writeback:0kB shmem:0kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 1225424kB writeback_tmp:0kB unstable:0kB pages_scanned:0 all_unreclaimable? no > Node 0 DMA free:15900kB min:136kB low:168kB high:200kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB writepending:0kB present:15984kB managed:15900kB mlocked:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB > lowmem_reserve[]: 0 3187 7643 7643 > Node 0 DMA32 free:419732kB min:28124kB low:35152kB high:42180kB active_anon:541180kB inactive_anon:248988kB active_file:1466388kB inactive_file:389632kB unevictable:0kB writepending:0kB present:3370280kB managed:3290932kB mlocked:0kB slab_reclaimable:217184kB slab_unreclaimable:4180kB kernel_stack:160kB pagetables:984kB bounce:0kB free_pcp:2236kB local_pcp:660kB free_cma:0kB > lowmem_reserve[]: 0 0 4456 4456 > > Signed-off-by: Stanislaw Gruszka > --- > v1 -> v2: nullify regbuf to avoid using uninitialized variable in line: > if (regbuf && copy_to_user(useraddr, regbuf, regs.len)) > > net/core/ethtool.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 6b3eee0..4b6dc9b 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1405,9 +1405,12 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr) > if (regs.len > reglen) > regs.len = reglen; > > - regbuf = vzalloc(reglen); > - if (reglen && !regbuf) > - return -ENOMEM; > + regbuf = NULL; Any reason to prefer this over changing the declaration to include the assignment? void *regbuf = NULL; > + if (reglen) { > + regbuf = vzalloc(reglen); > + if (!regbuf) > + return -ENOMEM; > + } > > ops->get_regs(dev, ®s, regbuf); > > -- > 1.8.3.1 > > -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready.