linux-kernel - Re: [PATCH net v1 1/1] ethtool: fix clearing of WoL flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231019122114.5b4a13a9@kmaincent-XPS-13-7390>
Date:   Thu, 19 Oct 2023 12:21:14 +0200
From:   Köry Maincent <kory.maincent@...tlin.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     Oleksij Rempel <o.rempel@...gutronix.de>,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Vladimir Oltean <olteanv@...il.com>, kernel@...gutronix.de,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net v1 1/1] ethtool: fix clearing of WoL flags

On Thu, 19 Oct 2023 11:51:40 +0200
Michal Kubecek <mkubecek@...e.cz> wrote:

> On Thu, Oct 19, 2023 at 11:05:10AM +0200, Michal Kubecek wrote:
> > On Thu, Oct 19, 2023 at 09:09:04AM +0200, Oleksij Rempel wrote:  
> > > With current kernel it is possible to set flags, but not possible to
> > > remove existing WoL flags. For example:
> > > ~$ ethtool lan2
> > > ...
> > >         Supports Wake-on: pg
> > >         Wake-on: d
> > > ...
> > > ~$ ethtool -s lan2 wol gp
> > > ~$ ethtool lan2
> > > ...
> > >         Wake-on: pg
> > > ...
> > > ~$ ethtool -s lan2 wol d
> > > ~$ ethtool lan2
> > > ...
> > >         Wake-on: pg
> > > ...
> > >   
> > 
> > How recent was the kernel where you encountered the issue? I suspect the
> > issue might be related to recent 108a36d07c01 ("ethtool: Fix mod state
> > of verbose no_mask bitset"), I'll look into it closer.  
> 
> The issue was indeed introduced by commit 108a36d07c01 ("ethtool: Fix
> mod state of verbose no_mask bitset"). The problem is that a "no mask"
> verbose bitset only contains bit attributes for bits to be set. This
> worked correctly before this commit because we were always updating
> a zero bitmap (since commit 6699170376ab ("ethtool: fix application of
> verbose no_mask bitset"), that is) so that the rest was left zero
> naturally. But now the 1->0 change (old_val is true, bit not present in
> netlink nest) no longer works.

Doh I had not seen this issue! Thanks you for reporting it.
I will send the revert then and will update the fix for next merge-window.

> To fix the issue while keeping more precise modification tracking
> introduced by commit 108a36d07c01 ("ethtool: Fix mod state of verbose
> no_mask bitset"), we would have to either iterate through all possible
> bits in "no mask" case or update a temporary zero bitmap and then set
> mod by comparing it to the original (and rewrite the target if they
> differ). This is exactly what I was trying to avoid from the start but
> it wouldn't be more complicated than current code.
> 
> As we are rather late in the 6.6 cycle (rc6 out), the safest solution
> seems to be reverting commit 108a36d07c01 ("ethtool: Fix mod state of
> verbose no_mask bitset") in net tree as sending a notification even on
> a request which turns out to be no-op is not a serious problem (after
> all, this is what happens for ioctl requests most of the time and IIRC
> there are more cases where it happens for netlink requests). Then we can
> fix the change detection properly in net-next in the way proposed in
> previous paragraph (I would prefer not risking more intrusive changes at
> this stage).