Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion

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

On Wed, Oct 25, 2017 at 01:09:44PM +0000, Aviv Heller wrote:
> -----Original message-----
> > From: Steffen Klassert
> > Sent: Wednesday, October 25 2017, 10:22 am
> > To: avivh@mellanox.com
> > Cc: Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion
> > 
> > On Tue, Oct 24, 2017 at 06:10:30PM +0300, avivh@mellanox.com wrote:
> > > From: Aviv Heller <avivh@mellanox.com>
> > > 
> > > Adding the state to the offload device prior to replay init in
> > > xfrm_state_construct() will result in NULL dereference if a matching
> > > ESP packet is received in between.
> > > 
> > > Adding it after insertion also has the benefit of the driver not having
> > > to check whether a state with the same match criteria already exists,
> > > but forces us to use an atomic type for the offload_handle, to make
> > > certain a stack-read/driver-write race won't result in reading corrupt
> > > data.
> > 
> > No, this will add multiple atomic operations to the packet path,
> > even in the non offloaded case.
> > 
> > I think the problem is that we set XFRM_STATE_VALID to early.
> > This was not a problem before we had offloading because
> > it was not possible to lookup this state before we inserted
> > it into the SADB. Now that the driver holds a subset of states
> > too, we need to make sure the state is fully initialized
> > before we mark it as valid.
> > 
> > The patch below should do it, in combination with your patch 1/3.
> > 
> > Could you please test this?
> > 
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index b997f13..96eb263 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -587,10 +587,6 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
> >  	if (attrs[XFRMA_OUTPUT_MARK])
> >  		x->props.output_mark = nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
> >  
> > -	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
> > -	if (err)
> > -		goto error;
> > -
> >  	if (attrs[XFRMA_SEC_CTX]) {
> >  		err = security_xfrm_state_alloc(x,
> >  						nla_data(attrs[XFRMA_SEC_CTX]));
> > @@ -620,6 +616,10 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
> >  	/* override default values from above */
> >  	xfrm_update_ae_params(x, attrs, 0);
> >  
> > +	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
> > +	if (err)
> > +		goto error;
> > +
> >  	return x;
> >  
> >  error:
> > 
> 
> Hi Steffen,
> 
> This patch does not work, due to:
> 	if (!x->type_offload)
> 		return -EINVAL;
> 
> test in xfrm_dev_state_add().

There is certainly a way arround that :)
The easiest I can think of would be to propagate XFRM_STATE_VALID
only after the state is inserted into the SADBs. I.e. move the
setting of XFRM_STATE_VALID out of __xfrm_init_state() and let the
callers do it.

> 
> I agree with your analysis, and that we take a little performance hit due to the atomics, but we get the benefit of calling xfrm_dev_state_add() after the state is completely initialized, and passed the criteria for addition by xfrm_state_add().

We already have too many of these atomic operatons in the
packet path, adding more is a no go.