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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3YuVcj5uNRHS7Ek@unreal>
Date:   Thu, 17 Nov 2022 14:51:33 +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 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>
> > 
> > @@ -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