diff mbox series

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

Message ID 1686179653-29750-1-git-send-email-justin.chen@broadcom.com (mailing list archive)
State Accepted
Commit 55b24334c0f2db55ca059848121939f59133bdb6
Delegated to: Netdev Maintainers
Headers show
Series [v2,net-next] ethtool: ioctl: improve error checking for set_wol | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 10 this patch: 10
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 10 this patch: 10
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 27 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Justin Chen June 7, 2023, 11:14 p.m. UTC 
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(-)

Comments

Florian Fainelli June 8, 2023, 4:01 p.m. UTC | #1 
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>
patchwork-bot+netdevbpf@kernel.org June 9, 2023, 2:40 a.m. UTC | #2 
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!
Justin Chen June 9, 2023, 8:47 p.m. UTC | #3 
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
Jakub Kicinski June 10, 2023, 6:18 a.m. UTC | #4 
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).
diff mbox series

Patch

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;