netdev - Re: [PATCH] net: don't lose features in netdev_add_tso

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: <20160517.141025.306603458593770186.davem@davemloft.net>
Date:	Tue, 17 May 2016 14:10:25 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	dmichail@...gle.com
Cc:	netdev@...r.kernel.org, edumazet@...gle.com
Subject: Re: [PATCH] net: don't lose features in netdev_add_tso_features()

From: Dimitris Michailidis <dmichail@...gle.com>
Date: Mon, 16 May 2016 15:33:35 -0700

> The goal of netdev_add_tso_features() is to enable all TSO features but
> it unintentionally loses NETIF_F_ALL_FOR_ALL features. This is because
> the netdev_increment_features() it calls clears any NETIF_F_ALL_FOR_ALL
> bits that aren't included in the incremental features and none of them
> are included in NETIF_F_ALL_TSO. The behavior can be seen by enabling
> tx-nocache-copy on the slaves and noticing the feature remains off at
> the master.
> 
> Fix this by including NETIF_F_ALL_FOR_ALL in the incremental features.
> 
> Signed-off-by: Dave Platt <dplatt@...gle.com>
> Signed-off-by: Dimitris Michailidis <dmichail@...gle.com>

This doesn't look right to me at all.

The second argument to netdev_increment_features, 'one', clearly
states that all it is supposed to do is handle a new device being
added which has the features mentioned in 'one'.  It makes no sense to
arbitrarily add ALL_FOR_ALL in there, and if it is legitimate, why
doesn't every other call site of netdev_increment_features have this
same problem?

If netdev_increment_features is doing something different either it's
documentation is wrong or it's implementation is losing ALL_FOR_ALL
bits erroneously.