Extend XFRM core to allow packet offload configuration

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_policy.c +7 -0

@@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (!dev)
 		goto free_dst;
 
+	dst1 = &xdst0->u.dst;
+	/* Packet offload: both policy and SA should be offloaded */
+	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
+	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
+		err = -EINVAL;

Quoted 18 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_policy.c +12 -0

@@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (!dev)
 		goto free_dst;
 
+	dst1 = &xdst0->u.dst;
+	/* Packet offload: both policy and SA should be offloaded */
+	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
+	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+
+	/* Packet offload: both policy and SA should have same device */
+	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&

net/xfrm/xfrm_policy.c static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy, · line 2729

Quoted 5 lines

net/xfrm/xfrm_policy.c +5 -0

+	    policy->xdo.dev != dst1->xfrm->xso.dev) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+

This is the wrong place for these checks. Things went already wrong in the lookup if policy and state do not match here.

Quoted 19 lines · Steffen Klassert

Quoted 18 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_policy.c +12 -0

@@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (!dev)
 		goto free_dst;
 
+	dst1 = &xdst0->u.dst;
+	/* Packet offload: both policy and SA should be offloaded */
+	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
+	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+
+	/* Packet offload: both policy and SA should have same device */
+	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&

Quoted 8 lines

net/xfrm/xfrm_policy.c static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy, · line 2729

Quoted 5 lines

net/xfrm/xfrm_policy.c +5 -0

+	    policy->xdo.dev != dst1->xfrm->xso.dev) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+

This is the wrong place for these checks. Things went already wrong in the lookup if policy and state do not match here.

Where do you think we should put such checks?

Quoted 29 lines · Leon Romanovsky

Quoted 27 lines · Steffen Klassert

Quoted 13 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_policy.c +7 -0

@@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (!dev)
 		goto free_dst;
 
+	dst1 = &xdst0->u.dst;
+	/* Packet offload: both policy and SA should be offloaded */
+	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
+	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
+		err = -EINVAL;

net/xfrm/xfrm_policy.c static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy, · line 2729

Quoted 10 lines

net/xfrm/xfrm_policy.c +10 -0

+		goto free_dst;
+	}
+
+	/* Packet offload: both policy and SA should have same device */
+	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	    policy->xdo.dev != dst1->xfrm->xso.dev) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+

This is the wrong place for these checks. Things went already wrong in the lookup if policy and state do not match here.

Quoted 1 line

Where do you think we should put such checks?

You need to create a new lookup key for this and match the policies template against the TS of the state. This happens in xfrm_state_find. Unfortunately this affects also the SW datapath even without HW policies/states. So please try to make it a NOP if there are no HW policies/states.

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_state.c +7 -3

@@ -1166,16 +1187,24 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			spin_lock_bh(&net->xfrm.xfrm_state_lock);
 			x->km.state = XFRM_STATE_ACQ;
 			list_add(&x->km.all, &net->xfrm.state_all);
-			hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
+			XFRM_STATE_INSERT(bydst, &x->bydst,
+					  net->xfrm.state_bydst + h,
+					  x->xso.type);
 			h = xfrm_src_hash(net, daddr, saddr, encap_family);
