Date:   Mon, 21 Nov 2022 14:02:52 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

On Mon, Nov 21, 2022 at 01:34:30PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 21, 2022 at 12:25:21PM +0100, Steffen Klassert wrote:
> > On Mon, Nov 21, 2022 at 01:15:36PM +0200, Leon Romanovsky wrote:
> > > On Mon, Nov 21, 2022 at 12:09:26PM +0100, Steffen Klassert wrote:
> > > > On Mon, Nov 21, 2022 at 12:27:01PM +0200, Leon Romanovsky wrote:
> > > > > On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> > > > > > On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > > > > > > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > > > > > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > > > > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > > > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > > > > > > From: Leon Romanovsky <leonro@...dia.com>
> > > > > > > > > 
> > > > > > > > > > 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.
> > > > > > > 
> > > > > > > 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).
> > > > 
> > > > 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.
> > 
> > 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?

I think that something like this will do the trick.

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 5076f9d7a752..d1c9ef857755 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -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;
                }
+               if (x && xfrm_state_and_policy_mixed(x, pol)) {
+                       *err = -EINVAL;
+                       x = NULL;
+               }
        } else {
+               if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET)
+                       error = -EINVAL;
+
                *err = acquire_in_progress ? -EAGAIN : error;
        }
        rcu_read_unlock();
(END)


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

Powered by blists - more mailing lists