Re: [PATCH 08/14] xfrm: state: exact mark/mask match for SPI-keyed control-plane SA lookups

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

On Tue, Nov 03, 2026 at 08:05:04AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 4 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The patch changes netlink request semantics without saying so.
> - [Medium] The commit says ALLOCSPI larval states may be unmarked (0/0)
>   and keeps UPDSA on the wildcard match for that reason.
> - [Medium] UPDSA (xfrm_state_update()) still finds its target through
>   __xfrm_state_locate(), which calls the wildcard…
> - [Medium] The commit says it wires exact matching into DELSA/GETSA, but
>   xfrm_user_state_lookup() converts only the SPI branch.

The commit messages could have been more precise, but no need
to revert it because of that.

> [Severity: Medium]
> UPDSA still picks its target through this wildcard lookup. Can
> xfrm_state_update() still update the wrong SA when the states are VALID?
> 
> Overlapping SAs can coexist. The duplicate check in xfrm_state_add()
> also goes through __xfrm_state_locate(), so the result depends on
> insertion order:
> 
>   NEWSA A {mark 5, mask 0xff}
>   NEWSA B {mark 0, mask 0}, same daddr/spi/proto
>     key is 0, (0 & 0xff) != 5, so the add succeeds
> 
> XFRM_STATE_INSERT puts newer software states ahead of older ones in the
> byspi chain, so B sits before A. Then:
> 
>   UPDSA with XFRMA_MARK 5/0xff
>     xfrm_add_sa()->xfrm_state_update()->__xfrm_state_locate()
>       key is 5, B matches first since (5 & 0) == 0
> 
> In the VALID branch, xfrm_state_update() then overwrites B's encap,
> coaddr, lifetimes, smark and if_id, and restarts B's timer. A, the SA
> named in the request, stays unchanged.
> 
> The commit message gives unmarked ALLOCSPI larval states as the reason
> to keep the wildcard here. That reason only covers XFRM_STATE_ACQ
> states.
> 
> Would it work to try an exact match first, and fall back to the wildcard
> only for XFRM_STATE_ACQ larval states?

Might work, can be done with a followup. I leave that to Antony.

> 
> [ ... ]
> 
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index c1571c7a23153..5a9f729b4fcb0 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -1109,11 +1109,12 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
> >  	struct xfrm_state *x = NULL;
> >  	struct xfrm_mark m;
> >  	int err;
> > -	u32 mark = xfrm_mark_get(attrs, &m);
> > +
> > +	xfrm_mark_get(attrs, &m);
> >
> >  	if (xfrm_id_proto_match(p->proto, IPSEC_PROTO_ANY)) {
> >  		err = -ESRCH;
> > -		x = xfrm_state_lookup(net, mark, &p->daddr, p->spi, p->proto, p->family);
> > +		x = xfrm_state_lookup_exact(net, &m, &p->daddr, p->spi, p->proto, p->family);
> 
> [Severity: Medium]
> The commit message says ALLOCSPI larval states may be unmarked (0/0).
> Can DELSA still find those larval states after this change?
> 
> xfrm_alloc_userspi() passes the request's mark to xfrm_find_acq(), and
> __find_acq_core() copies it into the new ACQ state:
> 
> net/xfrm/xfrm_state.c:__find_acq_core() {
>     ...
> 		x->mark.v = m->v;
> 		x->mark.m = m->m;
>     ...
> }
> 
> So an ALLOCSPI without XFRMA_MARK creates a 0/0 larval state that holds
> the allocated SPI.
> 
> Suppose negotiation fails, or the CHILD_SA is torn down before UPDSA. An
> IKE daemon then sends DELSA for that SPI with its configured inbound
> mark, for example 5/0xffffffff. strongSwan behaves this way: its
> ALLOCSPI carries no mark, and its child_sa destroy deletes my_spi with
> mark_in.

If the larval state was allocated without mark, then it should also be
deleted witout mark. Looks more like a strongSwan problem.

> 
> Before this patch the key was 5. Since (5 & 0) == 0, the larval state
> was found and deleted.
> 
> Now xfrm_del_sa()->xfrm_user_state_lookup()->xfrm_state_lookup_exact()
> requires v == 5 && m == 0xffffffff, so DELSA returns -ESRCH. No delete
> notification is sent. The larval state keeps the SPI reserved until its
> acquire hard timer (sysctl_acq_expires) fires.

In the worst case, this SPI is blocked for 30 sec. with a standard
configuration.

> 
> >  	} else {
> >  		xfrm_address_t *saddr = NULL;
> >
> > @@ -1124,7 +1125,7 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
> >  		}
> >
> >  		err = -ESRCH;
> > -		x = xfrm_state_lookup_byaddr(net, mark,
> > +		x = xfrm_state_lookup_byaddr(net, m.v & m.m,
> >  					     &p->daddr, saddr,
> >  					     p->proto, p->family);
> 
> [Severity: Medium]
> The commit message says DELSA/GETSA now use exact matching. This branch
> still does a wildcard lookup and drops the request's mask. Should the
> non-SPI path be converted as well?
> 
> This branch handles protocols where xfrm_id_proto_match(p->proto,
> IPSEC_PROTO_ANY) is false, such as MIPv6 IPPROTO_ROUTING and
> IPPROTO_DSTOPTS states. __xfrm_state_lookup_byaddr() still does:

This is ancient stuff, not even sure if this is still functional.
We should consider to deprecate these.

> 
> 		if ((mark & x->mark.m) != x->mark.v)
> 			continue;
> 
> Overlapping states can coexist here too:
> 
>   NEWSA A {mark 5, mask 0xff}
>   NEWSA B {mark 0, mask 0}, same daddr/saddr/proto
>     duplicate check key is 0, (0 & 0xff) != 5, so the add succeeds
> 
> XFRM_STATE_INSERT puts B ahead of A in the bysrc chain. Then:
> 
>   DELSA with XFRMA_MARK 5/0xff and XFRMA_SRCADDR
>     xfrm_del_sa()->xfrm_user_state_lookup()->xfrm_state_lookup_byaddr()
>       key is 5, B matches first since (5 & 0) == 0
>     xfrm_state_delete(B)
> 
> As a result B is deleted, not the SA named in the request. GETSA returns
> the wrong SA in the same way.

It is a missing path, can be fixed with a followup.