-			hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
+			XFRM_STATE_INSERT(bysrc, &x->bysrc,
+					  net->xfrm.state_bysrc + h,
+					  x->xso.type);
 			if (x->id.spi) {
 				h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
-				hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
+				XFRM_STATE_INSERT(byspi, &x->byspi,

Quoted 23 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_state.c +9 -3

@@ -1166,16 +1187,24 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			spin_lock_bh(&net->xfrm.xfrm_state_lock);
 			x->km.state = XFRM_STATE_ACQ;
 			list_add(&x->km.all, &net->xfrm.state_all);
-			hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
+			XFRM_STATE_INSERT(bydst, &x->bydst,
+					  net->xfrm.state_bydst + h,
+					  x->xso.type);
 			h = xfrm_src_hash(net, daddr, saddr, encap_family);
-			hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
+			XFRM_STATE_INSERT(bysrc, &x->bysrc,
+					  net->xfrm.state_bysrc + h,
+					  x->xso.type);
 			if (x->id.spi) {
 				h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
-				hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
+				XFRM_STATE_INSERT(byspi, &x->byspi,
+						  net->xfrm.state_byspi + h,
+						  x->xso.type);
 			}
 			if (x->km.seq) {

net/xfrm/xfrm_state.c xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, · line 1208

Quoted 6 lines

net/xfrm/xfrm_state.c +3 -1

 				h = xfrm_seq_hash(net, x->km.seq);
-				hlist_add_head_rcu(&x->byseq, net->xfrm.state_byseq + h);
+				XFRM_STATE_INSERT(byseq, &x->byseq,
+						  net->xfrm.state_byseq + h,
+						  x->xso.type);
 			}

This does not work. A larval state will never have a x->xso.type set.

So this raises the question how to handle acquires with this packet offload.

You could place the type and offload device to the template, but we also have to make sure not to mess too much with the non offloaded codepath.

This is yet another corner case where the concept of doing policy and state lookup in software for a HW offload does not work so well. I fear this is not the last corner case that comes up once you put this into a real network.

Quoted 4 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

This does not work. A larval state will never have a x->xso.type set.

x->xso.type always exists. Default is 0, which is XFRM_DEV_OFFLOAD_UNSPECIFIED. It means this XFRM_STATE_INSERT() will behave exactly as hlist_add_head_rcu() before.

Quoted 3 lines

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

Quoted 9 lines

You could place the type and offload device to the template, but we also have to make sure not to mess too much with the non offloaded codepath.

This is yet another corner case where the concept of doing policy and state lookup in software for a HW offload does not work so well. I fear this is not the last corner case that comes up once you put this into a real network.

It is not different from any other kernel code, bugs will be fixed.

Quoted 8 lines · Leon Romanovsky

Quoted 4 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

This does not work. A larval state will never have a x->xso.type set.

x->xso.type always exists. Default is 0, which is XFRM_DEV_OFFLOAD_UNSPECIFIED. It means this XFRM_STATE_INSERT() will behave exactly as hlist_add_head_rcu() before.

Sure it exists, and is always 0 here.

Quoted 4 lines

Quoted 3 lines

So this raises the question how to handle acquires with this packet offload.

Quoted 1 line

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 5 lines

Quoted 5 lines

You could place the type and offload device to the template, but we also have to make sure not to mess too much with the non offloaded codepath.

Quoted 7 lines

Quoted 5 lines

This is yet another corner case where the concept of doing policy and state lookup in software for a HW offload does not work so well. I fear this is not the last corner case that comes up once you put this into a real network.

It is not different from any other kernel code, bugs will be fixed.

The thing that is different here is, that the concept is already broken. We can't split the datapath to be partially handled in SW and HW in any sane way, this becomes clearer and clearer.

The full protocol offload simply does not fit well into HW, but we try to make it fit with a hammer. This is the problem why I do not really like this, and is also the reason why this is still not merged. We might be much better of by doing a HW frindly redesign of the protocol and offload this then. But, yes that takes time and will have issues too.

Quoted 10 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

Quoted 4 lines

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

For now, all such packets will be dropped as we have offlaoded policy but not SA.

Quoted 6 lines

Quoted 1 line

It is not different from any other kernel code, bugs will be fixed.

The thing that is different here is, that the concept is already broken. We can't split the datapath to be partially handled in SW and HW in any sane way, this becomes clearer and clearer.

Quoted 6 lines

The full protocol offload simply does not fit well into HW, but we try to make it fit with a hammer. This is the problem why I do not really like this, and is also the reason why this is still not merged. We might be much better of by doing a HW frindly redesign of the protocol and offload this then. But, yes that takes time and will have issues too.

When you say "protocol", what do you mean? Many users, who have deployed IPsec solutions, just want to have same look and feel but much faster.

I truly believe that this packet offload fits SW model and the (small) amount of changes supports it. There are almost no changes to the stack to natively support this offload.

As long as HW involved, you will never have solution without issues, and like you said even redesign "will have issues".

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 2 lines

For now, all such packets will be dropped as we have offlaoded policy but not SA.

I think you missed my point. If the HW policy does not match the SW acquire state, then each packet will geneate a new acquire. So you need to make sure that policy and acquire state will match to send the acquire just once to userspace.

Quoted 22 lines

Quoted 7 lines

Quoted 2 lines

It is not different from any other kernel code, bugs will be fixed.

The thing that is different here is, that the concept is already broken. We can't split the datapath to be partially handled in SW and HW in any sane way, this becomes clearer and clearer.

Quoted 6 lines

The full protocol offload simply does not fit well into HW, but we try to make it fit with a hammer. This is the problem why I do not really like this, and is also the reason why this is still not merged. We might be much better of by doing a HW frindly redesign of the protocol and offload this then. But, yes that takes time and will have issues too.

When you say "protocol", what do you mean? Many users, who have deployed IPsec solutions, just want to have same look and feel but much faster.

I truly believe that this packet offload fits SW model and the (small) amount of changes supports it. There are almost no changes to the stack to natively support this offload.

Quoted 2 lines

As long as HW involved, you will never have solution without issues, and like you said even redesign "will have issues".

Things would be much easier, if we don't need to add HW policies and states to SW databases. But yes, a redesign might have issues too. That's why we are still working on the current soluion :)

Quoted 17 lines · Steffen Klassert

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 7 lines

Quoted 2 lines

For now, all such packets will be dropped as we have offlaoded policy but not SA.

I think you missed my point. If the HW policy does not match the SW acquire state, then each packet will geneate a new acquire. So you need to make sure that policy and acquire state will match to send the acquire just once to userspace.

I think that I'm still missing the point.

We require both policy and SA to be offloaded. It means that once we hit HW policy, we must hit SA too (at least this is how mlx5 part is implemented).

Quoted 28 lines · Leon Romanovsky

Quoted 17 lines · Steffen Klassert

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 7 lines

Quoted 2 lines

For now, all such packets will be dropped as we have offlaoded policy but not SA.

I think you missed my point. If the HW policy does not match the SW acquire state, then each packet will geneate a new acquire. So you need to make sure that policy and acquire state will match to send the acquire just once to userspace.

I think that I'm still missing the point.

Quoted 3 lines

We require both policy and SA to be offloaded. It means that once we hit HW policy, we must hit SA too (at least this is how mlx5 part is implemented).

Let's assume a packet hits a HW policy. Then this HW policy must match a HW state. In case there is no matching HW state, we generate an acquire and insert a larval state. Currently, larval states are never marked as HW.

Now, the next packet from the same flow maches again this HW policy, but it does not find the larval state because it is not marked as a HW state. So we generate another acquire and insert another larval state. Same happens for packets 3,4,5...

Expected behaviour for subsequent packets is that the lookup will find a matching HW larval state and the packet is dropped without creating another acquire + larval state for the same flow.

Quoted 33 lines · Steffen Klassert

Quoted 31 lines · Leon Romanovsky

Quoted 17 lines · Steffen Klassert

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 7 lines

Quoted 2 lines

For now, all such packets will be dropped as we have offlaoded policy but not SA.

I think you missed my point. If the HW policy does not match the SW acquire state, then each packet will geneate a new acquire. So you need to make sure that policy and acquire state will match to send the acquire just once to userspace.

I think that I'm still missing the point.

We require both policy and SA to be offloaded. It means that once we hit HW policy, we must hit SA too (at least this is how mlx5 part is implemented).

Quoted 3 lines

Let's assume a packet hits a HW policy. Then this HW policy must match a HW state. In case there is no matching HW state, we generate an acquire and insert a larval state. Currently, larval states are never marked as HW.

And this is there our views are different. If HW (in RX) sees policy but doesn't have state, this packet will be dropped in HW. It won't get to stack and no acquire request will be issues.

Quoted 38 lines · Leon Romanovsky

Quoted 36 lines · Steffen Klassert

Quoted 26 lines · Leon Romanovsky

Quoted 17 lines · Steffen Klassert

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 7 lines

Quoted 2 lines

For now, all such packets will be dropped as we have offlaoded policy but not SA.

I think you missed my point. If the HW policy does not match the SW acquire state, then each packet will geneate a new acquire. So you need to make sure that policy and acquire state will match to send the acquire just once to userspace.

Quoted 5 lines

I think that I'm still missing the point.

We require both policy and SA to be offloaded. It means that once we hit HW policy, we must hit SA too (at least this is how mlx5 part is implemented).

Let's assume a packet hits a HW policy. Then this HW policy must match a HW state. In case there is no matching HW state, we generate an acquire and insert a larval state. Currently, larval states are never marked as HW.

Quoted 3 lines

And this is there our views are different. If HW (in RX) sees policy but doesn't have state, this packet will be dropped in HW. It won't get to stack and no acquire request will be issues.

This makes no sense. Acquires are always generated at TX, never at RX.

On RX, the state lookup happens first, the policy must match to the decapsulated packet.

Quoted 39 lines · Steffen Klassert

Quoted 38 lines · Leon Romanovsky

Quoted 36 lines · Steffen Klassert

Quoted 26 lines · Leon Romanovsky

Quoted 17 lines · Steffen Klassert

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 5 lines · Steffen Klassert

Quoted 1 line · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

So this raises the question how to handle acquires with this packet offload.

We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is, we might match a HW policy but create a SW acquire state. This will not match anymore as soon as the lookup is implemented correctly.

Quoted 7 lines

Quoted 2 lines

For now, all such packets will be dropped as we have offlaoded policy but not SA.

I think you missed my point. If the HW policy does not match the SW acquire state, then each packet will geneate a new acquire. So you need to make sure that policy and acquire state will match to send the acquire just once to userspace.

Quoted 5 lines

I think that I'm still missing the point.

We require both policy and SA to be offloaded. It means that once we hit HW policy, we must hit SA too (at least this is how mlx5 part is implemented).

Let's assume a packet hits a HW policy. Then this HW policy must match a HW state. In case there is no matching HW state, we generate an acquire and insert a larval state. Currently, larval states are never marked as HW.

Quoted 5 lines

Quoted 3 lines

And this is there our views are different. If HW (in RX) sees policy but doesn't have state, this packet will be dropped in HW. It won't get to stack and no acquire request will be issues.

This makes no sense. Acquires are always generated at TX, never at RX.

Sorry, my bad. But why can't we drop all packets that don't have HW state? Why do we need to add larval?

Quoted 5 lines

On RX, the state lookup happens first, the policy must match to the decapsulated packet.

I think that something like this will do the trick.

net/xfrm/xfrm_state.c +24 -1

index 5076f9d7a752..d1c9ef857755 100644
@@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
        }
 }
 
