Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert <steffen.klassert@secunet.com>]

On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > 
> > > 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;
> > 
> > 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.

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