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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20140319.155930.1805664690676428310.davem@davemloft.net>
Date:	Wed, 19 Mar 2014 15:59:30 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	VenkatKumar.Duvvuru@...lex.com
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v5 net-next 0/2] patchset: Support for configurable RSS
 hash key

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.

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.

And the data structure is furthermore named in a way that suggests
it's just for doing things with the RSS hash.

Just add:

struct ethtool_rxfh {
	__u32	cmd;
	__u32	key_size;
	__u32	key[0];
};

And that's it.

To change the indirection table, call "ETHTOOL_SRXFH".  To set the
RSS flow hash, call "ETHTOOL_SRSSH".
--
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