+static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
+                                       struct xfrm_policy *p)
+{
+       /* Packet offload: both policy and SA should be offloaded */
+       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
+               return false;
+
+       /* Packet offload: both policy and SA should have same device */
+       if (p->xdo.dev != x->xso.dev)
+               return true;
+
+       return false;
+}
+
 struct xfrm_state *
 xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                const struct flowi *fl, struct xfrm_tmpl *tmpl,

@@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 found:
        x = best;
-       if (!x && !error && !acquire_in_progress) {
+       if (!x && !error && !acquire_in_progress &&
+           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
                if (tmpl->id.spi &&
                    (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
                                              tmpl->id.proto, encap_family)) != NULL) {

Quoted 50 lines · Leon Romanovsky

I think that something like this will do the trick.

net/xfrm/xfrm_state.c +24 -1

index 5076f9d7a752..d1c9ef857755 100644
@@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
        }
 }
 
+static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
+                                       struct xfrm_policy *p)
+{
+       /* Packet offload: both policy and SA should be offloaded */
+       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
+               return false;
+
+       /* Packet offload: both policy and SA should have same device */
+       if (p->xdo.dev != x->xso.dev)
+               return true;
+
+       return false;
+}
+
 struct xfrm_state *
 xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                const struct flowi *fl, struct xfrm_tmpl *tmpl,

