netdev - Re: [PATCH ipsec-next 1/3] xfrm: Add support for per cpu xfrm state handling.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZiDrx7wMV/DQryV+@gauss3.secunet.de>
Date: Thu, 18 Apr 2024 11:45:43 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Tobias Brunner <tobias@...ongswan.org>
CC: <netdev@...r.kernel.org>, <devel@...ux-ipsec.org>, Paul Wouters
	<paul@...ats.ca>, Antony Antony <antony.antony@...unet.com>, Daniel Xu
	<dxu@...uu.xyz>
Subject: Re: [PATCH ipsec-next 1/3] xfrm: Add support for per cpu xfrm state
 handling.

On Fri, Apr 12, 2024 at 12:37:24PM +0200, Tobias Brunner wrote:
> On 12.04.24 08:05, Steffen Klassert wrote:
> >  
> > diff --git a/net/key/af_key.c b/net/key/af_key.c
> > index f79fb99271ed..b9a1eb3ff461 100644
> > --- a/net/key/af_key.c
> > +++ b/net/key/af_key.c
> > @@ -1354,7 +1354,7 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, const struct sadb_
> >  	}
> >  
> >  	if (hdr->sadb_msg_seq) {
> > -		x = xfrm_find_acq_byseq(net, DUMMY_MARK, hdr->sadb_msg_seq);
> > +		x = xfrm_find_acq_byseq(net, DUMMY_MARK, hdr->sadb_msg_seq, 0);
> 
> Shouldn't this be UINT_MAX instead of 0?  (Not sure if it makes a
> difference in practice, but it would be consistent with XFRM.)

Yes, right fixed

> 
> >  		if (x && !xfrm_addr_equal(&x->id.daddr, xdaddr, family)) {
> >  			xfrm_state_put(x);
> >  			x = NULL;
> > @@ -1362,7 +1362,8 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, const struct sadb_
> >  	}
> >  
> >  	if (!x)
> > -		x = xfrm_find_acq(net, &dummy_mark, mode, reqid, 0, proto, xdaddr, xsaddr, 1, family);
> > +		x = xfrm_find_acq(net, &dummy_mark, mode, reqid, 0, 0, proto,
> > +				  xdaddr, xsaddr, 1, family);
> 
> Same as above.

Fixed.

> 
> >  
> >  	if (x == NULL)
> >  		return -ENOENT;
> > @@ -1417,7 +1418,7 @@ static int pfkey_acquire(struct sock *sk, struct sk_buff *skb, const struct sadb
> >  	if (hdr->sadb_msg_seq == 0 || hdr->sadb_msg_errno == 0)
> >  		return 0;
> >  
> > -	x = xfrm_find_acq_byseq(net, DUMMY_MARK, hdr->sadb_msg_seq);
> > +	x = xfrm_find_acq_byseq(net, DUMMY_MARK, hdr->sadb_msg_seq, 0);
> 
> Same as above.

Fixed too.

> >  	return x;
> > @@ -972,6 +973,7 @@ xfrm_init_tempstate(struct xfrm_state *x, const struct flowi *fl,
> >  	x->props.mode = tmpl->mode;
> >  	x->props.reqid = tmpl->reqid;
> >  	x->props.family = tmpl->encap_family;
> > +	x->type_offload = NULL;
> 
> This seems unrelated.  And is it actually necessary?  xfrm_state_alloc()
> uses kmem_cache_zalloc(), so this should be NULL anyway.

Removed.

> > @@ -1333,6 +1350,9 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  			x = NULL;
> >  			error = -ESRCH;
> >  		}
> > +
> > +		if (best)
> > +			x = best;
> 
> Since `x` could be assigned to a (more specific in terms of CPU ID)
> state in state XFRM_STATE_ACQ at this point, it might warrant a comment
> that clarifies why it is unconditionally overridden with `best`.  That
> is, so this other matching "fallback" SA is used for this packet while
> the CPU-specific acquire is handled, which might not be immedialy
> obvious when reading the code.

Good point. I've added a comment.

Thanks for the review!