Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Edward Cree <ecree@solarflare.com>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: <netfilter-devel@vger.kernel.org>, <davem@davemloft.net>,
	<netdev@vger.kernel.org>, <jakub.kicinski@netronome.com>,
	<jiri@resnulli.us>, <saeedm@mellanox.com>, <vishal@chelsio.com>,
	<vladbu@mellanox.com>
Subject: Re: [PATCH net-next,v3 0/4] flow_offload: update mangle action representation
Date: Fri, 6 Sep 2019 17:49:07 +0100
Message-ID: <1637ec50-daae-65df-fcaa-bfd763dbb1d9@solarflare.com> (raw)
In-Reply-To: <20190906155804.v4lviltxs72a45tq@salvia>

On 06/09/2019 16:58, Pablo Neira Ayuso wrote:
> In tc pedit ex, those are _indeed_ two separated actions: 
I read the code again and I get it now, there's double iteration
 already over tcf_exts_for_each_action and tcf_pedit_nkeys, and
 it's only within the latter that you coalesce.

However, have you considered that iproute2 (i.e. tc tool) isn't
 guaranteed to be the only userland consumer of the TC uAPI?  For all
 we know there could be another user out there producing things like
 a single pedit action with two keys, same offset but different
 masks, to mangle sport & dport separately, which your code now
 _would_ coalesce into a single mangle.  I don't know if that would
 lead to any problems, but I want to be sure you've thought about it ;)

>> Proper thing to do is have helper functions available to drivers to test
>> the pedit, and not just switch on the offset.  Why do I say that?
>>
>> Well, consider a pedit on UDP dport, with mask 0x00ff (network endian).
>> Now as a u32 pedit that's 0x000000ff offset 0, so field-blind offset
>> calculation (ffs in flow_action_mangle_entry()) will turn that into
>>  offset 3 mask 0xff.  Now driver does
>>     switch(offset) { /* 3 */
>>     case offsetof(struct udphdr, dest): /* 2 */
>>         /* Whoops, we never get here! */
>>     }
>>
>> Do you see the problem?
> This scenario you describe cannot _work_ right now, with the existing
> code. Without my patchset, this scenario you describe does _not_ work,
>
> The drivers in the tree need a mask of 0xffff to infer that this is
> UDP dport.
>
> The 'tc pedit ex' infrastructure does not allow for the scenario that
> you describe above.
>
> No driver in the tree allow for what you describe already.
Looks to me like existing nfp_fl_set_tport() handles just fine any
 arbitrary mask across the u32 that contains UDP sport & dport.
And the uAPI we have to maintain is the uAPI we expose, not the subset
 that iproute2 uses.  I could write a patched tc tool *today* that does
 a pedit of 'UDP header, offset 0, mask 0xff0000ff' and the nfp driver
 would accept that fine (no idea what the fw / chip would do with it,
 but presumably it works or Netronome folks would have put checks in),
 whereas with your patch it'll complain "invalid pedit L4 action"
 because the mask isn't all-1s.
And if I made it produce my example from above, mask 0x000000ff, you'd
 calculate an offset of 3 and hit the other error, "unsupported section
 of L4 header", which again would have worked before.

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-06  0:03 Pablo Neira Ayuso
2019-09-06  0:04 ` [PATCH net-next,v3 1/4] net: flow_offload: flip mangle action mask Pablo Neira Ayuso
2019-09-06  0:04 ` [PATCH net-next,v3 2/4] net: flow_offload: bitwise AND on mangle action value field Pablo Neira Ayuso
2019-09-06  0:04 ` [PATCH net-next,v3 3/4] net: flow_offload: mangle action at byte level Pablo Neira Ayuso
2019-09-06  0:04 ` [PATCH net-next,v3 4/4] netfilter: nft_payload: packet mangling offload support Pablo Neira Ayuso
2019-09-06 10:02 ` [PATCH net-next,v3 0/4] flow_offload: update mangle action representation Edward Cree
2019-09-06 10:56   ` Pablo Neira Ayuso
2019-09-06 12:55     ` Edward Cree
2019-09-06 13:14       ` Pablo Neira Ayuso
2019-09-06 13:37         ` Edward Cree
2019-09-06 14:50           ` Pablo Neira Ayuso
2019-09-06 15:13             ` Edward Cree
2019-09-06 15:58               ` Pablo Neira Ayuso
2019-09-06 16:49                 ` Edward Cree [this message]
2019-09-06 18:15                   ` Pablo Neira Ayuso

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1637ec50-daae-65df-fcaa-bfd763dbb1d9@solarflare.com \
    --to=ecree@solarflare.com \
    --cc=davem@davemloft.net \
    --cc=jakub.kicinski@netronome.com \
    --cc=jiri@resnulli.us \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    --cc=saeedm@mellanox.com \
    --cc=vishal@chelsio.com \
    --cc=vladbu@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git