Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <a366c691-fb81-8e30-3853-3260ceabf080@linux.ibm.com>
Date:   Tue, 14 Sep 2021 20:31:29 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Subject: Re: [PATCH V3 net-next 2/4] ethtool: extend coalesce setting uAPI
 with CQE mode
Content-Language: en-US
To:     Yufeng Mo <moyufeng@huawei.com>, davem@davemloft.net,
        kuba@kernel.org
Cc:     netdev@vger.kernel.org, shenjian15@huawei.com,
        lipeng321@huawei.com, yisen.zhuang@huawei.com,
        linyunsheng@huawei.com, huangguangbin2@huawei.com,
        chenhao288@hisilicon.com, salil.mehta@huawei.com,
        linuxarm@huawei.com, linuxarm@openeuler.org, dledford@redhat.com,
        jgg@ziepe.ca, netanel@amazon.com, akiyano@amazon.com,
        thomas.lendacky@amd.com, irusskikh@marvell.com,
        michael.chan@broadcom.com, edwin.peer@broadcom.com,
        rohitm@chelsio.com, jacob.e.keller@intel.com,
        ioana.ciornei@nxp.com, vladimir.oltean@nxp.com,
        sgoutham@marvell.com, sbhatta@marvell.com, saeedm@nvidia.com,
        ecree.xilinx@gmail.com, grygorii.strashko@ti.com,
        merez@codeaurora.org, kvalo@codeaurora.org,
        linux-wireless@vger.kernel.org
References: <1629444920-25437-1-git-send-email-moyufeng@huawei.com>
 <1629444920-25437-3-git-send-email-moyufeng@huawei.com>
From:   Julian Wiedmann <jwi@linux.ibm.com>
In-Reply-To: <1629444920-25437-3-git-send-email-moyufeng@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 20.08.21 10:35, Yufeng Mo wrote:
> In order to support more coalesce parameters through netlink,
> add two new parameter kernel_coal and extack for .set_coalesce
> and .get_coalesce, then some extra info can return to user with
> the netlink API.
> 
> Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---

[...]

> index 81fa36a..f2abc31 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1619,12 +1619,14 @@ static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
>  						   void __user *useraddr)
>  {
>  	struct ethtool_coalesce coalesce = { .cmd = ETHTOOL_GCOALESCE };
> +	struct kernel_ethtool_coalesce kernel_coalesce = {};
>  	int ret;
>  
>  	if (!dev->ethtool_ops->get_coalesce)
>  		return -EOPNOTSUPP;
>  
> -	ret = dev->ethtool_ops->get_coalesce(dev, &coalesce);
> +	ret = dev->ethtool_ops->get_coalesce(dev, &coalesce, &kernel_coalesce,
> +					     NULL);
>  	if (ret)
>  		return ret;
>  
> @@ -1691,19 +1693,26 @@ ethtool_set_coalesce_supported(struct net_device *dev,
>  static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
>  						   void __user *useraddr)
>  {
> +	struct kernel_ethtool_coalesce kernel_coalesce = {};
>  	struct ethtool_coalesce coalesce;
>  	int ret;
>  
> -	if (!dev->ethtool_ops->set_coalesce)
> +	if (!dev->ethtool_ops->set_coalesce && !dev->ethtool_ops->get_coalesce)
>  		return -EOPNOTSUPP;
>  

This needs to be

	if (!set_coalesce || !get_coalesce)
		return -EOPNOTSUPP;

Otherwise you end up calling a NULL pointer below if just _one_ of the
callbacks is available.


> +	ret = dev->ethtool_ops->get_coalesce(dev, &coalesce, &kernel_coalesce,
> +					     NULL);
> +	if (ret)
> +		return ret;
> +
>  	if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
>  		return -EFAULT;
>  
>  	if (!ethtool_set_coalesce_supported(dev, &coalesce))
>  		return -EOPNOTSUPP;
>  
> -	ret = dev->ethtool_ops->set_coalesce(dev, &coalesce);
> +	ret = dev->ethtool_ops->set_coalesce(dev, &coalesce, &kernel_coalesce,
> +					     NULL);
>  	if (!ret)
>  		ethtool_notify(dev, ETHTOOL_MSG_COALESCE_NTF, NULL);
>  	return ret;
>