diff mbox series

[net-next] ethtool: ioctl: account for sopass diff in set_wol

Message ID 1686605822-34544-1-git-send-email-justin.chen@broadcom.com
State New
Headers
Series [net-next] ethtool: ioctl: account for sopass diff in set_wol |

Commit Message

Justin Chen June 12, 2023, 9:37 p.m. UTC 
  sopass won't be set if wolopt doesn't change. This means the following
will fail to set the correct sopass.
ethtool -s eth0 wol s sopass 11:22:33:44:55:66
ethtool -s eth0 wol s sopass 22:44:55:66:77:88

Make sure we call into the driver layer set_wol if sopass is different.

Fixes: 55b24334c0f2 ("ethtool: ioctl: improve error checking for set_wol")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---

Note: Tagged "Fixes" patch has not hit rc yet.

 net/ethtool/ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Andrew Lunn June 12, 2023, 10:28 p.m. UTC | #1 
On Mon, Jun 12, 2023 at 02:37:00PM -0700, Justin Chen wrote:
> sopass won't be set if wolopt doesn't change. This means the following
> will fail to set the correct sopass.
> ethtool -s eth0 wol s sopass 11:22:33:44:55:66
> ethtool -s eth0 wol s sopass 22:44:55:66:77:88
> 
> Make sure we call into the driver layer set_wol if sopass is different.
> 
> Fixes: 55b24334c0f2 ("ethtool: ioctl: improve error checking for set_wol")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> ---
> 
> Note: Tagged "Fixes" patch has not hit rc yet.
> 
>  net/ethtool/ioctl.c | 3 ++-

Hi Justin

Does the netlink version get this correct?

And just for my own curiosity, why are you so interested in the ioctl
version, which is deprecated and not used by modern versions of
ethtool?

	Andrew
Justin Chen June 12, 2023, 11 p.m. UTC | #2 
On 6/12/23 3:28 PM, Andrew Lunn wrote:
> On Mon, Jun 12, 2023 at 02:37:00PM -0700, Justin Chen wrote:
>> sopass won't be set if wolopt doesn't change. This means the following
>> will fail to set the correct sopass.
>> ethtool -s eth0 wol s sopass 11:22:33:44:55:66
>> ethtool -s eth0 wol s sopass 22:44:55:66:77:88
>>
>> Make sure we call into the driver layer set_wol if sopass is different.
>>
>> Fixes: 55b24334c0f2 ("ethtool: ioctl: improve error checking for set_wol")
>> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
>> ---
>>
>> Note: Tagged "Fixes" patch has not hit rc yet.
>>
>>   net/ethtool/ioctl.c | 3 ++-
> 
> Hi Justin
> 
> Does the netlink version get this correct?
>

Yup netlink gets this correct

> And just for my own curiosity, why are you so interested in the ioctl
> version, which is deprecated and not used by modern versions of
> ethtool?
>
No particular reason. I was working on another driver and was planning 
to remove the set_wol error checking in the driver layer when I realized 
netlink set_wol was already doing it. Then on further inspection I 
noticed the ioctl version doesn't do the same thing.

Justin

> 	Andrew
patchwork-bot+netdevbpf@kernel.org June 14, 2023, 5:10 a.m. UTC | #3 
Hello:

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

On Mon, 12 Jun 2023 14:37:00 -0700 you wrote:
> sopass won't be set if wolopt doesn't change. This means the following
> will fail to set the correct sopass.
> ethtool -s eth0 wol s sopass 11:22:33:44:55:66
> ethtool -s eth0 wol s sopass 22:44:55:66:77:88
> 
> Make sure we call into the driver layer set_wol if sopass is different.
> 
> [...]

Here is the summary with links:
  - [net-next] ethtool: ioctl: account for sopass diff in set_wol
    https://git.kernel.org/netdev/net-next/c/2bddad9ec65a

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 37b582225854..4a51e0ec295c 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1452,7 +1452,8 @@  static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 	if (wol.wolopts & ~cur_wol.supported)
 		return -EINVAL;
 
-	if (wol.wolopts == cur_wol.wolopts)
+	if (wol.wolopts == cur_wol.wolopts &&
+	    !memcmp(wol.sopass, cur_wol.sopass, sizeof(wol.sopass)))
 		return 0;
 
 	ret = dev->ethtool_ops->set_wol(dev, &wol);