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]

Message-ID: <20110516205137.GA7667@rere.qmqm.pl>
Date:	Mon, 16 May 2011 22:51:37 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT
 return

On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > all drivers are converted to fix/set_features.
> > > > 
> > > > Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> > > > ---
> > > > 
> > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > [...]
> > > ETHTOOL_F_WISH means that the requested features could not all be
> > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > remembered.
> > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > (net: Implement SFEATURES compatibility for not updated drivers).
> That's also problematic because it means we can't make any use of the
> 'available' masks from ETHTOOL_GFEATURES.
> 
> The patch I sent is actually tested with a modified ethtool.  The
> fallback works.  I don't think you've tested whether any of your
> proposals can actually practically be used by ethtool.

While reading your patches I noted some differences in the way we see
the new [GS]FEATURES ops.

First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
feature names become an ABI instead. That's what ETH_SS_FEATURES string
set is for, and that's what comments in kernel's <linux/ethtool.h>
include say.

dev->features are exposed directly by kernel only in two ways:
 1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
    in headers for userspace, this should be treated like a debugging
    facility and not an ABI
 2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
    and RX hashing) that are renamed to ETH_FLAG_* - only those constants
    are in the ABI and only in relation with ETHTOOL_[GS]FLAGS

Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel? The
assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
but there is an conversion layer in kernel that allows old binaries to
work correctly in the common case. (-EOPNOTSUPP is still returned for
drivers which can't change particular feature. The difference is seen
only in that disabling and enabling e.g. checksumming won't disable other
dependent features in the result.)

Right now we already agree that NETIF_F_COMPAT should go.

I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
This might be made even more useful by adding simple wildcard matching.

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