[v2] ethtool: ioctl: improve error checking for set_wol

[PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol

Posted by Justin Chen 2 years, 8 months ago

The netlink version of set_wol checks for not supported wolopts and avoids
setting wol when the correct wolopt is already set. If we do the same with
the ioctl version then we can remove these checks from the driver layer.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
v2
	- Return -EINVAL instead of -EOPNOTSUPP

 net/ethtool/ioctl.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 6bb778e10461..37b582225854 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1436,15 +1436,25 @@ static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
 
 static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 {
-	struct ethtool_wolinfo wol;
+	struct ethtool_wolinfo wol, cur_wol;
 	int ret;
 
-	if (!dev->ethtool_ops->set_wol)
+	if (!dev->ethtool_ops->get_wol || !dev->ethtool_ops->set_wol)
 		return -EOPNOTSUPP;
 
+	memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
+	cur_wol.cmd = ETHTOOL_GWOL;
+	dev->ethtool_ops->get_wol(dev, &cur_wol);
+
 	if (copy_from_user(&wol, useraddr, sizeof(wol)))
 		return -EFAULT;
 
+	if (wol.wolopts & ~cur_wol.supported)
+		return -EINVAL;
+
+	if (wol.wolopts == cur_wol.wolopts)
+		return 0;
+
 	ret = dev->ethtool_ops->set_wol(dev, &wol);
 	if (ret)
 		return ret;
-- 
2.7.4

Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol

Posted by Justin Chen 2 years, 8 months ago


On 6/7/23 4:14 PM, Justin Chen wrote:
> The netlink version of set_wol checks for not supported wolopts and avoids
> setting wol when the correct wolopt is already set. If we do the same with
> the ioctl version then we can remove these checks from the driver layer.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> ---
> v2
> 	- Return -EINVAL instead of -EOPNOTSUPP
> 
>   net/ethtool/ioctl.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 6bb778e10461..37b582225854 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1436,15 +1436,25 @@ static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
>   
>   static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
>   {
> -	struct ethtool_wolinfo wol;
> +	struct ethtool_wolinfo wol, cur_wol;
>   	int ret;
>   
> -	if (!dev->ethtool_ops->set_wol)
> +	if (!dev->ethtool_ops->get_wol || !dev->ethtool_ops->set_wol)
>   		return -EOPNOTSUPP;
>   
> +	memset(&cur_wol, 0, sizeof(struct ethtool_wolinfo));
> +	cur_wol.cmd = ETHTOOL_GWOL;
> +	dev->ethtool_ops->get_wol(dev, &cur_wol);
> +
>   	if (copy_from_user(&wol, useraddr, sizeof(wol)))
>   		return -EFAULT;
>   
> +	if (wol.wolopts & ~cur_wol.supported)
> +		return -EINVAL;
> +
> +	if (wol.wolopts == cur_wol.wolopts)
> +		return 0;
> +
>   	ret = dev->ethtool_ops->set_wol(dev, &wol);
>   	if (ret)
>   		return ret;

Was thinking more about this patch. I realized we don't account for the 
different sopass case.
# ethtool -s eth0 wol s sopass 11:22:33:44:55:66
# ethtool -s eth0 wol s sopass 22:44:55:66:77:88

For this case, the second sopass values won't be stored.

Can you drop this patch? I will submit another version.

Thanks,
Justin

Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol

Posted by Jakub Kicinski 2 years, 8 months ago

On Fri, 9 Jun 2023 13:47:22 -0700 Justin Chen wrote:
> Was thinking more about this patch. I realized we don't account for the 
> different sopass case.
> # ethtool -s eth0 wol s sopass 11:22:33:44:55:66
> # ethtool -s eth0 wol s sopass 22:44:55:66:77:88
> 
> For this case, the second sopass values won't be stored.
>
> Can you drop this patch? I will submit another version.

We can't drop patches, it'd mess up commit IDs and basing
trees on top of net-next would be a major PITA for people.
Please send a fix on top (with a Fixes tag making it clear
that the problem has not reached any -rc kernel).

Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol

Posted by patchwork-bot+netdevbpf@kernel.org 2 years, 8 months ago

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  7 Jun 2023 16:14:11 -0700 you wrote:
> The netlink version of set_wol checks for not supported wolopts and avoids
> setting wol when the correct wolopt is already set. If we do the same with
> the ioctl version then we can remove these checks from the driver layer.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> 
> [...]

Here is the summary with links:
  - [v2,net-next] ethtool: ioctl: improve error checking for set_wol
    https://git.kernel.org/netdev/net-next/c/55b24334c0f2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 net-next] ethtool: ioctl: improve error checking for set_wol

Posted by Florian Fainelli 2 years, 8 months ago


On 6/7/2023 4:14 PM, Justin Chen wrote:
> The netlink version of set_wol checks for not supported wolopts and avoids
> setting wol when the correct wolopt is already set. If we do the same with
> the ioctl version then we can remove these checks from the driver layer.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian