linux-kernel - Re: [PATCH net-next] ethtool: ioctl: improve error checking for set

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: <ZHmuQAeP82TuBssK@corigine.com>
Date:   Fri, 2 Jun 2023 10:54:24 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Justin Chen <justin.chen@...adcom.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, bcm-kernel-feedback-list@...adcom.com,
        florian.fainelli@...adcom.com,
        Daniil Tatianin <d-tatianin@...dex-team.ru>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next] ethtool: ioctl: improve error checking for
 set_wol

On Thu, Jun 01, 2023 at 09:22:50AM -0700, Justin Chen wrote:
> 
> 
> On 6/1/2023 8:55 AM, Simon Horman wrote:
> > + Daniil Tatianin <d-tatianin@...dex-team.ru>, Andrew Lunn <andrew@...n.ch>
> >    as per ./scripts/get_maintainer.pl --git-min-percent 25 net/ethtool/ioctl.c
> > 
> > On Wed, May 31, 2023 at 01:53:49PM -0700, 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.
> > 
> > Hi Justin,
> > 
> > Are you planning follow-up patches for the driver layer?
> > 
> 
> I was planning to for the Broadcom drivers since those I can test. But I
> could do it across the board if that is preferred.

I think that would be my suggestion.
But I'm unsure of the magnitude of change involved.
Or how risky it is in terms of introducing regressions.
In any case, perhaps it's best to start small.

> > > Signed-off-by: Justin Chen <justin.chen@...adcom.com>
> > > ---
> > >   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..80f456f83db0 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;
> > 
> > Are there cases where (in-tree) drivers provide set_wol byt not get_wol?
> > If so, does this break their set_wol support?
> > 
> 
> My original thought was to match netlink set wol behavior. So drivers that
> do that won't work with netlink set_wol right now. I'll skim around to see
> if any drivers do this. But I would reckon this should be a driver fix.

It seems, from other discussion in a different sub-thread, that we are
likely clear there :)

As that was my only real reservation wrt this patch:

Reviewed-by: Simon Horman <simon.horman@...igine.com>