Re: [PATCH net-next,v5 3/4] net: flow_offload: mangle action at byte level

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Tue, 15 Oct 2019 00:10:50 +0200, Pablo Neira Ayuso wrote:
> The flow mangle action is originally modeled after the tc pedit action,
> this has a number of shortcomings:
> 
> 1) The tc pedit offset must be set on the 32-bits boundaries. Many
>    protocol header field offsets are not aligned to 32-bits, eg. port
>    destination, port source and ethernet destination. This patch adjusts
>    the offset accordingly and trim off length in these case, so drivers get
>    an exact offset and length to the header fields.
> 
> 2) The maximum mangle length is one word of 32-bits, hence you need to
>    up to four actions to mangle an IPv6 address. This patch coalesces
>    consecutive tc pedit actions into one single action so drivers can
>    configure the IPv6 mangling in one go. Ethernet address fields now
>    require one single action instead of two too.
> 
> This patch finds the header field from the 32-bit offset and mask. If
> there is no matching header field, fall back to passing the original
> list of mangle actions to the driver.
> 
> The following drivers have been updated accordingly to use this new
> mangle action layout:
> 
> 1) The cxgb4 driver does not need to split protocol field matching
>    larger than one 32-bit words into multiple definitions. Instead one
>    single definition per protocol field is enough. Checking for
>    transport protocol ports is also simplified.
> 
> 2) The mlx5 driver logic to disallow IPv4 ttl and IPv6 hoplimit fields
>    becomes more simple too.
> 
> 3) The nfp driver uses the nfp_fl_set_helper() function to configure the
>    payload mangling. The memchr_inv() function is used to check for
>    proper initialization of the value and mask. The driver has been
>    updated to refer to the exact protocol header offsets too.
> 
> As a result, this patch reduces code complexity on the driver side at
> the cost of adding code to the core to perform offset and length
> adjustment; and to coalesce consecutive actions.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v5: add header field definitions to calculate header field from offset
>     and mask.

Let's see if I can recount the facts:
 (1) this is a "improvement" to simplify driver work but driver
     developers (Ed and I) don't like it;
 (2) it's supposed to simplify things yet it makes the code longer;
 (3) it causes loss of functionality (looks like a single u32 changing
     both sport and dport is rejected by the IR since it wouldn't
     match fields); 
 (4) at v5 it still is buggy (see below).

The motivation for this patch remains unclear.

You are posting new versions at a slow pace which makes it hard to
keep re-reviewing it (v1 on Aug 30th).

With that I'd like to one more time ask you to please stop reposting
this patch and post patches 1 and 2 as separate improvements.

>  int tc_setup_flow_action(struct flow_action *flow_action,
>  			 const struct tcf_exts *exts, bool rtnl_held)
>  {
>  	const struct tc_action *act;
> -	int i, j, k, err = 0;
> +	int i, j, err = 0;
>  
>  	if (!exts)
>  		return 0;
> @@ -3396,25 +3612,9 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>  		} else if (is_tcf_tunnel_release(act)) {
>  			entry->id = FLOW_ACTION_TUNNEL_DECAP;
>  		} else if (is_tcf_pedit(act)) {
> -			for (k = 0; k < tcf_pedit_nkeys(act); k++) {
> -				switch (tcf_pedit_cmd(act, k)) {
> -				case TCA_PEDIT_KEY_EX_CMD_SET:
> -					entry->id = FLOW_ACTION_MANGLE;
> -					break;
> -				case TCA_PEDIT_KEY_EX_CMD_ADD:
> -					entry->id = FLOW_ACTION_ADD;
> -					break;
> -				default:
> -					err = -EOPNOTSUPP;
> -					goto err_out;
> -				}
> -				entry->mangle.htype = tcf_pedit_htype(act, k);
> -				entry->mangle.mask = ~tcf_pedit_mask(act, k);
> -				entry->mangle.val = tcf_pedit_val(act, k) &
> -							entry->mangle.mask;
> -				entry->mangle.offset = tcf_pedit_offset(act, k);
> -				entry = &flow_action->entries[++j];
> -			}
> +			j = flow_action_mangle(flow_action, act, j);
> +			if (j < 0)
> +				goto err_out;

Here we goto out without setting err, so return success even though the
actions didn't really get translated?

Any error from flow_action_mangle() seems to get effectively ignored?

>  		} else if (is_tcf_csum(act)) {
>  			entry->id = FLOW_ACTION_CSUM;
>  			entry->csum_flags = tcf_csum_update_flags(act);
> @@ -3468,9 +3668,9 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>  			goto err_out;
>  		}
>  
> -		if (!is_tcf_pedit(act))
> -			j++;
> +		j++;
>  	}
> +	flow_action->num_entries = j;
>  
>  err_out:
>  	if (!rtnl_held)