[PATCH net 0/2] nfp: flower: a few small conntrack offload fixes

[PATCH net 0/2] nfp: flower: a few small conntrack offload fixes

[Louis Peens]

This small series addresses two bugs in the nfp conntrack offloading
code.

The first patch is a check to prevent offloading for a case which is
currently not supported by the nfp.

The second patch fixes up parsing of layer4 mangling code so it can be
correctly offloaded.

Hui Zhou (2):
  nfp: flower: add hardware offload check for post ct entry
  nfp: flower: fix hardware offload for the transfer layer port

 .../ethernet/netronome/nfp/flower/conntrack.c | 42 +++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH net 1/2] nfp: flower: add hardware offload check for post ct entry

[Louis Peens]

From: Hui Zhou <hui.zhou@corigine.com>

The nfp offload flow pay will not allocate a mask id when the out port
is openvswitch internal port. This is because these flows are used to
configure the pre_tun table and are never actually send to the firmware
as an add-flow message. When a tc rule which action contains ct and
the post ct entry's out port is openvswitch internal port, the merge
offload flow pay with the wrong mask id of 0 will be send to the
firmware. Actually, the nfp can not support hardware offload for this
situation, so return EOPNOTSUPP.

Fixes: bd0fe7f96a3c ("nfp: flower-ct: add zone table entry when handling pre/post_ct flows")
CC: stable@vger.kernel.org # 5.14+
Signed-off-by: Hui Zhou <hui.zhou@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 .../ethernet/netronome/nfp/flower/conntrack.c | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
index 2967bab72505..726d8cdf0b9c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
@@ -1864,10 +1864,30 @@ int nfp_fl_ct_handle_post_ct(struct nfp_flower_priv *priv,
 {
 	struct flow_rule *rule = flow_cls_offload_flow_rule(flow);
 	struct nfp_fl_ct_flow_entry *ct_entry;
+	struct flow_action_entry *ct_goto;
 	struct nfp_fl_ct_zone_entry *zt;
+	struct flow_action_entry *act;
 	bool wildcarded = false;
 	struct flow_match_ct ct;
-	struct flow_action_entry *ct_goto;
+	int i;
+
+	flow_action_for_each(i, act, &rule->action) {
+		switch (act->id) {
+		case FLOW_ACTION_REDIRECT:
+		case FLOW_ACTION_REDIRECT_INGRESS:
+		case FLOW_ACTION_MIRRED:
+		case FLOW_ACTION_MIRRED_INGRESS:
+			if (act->dev->rtnl_link_ops &&
+			    !strcmp(act->dev->rtnl_link_ops->kind, "openvswitch")) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "unsupported offload: out port is openvswitch internal port");
+				return -EOPNOTSUPP;
+			}
+			break;
+		default:
+			break;
+		}
+	}
 
 	flow_rule_match_ct(rule, &ct);
 	if (!ct.mask->ct_zone) {
-- 
2.34.1

[PATCH net 2/2] nfp: flower: fix hardware offload for the transfer layer port

[Louis Peens]

From: Hui Zhou <hui.zhou@corigine.com>

The nfp driver will merge the tp source port and tp destination port
into one dword which the offset must be zero to do hardware offload, but
the mangle action for the tp source port and tp destination port is
separated for tc ct action. Modify the mangle action for the
FLOW_ACT_MANGLE_HDR_TYPE_TCP and FLOW_ACT_MANGLE_HDR_TYPE_UDP to satisfy
the nfp driver offload check for the tp port.

Fixes: 5cee92c6f57a ("nfp: flower: support hw offload for ct nat action")
CC: stable@vger.kernel.org # 6.1+
Signed-off-by: Hui Zhou <hui.zhou@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
 .../ethernet/netronome/nfp/flower/conntrack.c | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
index 726d8cdf0b9c..8eaf3b1611a0 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
@@ -1424,10 +1424,26 @@ static void nfp_nft_ct_translate_mangle_action(struct flow_action_entry *mangle_
 		mangle_action->mangle.mask = (__force u32)cpu_to_be32(mangle_action->mangle.mask);
 		return;
 
+	/* Both struct tcphdr and struct udphdr start with
+	 *	__be16 source;
+	 *	__be16 dest;
+	 * so we can use the same code for both.
+	 */
 	case FLOW_ACT_MANGLE_HDR_TYPE_TCP:
 	case FLOW_ACT_MANGLE_HDR_TYPE_UDP:
-		mangle_action->mangle.val = (__force u16)cpu_to_be16(mangle_action->mangle.val);
-		mangle_action->mangle.mask = (__force u16)cpu_to_be16(mangle_action->mangle.mask);
+		if (mangle_action->mangle.offset == offsetof(struct tcphdr, source)) {
+			mangle_action->mangle.val =
+				(__force u32)cpu_to_be32(mangle_action->mangle.val << 16);
+			mangle_action->mangle.mask =
+				(__force u32)cpu_to_be32(mangle_action->mangle.mask << 16 | 0xFFFF);
+		}
+		if (mangle_action->mangle.offset == offsetof(struct tcphdr, dest)) {
+			mangle_action->mangle.offset = 0;
+			mangle_action->mangle.val =
+				(__force u32)cpu_to_be32(mangle_action->mangle.val);
+			mangle_action->mangle.mask =
+				(__force u32)cpu_to_be32(mangle_action->mangle.mask);
+		}
 		return;
 
 	default:
-- 
2.34.1

Re: [PATCH net 2/2] nfp: flower: fix hardware offload for the transfer layer port

[Jakub Kicinski]

On Fri,  8 Dec 2023 08:59:56 +0200 Louis Peens wrote:
> +		if (mangle_action->mangle.offset == offsetof(struct tcphdr, source)) {
> +			mangle_action->mangle.val =
> +				(__force u32)cpu_to_be32(mangle_action->mangle.val << 16);
> +			mangle_action->mangle.mask =
> +				(__force u32)cpu_to_be32(mangle_action->mangle.mask << 16 | 0xFFFF);

This a bit odd. Here you fill in the "other half" of the mask with Fs...

> +		}
> +		if (mangle_action->mangle.offset == offsetof(struct tcphdr, dest)) {
> +			mangle_action->mangle.offset = 0;
> +			mangle_action->mangle.val =
> +				(__force u32)cpu_to_be32(mangle_action->mangle.val);
> +			mangle_action->mangle.mask =
> +				(__force u32)cpu_to_be32(mangle_action->mangle.mask);
> +		}

.. but here you just let it be zero.

If it's correct it'd be good to explain in the commit msg why.
-- 
pw-bot: cr

Re: [PATCH net 2/2] nfp: flower: fix hardware offload for the transfer layer port

[Louis Peens]

On Mon, Dec 11, 2023 at 07:08:49PM -0800, Jakub Kicinski wrote:
> On Fri,  8 Dec 2023 08:59:56 +0200 Louis Peens wrote:
> > +		if (mangle_action->mangle.offset == offsetof(struct tcphdr, source)) {
> > +			mangle_action->mangle.val =
> > +				(__force u32)cpu_to_be32(mangle_action->mangle.val << 16);
> > +			mangle_action->mangle.mask =
> > +				(__force u32)cpu_to_be32(mangle_action->mangle.mask << 16 | 0xFFFF);
> 
> This a bit odd. Here you fill in the "other half" of the mask with Fs...
> 
> > +		}
> > +		if (mangle_action->mangle.offset == offsetof(struct tcphdr, dest)) {
> > +			mangle_action->mangle.offset = 0;
> > +			mangle_action->mangle.val =
> > +				(__force u32)cpu_to_be32(mangle_action->mangle.val);
> > +			mangle_action->mangle.mask =
> > +				(__force u32)cpu_to_be32(mangle_action->mangle.mask);
> > +		}
> 
> .. but here you just let it be zero.
> 
> If it's correct it'd be good to explain in the commit msg why.

Thanks for asking, it does indeed look a bit strange. It has to do with
act_ct using the inverse value of the mask, basically requiring a
rotate-left operation for the source field. It can definitely do with a
better explanation. Will submit a v2 doing so.

> -- 
> pw-bot: cr