netdev - Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS hash key

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, 21 Mar 2014 15:28:36 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	ben@...adent.org.uk
Cc:	VenkatKumar.Duvvuru@...lex.com, netdev@...r.kernel.org
Subject: Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS
 hash key

From: Ben Hutchings <ben@...adent.org.uk>
Date: Fri, 21 Mar 2014 14:33:13 +0000

> On Wed, 2014-03-19 at 15:59 -0400, David Miller wrote:
>> From: Venkat Duvvuru <VenkatKumar.Duvvuru@...lex.com>
>> Date: Mon, 17 Mar 2014 18:01:33 +0530
>> 
>> > NIC drivers that support RSS use either a hard-coded value or a random value for
>> > the RSS hash key. Irrespective of the type of the key used, the user would want
>> > to change the hash key if he/she is not satisfied with the effectiveness of the
>> > default hash-key in spreading the incoming flows evenly across the RSS queues.
>> > 
>> > This patch set adds support for configuring the RSS hash-key via the ethtool
>> > interface using -X option.
>> 
>> I apologize, but I really dislike this.  For several reasons.
>> 
>> First, why aren't we adding _just_ a RSS hash changing interface?
>>
>> We already have an interface for changing the indirection table,
>> there is absolutely not need to add a second interface that supports
>> both indirection table _plus_ hash modifications.
> 
> That's what I asked for, because I see the hash key and indirection
> table as being a single logical object (RSS context).
> 
>> And combining these two is what leads to this hard to audit, ugly,
>> data structure layout.
>> 
>> There's the indirection table at some offset, then the key at some
>> other offset.  This makes it impossible to impose type checking
>> of any kind on both objects.
> [...]
> 
> Which is why I previously suggested making the ethtool core find the two
> arrays and pass those into the driver operations.  We do that for other
> ethtool operations that use even a single variable-length array.

Sure, we could hide the dirt in the generic ethtool code and pass
separated out pointers to the drivers.

But you have to recognize that the dirt will still exist in userspace
when it calls this stuff.

Nothing stops ethtool from providing a combined operation on the
command line, but that doesn't mean we necessarily have to have
a single kernel interface doing both thing.

I really still think that adding just a new key operation is the way
to go.
--
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