netdev - Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F

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]

Date:	Fri, 27 May 2011 18:34:09 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT
 return

On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
> On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> > On Fri, May 27, 2011 at 03:13:46PM +0100, Ben Hutchings wrote:
> > > On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:
> > > > On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> > > > > From: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> > > > > Date: Tue, 24 May 2011 11:14:37 +0200
> > > > > > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> > > > > >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > > > > >> > You guys really need to sort this out properly.
> > > > > >> > Please resubmit whatever final solution is agreed upon.
> > > > > >> I noticed that v2.6.39 was tagged today. We should definitely remove
> > > > > >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> > > > > >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> > > > > >> in - if we decide that the SFEATURES compatibility should be removed
> > > > > >> it won't matter.
> > [...]
> > > > We could just wait for 2.6.40 and pretend this code part never existed. ;-)
> > > I think I will make ethtool check for a minimum kernel version of 2.6.40
> > > before using ETHTOOL_{G,S}FEATURES.
> > 
> > > > I'll rebase the first patch tomorrow. Without it the compatibility in
> > > > ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.
> > > This is an improvement, but I still think the fallback is fundamentally
> > > broken - there's no good way for userland to tell what (if anything)
> > > went wrong when the return value has ETHTOOL_F_COMPAT set.
> > 
> > The same situation happens with ETHTOOL_F_WISH (userspace needs to reread
> > the features to find out what happened) and with old ETHTOOL_S{TSO,SG,...}
> > (those return success if any of the features in the group changes and also
> > posibly disable other features when one is disabled). This wasn't really
> > checked before.
> > 
> > Ben, I think I commented on your proposal of the userspace part, but I might
> > have missed some of your arguments about mine. Let's sum those up:
> > 
> > Your version:
> >    - reimplements ETHTOOL_Sxx via ETHTOOL_SFEATURES in userspace for kernels
> >      supporting the latter
> No, it implements 'ethtool -K' using ETHTOOL_SFEATURES.  Maybe you
> consider the ethtool utility to be a thin wrapper over the ethtool API,
> but that is not my intent.

You're right. I assumed just that. -K and other modes of operation look
like they are adapting the ETHTOOL_* calls for human consumption.
There's some additional code for analysing register and other dumps,
but I see it as just merging two tools for convenience.

> >      (note: ETHTOOL_S{SG,...} are not ever going away)
> >    - causes NETIF_F_* to be an ABI
> If feature flag numbers are not stable then what is the point of
> /sys/class/net/<name>/features?  Also, I'm not aware that features have
> ever been renumbered in the past.

Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
is a debugging aid. What is it used for besides that?

[...]
> > Both versions are rough at the edges, but working. Both assume that no
> > behaviour changes are to be made for old '-K' options.
> No, my changes are intended to enhance the old options.

Kernel side already enhances them to not forget other features
'wanted' state. What other enhancements are on your mind?

BTW, I just recalled that ETHTOOL_F_WISH is set only as an information
about bits in features[].valid masks. So zero return does not mean, that
other features (not in .valid masks) haven't changed. This means that
userspace needs to reread features after any SFEATURES call, not just
those returning non-zero.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html