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@xxxxxxxxxxxxx>
> ---
> 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)
