On Thu, Mar 28, 2019 at 02:01:09PM +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 __ethtool_get_strings which always uses
> the 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>
> ---
> net/core/ethtool.c | 37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index b1eb32419732..3e971a36e37c 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
...
> @@ -1897,9 +1902,14 @@ static int ethtool_get_stats(struct net_device *dev,
> void __user *useraddr)
> return -EFAULT;
>
> stats.n_stats = n_stats;
> - data = vzalloc(array_size(n_stats, sizeof(u64)));
> - if (n_stats && !data)
> - return -ENOMEM;
> +
> + if (n_stats) {
> + data = vzalloc(array_size(n_stats, sizeof(u64)));
> + if (!data)
> + return -ENOMEM;
> + } else {
> + data = NULL;
> + }
>
> ops->get_ethtool_stats(dev, &stats, data);
>
You avoid the vzalloc() call here but you still pass null data pointer
to device's get_ethtool_stats() handler which seems to contradict what
the commit message says. Is it really what you want? (The same applies
to ethtool_get_phy_stats() below.)
Michal
> @@ -1941,14 +1951,19 @@ static int ethtool_get_phy_stats(struct net_device
> *dev, void __user *useraddr)
> return -EFAULT;
>
> stats.n_stats = n_stats;
> - data = vzalloc(array_size(n_stats, sizeof(u64)));
> - if (n_stats && !data)
> - return -ENOMEM;
> +
> + if (n_stats) {
> + data = vzalloc(array_size(n_stats, sizeof(u64)));
> + if (!data)
> + return -ENOMEM;
> + } else {
> + data = NULL;
> + }
>
> if (dev->phydev && !ops->get_ethtool_phy_stats) {
> ret = phy_ethtool_get_stats(dev->phydev, &stats, data);
> if (ret < 0)
> - return ret;
> + goto out;
> } else {
> ops->get_ethtool_phy_stats(dev, &stats, data);
> }
> --
> 2.16.2
>