@@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 found:
        x = best;
-       if (!x && !error && !acquire_in_progress) {
+       if (!x && !error && !acquire_in_progress &&
+           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
                if (tmpl->id.spi &&
                    (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
                                              tmpl->id.proto, encap_family)) != NULL) {

@@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                        *err = -EAGAIN;
                        x = NULL;
                }

net/xfrm/xfrm_state.c xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, · line 1256

Quoted 3 lines

net/xfrm/xfrm_state.c +3 -0

+               if (x && xfrm_state_and_policy_mixed(x, pol)) {
+                       *err = -EINVAL;
+                       x = NULL;

If policy and state do not match here, this means the lookup returned the wrong state. The correct state might still sit in the database. At this point, you should either have found a matching state, or no state at all.

Quoted 51 lines · Steffen Klassert

Quoted 50 lines · Leon Romanovsky

I think that something like this will do the trick.

net/xfrm/xfrm_state.c +24 -1

index 5076f9d7a752..d1c9ef857755 100644
@@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
        }
 }
 
+static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
+                                       struct xfrm_policy *p)
+{
+       /* Packet offload: both policy and SA should be offloaded */
+       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
+               return false;
+
+       /* Packet offload: both policy and SA should have same device */
+       if (p->xdo.dev != x->xso.dev)
+               return true;
+
+       return false;
+}
+
 struct xfrm_state *
 xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                const struct flowi *fl, struct xfrm_tmpl *tmpl,

