netdev - Re: [PATCH net-next,v4 09/12] ethtool: add basic ethtool_rx_flow_spec to flow

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

Message-ID: <20181205135634.GA23191@unicorn.suse.cz>
Date:   Wed, 5 Dec 2018 14:56:34 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next,v4 09/12] ethtool: add basic
 ethtool_rx_flow_spec to flow_rule structure translator

On Wed, Dec 05, 2018 at 02:10:37PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 29, 2018 at 02:12:14PM +0100, Michal Kubecek wrote:
> > On Thu, Nov 29, 2018 at 03:22:28AM +0100, Pablo Neira Ayuso wrote:
> > ...
> > > +	act = &flow->rule->action.entries[0];
> > > +	switch (fs->ring_cookie) {
> > > +	case RX_CLS_FLOW_DISC:
> > > +		act->id = FLOW_ACTION_DROP;
> > > +		break;
> > > +	case RX_CLS_FLOW_WAKE:
> > > +		act->id = FLOW_ACTION_WAKE;
> > > +		break;
> > > +	default:
> > > +		act->id = FLOW_ACTION_QUEUE;
> > > +		act->queue_index = fs->ring_cookie;
> > > +		break;
> > > +	}
> > 
> > IMHO it would be cleaner if rules passing packets to RSS contexts were
> > also implemented using action, e.g. by using FLOW_ACTION_CONTEXT for
> > act->id and reusing act->queue_index for context id (using either an
> > anonymous union or more generic name).
> 
> Are refering to FLOW_RSS specifically? Or just rename in this action
> and field?

My idea is that currently the ethtool interface can apply four types of
action to matching packets

  - put into a specific queue
  - discard
  - distribute accross the queues according to a RSS context
  - use the rule as a wake-on-lan filter

What is confusing is that due to lack of extensibility of the ethtool
ioctl interface, third option is implemented by setting FLOW_RSS flag in
ethtool_rxnfc::fs.flow_type and passing the RSS context id in
ethtool_rxnfc::rss_context, i.e. outside struct ethtool_rx_flow_spec.
The context id would need to be passed to the conversion function in
addition to the struct ethtool_rx_flow_spec pointer.

I believe we should use this opportunity to handle this case in a way
which would match the other three. In particular by introducing another
value for act->id (e.g. FLOW_ACTION_CONTEXT) and passing the target
context id in the same structure (e.g. by renaming queue_index to
something more generic or by using an anonymous union with it).

Michal Kubecek