> -----邮件原件----- > 发件人: Michal Kubecek [mailto:mkube...@suse.cz] > 发送时间: 2019年3月29日 17:29 > 收件人: Li,Rongqing <lirongq...@baidu.com> > 抄送: netdev@vger.kernel.org > 主题: Re: [PATCH][v2] net: ethtool: not call vzalloc for zero sized memory > request > > On Fri, Mar 29, 2019 at 09:18:02AM +0800, Li RongQing wrote: > > NULL or ZERO_SIZE_PTR will be returned for zero sized memory request, > > and derefencing them will lead to a segfault > > > > so it is unnecessory to call vzalloc for zero sized memory request and > > not call functions which maybe derefence the NULL allocated memory > > > > this also fixes a possible memory leak if phy_ethtool_get_stats > > returns error, memory should be freed before exit > > > > Signed-off-by: Li RongQing <lirongq...@baidu.com> > > Reviewed-by: Wang Li <wangl...@baidu.com> > > --- > > v1->v2: not call get_ethtool_stats if n_stats is 0 > > > > net/core/ethtool.c | 46 > > ++++++++++++++++++++++++++++++---------------- > > 1 file changed, 30 insertions(+), 16 deletions(-) > > This looks correct, I have just one minor comment below. > > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index > > b1eb32419732..36ed619faf36 100644 > > --- a/net/core/ethtool.c > > +++ b/net/core/ethtool.c > > @@ -1797,11 +1797,16 @@ static int ethtool_get_strings(struct net_device > *dev, void __user *useraddr) > > WARN_ON_ONCE(!ret); > > > > gstrings.len = ret; > > - data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN)); > > - if (gstrings.len && !data) > > - return -ENOMEM; > > > > - __ethtool_get_strings(dev, gstrings.string_set, data); > > + if (gstrings.len) { > > + data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN)); > > + if (!data) > > + return -ENOMEM; > > + > > + __ethtool_get_strings(dev, gstrings.string_set, data); > > + } else { > > + data = NULL; > > + } > > > > ret = -EFAULT; > > if (copy_to_user(useraddr, &gstrings, sizeof(gstrings))) > > If gstrings.len is zero, there is no need to set data pointer as it's not > going to be > used so that you could omit the else branch. It's the same in the other two > functions. > I think it is must, since data is from stack, and not init, once it is passed into vfree, will cause panic
thanks -RongQing > Michal