@@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 found:
        x = best;
-       if (!x && !error && !acquire_in_progress) {
+       if (!x && !error && !acquire_in_progress &&
+           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
                if (tmpl->id.spi &&
                    (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
                                              tmpl->id.proto, encap_family)) != NULL) {

@@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                        *err = -EAGAIN;
                        x = NULL;
                }

Quoted 8 lines

net/xfrm/xfrm_state.c xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, · line 1256

Quoted 3 lines

net/xfrm/xfrm_state.c +3 -0

+               if (x && xfrm_state_and_policy_mixed(x, pol)) {
+                       *err = -EINVAL;
+                       x = NULL;

If policy and state do not match here, this means the lookup returned the wrong state. The correct state might still sit in the database. At this point, you should either have found a matching state, or no state at all.

I check for "x" because of "x = NULL" above.

Quoted 61 lines · Leon Romanovsky

Quoted 59 lines · Steffen Klassert

Quoted 47 lines · Leon Romanovsky

I think that something like this will do the trick.

net/xfrm/xfrm_state.c +24 -1

index 5076f9d7a752..d1c9ef857755 100644
@@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
        }
 }
 
+static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
+                                       struct xfrm_policy *p)
+{
+       /* Packet offload: both policy and SA should be offloaded */
+       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
+               return false;
+
+       /* Packet offload: both policy and SA should have same device */
+       if (p->xdo.dev != x->xso.dev)
+               return true;
+
+       return false;
+}
+
 struct xfrm_state *
 xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                const struct flowi *fl, struct xfrm_tmpl *tmpl,

@@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 found:
        x = best;
