netdev - RE: [net-next 03/15] i40evf: support ethtool RSS options

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] [day] [month] [year] [list]

Date:	Tue, 29 Apr 2014 23:51:45 +0000
From:	"Williams, Mitch A" <mitch.a.williams@...el.com>
To:	Ben Hutchings <ben@...adent.org.uk>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [net-next 03/15] i40evf: support ethtool RSS options



> -----Original Message-----
> From: Ben Hutchings [mailto:ben@...adent.org.uk]
> Sent: Tuesday, April 29, 2014 5:35 AM
> To: Kirsher, Jeffrey T
> Cc: davem@...emloft.net; Williams, Mitch A; netdev@...r.kernel.org;
> gospo@...hat.com; sassmann@...hat.com
> Subject: Re: [net-next 03/15] i40evf: support ethtool RSS options
> 
> On Mon, 2014-04-28 at 06:52 -0700, Jeff Kirsher wrote:
> [...]
> > +/**
> > + * i40evf_set_rss_hash_opt - Enable/Disable flow types for RSS hash
> > + * @adapter: board private structure
> > + * @cmd: ethtool rxnfc command
> > + *
> > + * Returns Success if the flow input set is supported.
> > + **/
> > +static int i40evf_set_rss_hash_opt(struct i40evf_adapter *adapter,
> > +				   struct ethtool_rxnfc *nfc)
> > +{
> [...]
> > +	case AH_ESP_V4_FLOW:
> > +	case AH_V4_FLOW:
> > +	case ESP_V4_FLOW:
> > +	case SCTP_V4_FLOW:
> > +		if ((nfc->data & RXH_L4_B_0_1) ||
> > +		    (nfc->data & RXH_L4_B_2_3))
> > +			return -EINVAL;
> > +		hena |= ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_OTHER);
> > +		break;
> > +	case AH_ESP_V6_FLOW:
> > +	case AH_V6_FLOW:
> > +	case ESP_V6_FLOW:
> > +	case SCTP_V6_FLOW:
> > +		if ((nfc->data & RXH_L4_B_0_1) ||
> > +		    (nfc->data & RXH_L4_B_2_3))
> > +			return -EINVAL;
> > +		hena |= ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_OTHER);
> > +		break;
> 
> I'm not sure why these cases operate on 'hena' as
> I40E_FILTER_PCTYPE_NONF_IPV{4,6}_OTHER are presumably always set in the
> register.
> 
> > +	case IPV4_FLOW:
> > +		hena |= ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_OTHER) |
> > +			((u64)1 << I40E_FILTER_PCTYPE_FRAG_IPV4);
> > +		break;
> > +	case IPV6_FLOW:
> > +		hena |= ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_OTHER) |
> > +			((u64)1 << I40E_FILTER_PCTYPE_FRAG_IPV6);
> > +		break;
> 
> These cases aren't validating that the user didn't ask for ports to be
> included in the hash.  They also interact oddly with UDP flow hashing
> configuration.  It seems to me that the I40E_FILTER_PCTYPE_FRAG_IPV{4,6}
> bits ought to be controllable through one flow type only.

This function is my best effort to reconcile how the hardware works with how the API works.

In general, you cannot control which field is hashed over for any given traffic type. You just enable or disable the traffic type.

However, if you disable a specific L4 protocol, the packet will still match on the underlying L3 protocol, as long as you leave the IP-specific bits turned on.

That said, you are correct, and I don't need to set the IP-specific bits because they're always turned on.

> 
> [...]
> > +static int i40evf_get_rxfh_indir(struct net_device *netdev, u32 *indir)
> > +{
> > +	struct i40evf_adapter *adapter = netdev_priv(netdev);
> > +	struct i40e_hw *hw = &adapter->hw;
> > +	u32 hlut_val;
> > +	int i, j;
> > +
> > +	for (i = 0, j = 0; i < I40E_VFQF_HLUT_MAX_INDEX; i++) {
> [...]
> 
> Off by 1?

Good catch, we all missed it.

I think Dave's already pulled this, and since it's nonfatal, I'll just fix these in a follow-on patch.

Thanks for your review.

-Mitch

> 
> Ben.
> 
> --
> Ben Hutchings
> Q.  Which is the greater problem in the world today, ignorance or apathy?
> A.  I don't know and I couldn't care less.