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

[Leon Romanovsky <leon@kernel.org>]

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@nvidia.com>
> > 
> > @@ -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) {
> >  				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.

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.

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

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


> 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.

BTW, IPsec packet offload mode is in use for almost two years already
in real networks.
https://docs.nvidia.com/networking/display/OFEDv521040/Changes+and+New+Features

Thanks