-       if (!x && !error && !acquire_in_progress) {
+       if (!x && !error && !acquire_in_progress &&
+           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
                if (tmpl->id.spi &&
                    (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
                                              tmpl->id.proto, encap_family)) != NULL) {

net/xfrm/xfrm_state.c xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, · line 1256

Quoted 6 lines

net/xfrm/xfrm_state.c +3 -0

@@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                        *err = -EAGAIN;
                        x = NULL;
                }
+               if (x && xfrm_state_and_policy_mixed(x, pol)) {
+                       *err = -EINVAL;
+                       x = NULL;

If policy and state do not match here, this means the lookup returned the wrong state. The correct state might still sit in the database. At this point, you should either have found a matching state, or no state at all.

Quoted 1 line

I check for "x" because of "x = NULL" above.

This does not change the fact that the lookup returned the wrong state.

Quoted 9 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 6 lines · Steffen Klassert

On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:

If policy and state do not match here, this means the lookup returned the wrong state. The correct state might still sit in the database. At this point, you should either have found a matching state, or no state at all.

Quoted 3 lines

Quoted 1 line

I check for "x" because of "x = NULL" above.

This does not change the fact that the lookup returned the wrong state.

Steffen, but this is exactly why we added this check - to catch wrong states and configurations.

Quoted 10 lines · Leon Romanovsky

Quoted 9 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 6 lines · Steffen Klassert

On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:

If policy and state do not match here, this means the lookup returned the wrong state. The correct state might still sit in the database. At this point, you should either have found a matching state, or no state at all.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I check for "x" because of "x = NULL" above.

This does not change the fact that the lookup returned the wrong state.

Steffen, but this is exactly why we added this check - to catch wrong states and configurations.

No, you have to adjust the lookup so that this can't happen. This is not a missconfiguration, The lookup found the wrong SA, this is a difference.

Use the offload type and dev as a lookup key and don't consider SAs that don't match this in the lookup.

This is really not too hard to do. The thing that could be a bit more difficult is that the lookup should be only adjusted when we really have HW policies installed. Otherwise this affects even systems that don't use this kind of offload.

Quoted 25 lines · Steffen Klassert

Quoted 10 lines · Leon Romanovsky

Quoted 9 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 6 lines · Steffen Klassert

On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:

If policy and state do not match here, this means the lookup returned the wrong state. The correct state might still sit in the database. At this point, you should either have found a matching state, or no state at all.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I check for "x" because of "x = NULL" above.

This does not change the fact that the lookup returned the wrong state.

Steffen, but this is exactly why we added this check - to catch wrong states and configurations.

No, you have to adjust the lookup so that this can't happen. This is not a missconfiguration, The lookup found the wrong SA, this is a difference.

Use the offload type and dev as a lookup key and don't consider SAs that don't match this in the lookup.

Quoted 4 lines

This is really not too hard to do. The thing that could be a bit more difficult is that the lookup should be only adjusted when we really have HW policies installed. Otherwise this affects even systems that don't use this kind of offload.

Thanks for an explanation, trying it now.

Quoted 1 line · Leon Romanovsky

Thanks for an explanation, trying it now.

Something like that?

net/xfrm/xfrm_state.c +24 -0

+			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+				/* HW states are in the head of list, there is no need
+				 * to iterate further.
+				 */
+				break;
+
+			/* Packet offload: both policy and SA should have same device */
+			if (pol->xdo.dev != x->xso.dev)
+				continue;
+		}
+
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&

@@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
 	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
+		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
+		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
+			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+				/* HW states are in the head of list, there is no need
+				 * to iterate further.
+				 */
+				break;
+
+			/* Packet offload: both policy and SA should have same device */
+			if (pol->xdo.dev != x->xso.dev)
+				continue;
+		}
+
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&

@@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			goto out;
 		}
 

Quoted 4 lines · Leon Romanovsky

Quoted 1 line · Leon Romanovsky

Thanks for an explanation, trying it now.

Something like that?

Yes :)

Quoted 4 lines

The code is untested yet.

net/xfrm/xfrm_state.c xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, · line 1119

Quoted 10 lines

net/xfrm/xfrm_state.c +2 -0

index 5076f9d7a752..5819023c32ba 100644
@@ -1115,6 +1115,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 	rcu_read_lock();
 	h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
 	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
+		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
+		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {

Please try to avoid that check for every state in the list. Maybe enable this code with a static key if packet offload is used?

Quoted 39 lines

net/xfrm/xfrm_state.c +24 -0

+			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+				/* HW states are in the head of list, there is no need
+				 * to iterate further.
+				 */
+				break;
+
+			/* Packet offload: both policy and SA should have same device */
+			if (pol->xdo.dev != x->xso.dev)
+				continue;
+		}
+
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&

@@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
 	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
+		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
+		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
+			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+				/* HW states are in the head of list, there is no need
+				 * to iterate further.
+				 */
+				break;
+
+			/* Packet offload: both policy and SA should have same device */
+			if (pol->xdo.dev != x->xso.dev)
+				continue;
+		}
+
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&

@@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			goto out;
 		}
 

net/xfrm/xfrm_state.c xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, · line 1223

Quoted 10 lines

net/xfrm/xfrm_state.c +10 -0

+		if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
+			memcpy(&x->xso, &pol->xdo, sizeof(x->xso));
+			error = pol->xdo.dev->xfrmdev_ops->xdo_dev_state_add(x);
+			if (error) {
+				x->km.state = XFRM_STATE_DEAD;
+				to_put = x;
+				x = NULL;
+				goto out;
+			}
+		}

I guess that is to handle acquires, right?

What is the idea behind that?

Sorry, my bad. But why can't we drop all packets that don't have HW state? Why do we need to add larval?

Quoted 3 lines · Leon Romanovsky

Sorry, my bad. But why can't we drop all packets that don't have HW state? Why do we need to add larval?

The first packet of a flow tiggers an acquire and inserts a larval state. On a traffic triggered connection, we need this to get a state with keys installed.

We need this larval state then, because that tells us we sent already an acquire to userspace. All subsequent packets of that flow will be dropped without sending another acquire. Otherwise each subsequent packet will generate another acquire until the keys are negotiated. If a flow starts sending on a high rate, this would be not so nice for userspace :)

The thing is that this SW acquire flow is a fraction case, as it applies to locally generated traffic.

Quoted 3 lines · Leon Romanovsky

The thing is that this SW acquire flow is a fraction case, as it applies to locally generated traffic.

A router can trigger an acquire on forwarded packets too. Without larvals this could quickly overwhelm the router.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Quoted 7 lines · Herbert Xu

Quoted 3 lines · Leon Romanovsky

The thing is that this SW acquire flow is a fraction case, as it applies to locally generated traffic.

A router can trigger an acquire on forwarded packets too. Without larvals this could quickly overwhelm the router.

This series doesn't support tunnel mode yet.

Maybe I was not clear, but I wanted to say what in eswitch case and tunnel mode, the packets will be handled purely by HW without raising into SW core.

Quoted 10 lines · Leon Romanovsky

Quoted 7 lines · Herbert Xu

Quoted 3 lines · Leon Romanovsky

The thing is that this SW acquire flow is a fraction case, as it applies to locally generated traffic.

A router can trigger an acquire on forwarded packets too. Without larvals this could quickly overwhelm the router.

This series doesn't support tunnel mode yet.

It does not matter if tunnel or transport mode, acquires must work as expected. This is a fundamental concept of IPsec, there is no way to tell userspace that we don't support this.

Quoted 3 lines

Maybe I was not clear, but I wanted to say what in eswitch case and tunnel mode, the packets will be handled purely by HW without raising into SW core.

Can you please explain why we need host interaction for transport, but not for tunnel mode?

And as said already, I want to see the full picture (transport
+ tunnel mode) before we merge it.

Quoted 6 lines · Steffen Klassert

Quoted 2 lines · Leon Romanovsky

Quoted 1 line · Herbert Xu

On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:

Can you please explain why we need host interaction for transport, but not for tunnel mode?

The main difference is that in transport mode, you must bring packet to the kernel in which you configured SA/policy. It means that we must ensure that such packets won't be checked again in SW because all packets (encrypted and not) pass XFRM logic.

- wire -> RX NIC -> kernel -> XFRM stack (we need HW DB here to skip this stage) -> .... ... -> kernel -> XFRM stack (skip for HW SA/policies) -> TX NIC -> wire.

In tunnel mode, we arrive to XFRM when nothing IPsec related is configured.

- wire -> RX PF NIC -> eswitch NIC logic -> TX uplink NIC -> RX representors -> XFRM stack in VM (nothing configured here) -> kernel

Quoted 13 lines · Leon Romanovsky

Quoted 6 lines · Steffen Klassert

Quoted 2 lines · Leon Romanovsky

Quoted 1 line · Herbert Xu

On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:

Can you please explain why we need host interaction for transport, but not for tunnel mode?

The main difference is that in transport mode, you must bring packet to the kernel in which you configured SA/policy. It means that we must ensure that such packets won't be checked again in SW because all packets (encrypted and not) pass XFRM logic.

Quoted 7 lines

- wire -> RX NIC -> kernel -> XFRM stack (we need HW DB here to skip this stage) -> .... ... -> kernel -> XFRM stack (skip for HW SA/policies) -> TX NIC -> wire.

In tunnel mode, we arrive to XFRM when nothing IPsec related is configured.

- wire -> RX PF NIC -> eswitch NIC logic -> TX uplink NIC -> RX representors -> XFRM stack in VM (nothing configured here) -> kernel

Forget about eswitch, VM, etc. for a moment. I'm interested how the simplest possible tunnel mode cases will work.

Forwarding:

wire -> random NIC RX -> kernel -> IPsec tunnel offload NIC TX -> wire wire -> IPsec tunnel offload NIC RX -> kernel -> random NIC TX -> wire

Local endpoints:

Application -> kernel -> IPsec tunnel offload NIC TX -> wire wire -> IPsec tunnel offload NIC RX -> kernel -> Application

These two must work, so how are these cases handled?

If you can do more fancy things with tunnel mode and special NICs at TX and RX, that's fine but not absolutely required.

net/xfrm/xfrm_output.c static int xfrm_output_one(struct sk_buff *skb, int err) · line 563

Quoted 11 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

net/xfrm/xfrm_output.c +0 -1

index ce9e360a96e2..819c7cd87d6b 100644
@@ -560,7 +560,6 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEPROTOERROR);
 			goto error_nolock;
 		}
-

Nit: No need to remove that empty line.

From: Leon Romanovsky <[email protected]>

+and for packetoffload

+Full offload mode:

Quoted 3 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Quoted 1 line

+and for packetoffload

Maybe better 'packet offload'

Quoted 2 lines

+Full offload mode:

This is now 'Packet offload mode'

From: Leon Romanovsky <[email protected]>

Changelog:
v7: As was discussed in IPsec workshop:

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Changelog:
v7: As was discussed in IPsec workshop:

Steffen, can we please merge the series so we won't miss another kernel release?

Quoted 6 lines · Leon Romanovsky

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Changelog:
v7: As was discussed in IPsec workshop:

Quoted 2 lines

Steffen, can we please merge the series so we won't miss another kernel release?

I'm already reviewing the patchset. But as I said at the IPsec workshop, there is no guarantee that it will make it during this release cycle.

Quoted 7 lines · Steffen Klassert

Quoted 6 lines · Leon Romanovsky

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Changelog:
v7: As was discussed in IPsec workshop:

Quoted 6 lines

Quoted 2 lines

Steffen, can we please merge the series so we won't miss another kernel release?

I'm already reviewing the patchset. But as I said at the IPsec workshop, there is no guarantee that it will make it during this release cycle.

Quoted 8 lines · Leon Romanovsky

Quoted 7 lines · Steffen Klassert

Quoted 6 lines · Leon Romanovsky

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Changelog:
v7: As was discussed in IPsec workshop:

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Steffen, can we please merge the series so we won't miss another kernel release?

I'm already reviewing the patchset. But as I said at the IPsec workshop, there is no guarantee that it will make it during this release cycle.

BTW mlx5 patches are almost ready and fully reviewed, will be ready by early next week.

Steffen in case you will be ready, how do you want to receive the whole package? I guess we will need to post all the patches for a final time, including mlx5 driver.

Quoted 17 lines · Saeed Mahameed

Quoted 15 lines · Leon Romanovsky

Quoted 13 lines · Steffen Klassert

Quoted 8 lines · Leon Romanovsky

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Changelog:
v7: As was discussed in IPsec workshop:

Steffen, can we please merge the series so we won't miss another kernel release?

I'm already reviewing the patchset. But as I said at the IPsec workshop, there is no guarantee that it will make it during this release cycle.

Quoted 6 lines

BTW mlx5 patches are almost ready and fully reviewed, will be ready by early next week.

Steffen in case you will be ready, how do you want to receive the whole package? I guess we will need to post all the patches for a final time, including mlx5 driver.

I guess you want me to take both, the xfrm and mlx5 patches, right?

I'm ok with that if nobody else objects to it.

Please split it anyways into a xfrm and a mlx5 pachset. I'll merge both separate once we are ready.

Thanks!