Re: [PATCH 11/14] xfrm: include mark in MIGRATE_STATE SA collision check

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

On Tue, Nov 03, 2026 at 08:05:05AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] MIGRATE_STATE checks for a collision with one key and installs
>   with another, so for non-SPI protocols (e.g. IPPROTO_IPIP) the…
> - [Medium] The new xfrm_state_mark_collides() checks for collisions
>   without the state lock.
> 
> Pre-existing issues:
> - [Low] Discarded: the issue predates this patch and a later patch in
>   this series fixes it.
> 
> --
> 
> xfrm: include mark in MIGRATE_STATE SA collision check
> 
> This commit makes XFRM_MSG_MIGRATE_STATE take the SA mark into account
> when it checks the new tuple for collisions. It adds
> xfrm_state_mark_collides(), which walks the SPI hash bucket for the target
> tuple and skips the state being migrated. It also makes
> xfrm_state_migrate_install() use xfrm_state_add() instead of
> xfrm_state_insert() when the mark changes.
> 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index f1e04df8d5011..65663c7ba836a 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -2202,10 +2202,12 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
> >  			       struct netlink_ext_ack *extack)
> >  {
> >  	if (m->new_family == m->old_family &&
> > -	    xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
> > +	    xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) &&
> > +	    xc->mark.v == x->mark.v && xc->mark.m == x->mark.m) {
> 
> [Severity: Medium]
> 
> With the added mark comparison, a migration that changes the mark but
> keeps the same daddr now goes through xfrm_state_add() instead of
> xfrm_state_insert(). For protocols that don't use the SPI for lookup, can
> this fail after the original SA has already been deleted?
> 
> xfrm_do_migrate_state() doesn't restrict um->id.proto. The pre-check in
> xfrm_state_mark_collides() always walks the byspi bucket for (new_daddr,
> spi, proto, family).
> 
> For IPPROTO_IPIP, use_spi is false in xfrm_state_add(), so
> __xfrm_state_locate() calls __xfrm_state_lookup_byaddr() with (daddr,
> saddr, proto, mark).
> 
> IPIP states created by ipcomp_tunnel_create() have a non-zero SPI:
> 
>     t->id.spi = x->props.saddr.a4;
> 
> That puts them in byspi, so xfrm_state_lookup_exact() can find them. Take
> these two states:
> 
>   state A: IPIP, src SA, dst D, spi SA, mark 1/0xff
>   state B: IPIP, src SB, dst D, spi SB, mark 2/0xff
> 
> and migrate A with new_saddr=SB, new_daddr=D and XFRMA_MARK=2/0xff:
> 
> xfrm_do_migrate_state()
>   xfrm_state_mark_collides()     /* only the (D, spi SA) bucket, misses B */
>   xfrm_state_migrate_create()    /* ipip_init_state() accepts the clone */
>   __xfrm_state_delete(x)         /* A is unhashed */
>   xfrm_state_migrate_install()
>     xfrm_state_add(xc)
>       __xfrm_state_locate()      /* use_spi == 0 */
>         __xfrm_state_lookup_byaddr()   /* finds B */
>       returns -EEXIST
> 
> A is gone, the clone is dropped, and userspace gets -EEXIST. That case
> contradicts the "Should not occur" comment in xfrm_do_migrate_state(). It
> also contradicts the series documentation, which says the old SA stays
> intact and the operation is safe to retry.
> 
> Should the pre-check use the same key that xfrm_state_add() uses for
> non-SPI protocols? Alternatively, should MIGRATE_STATE reject protocols
> that don't use the SPI for lookup?

Both is possible, I'd leave that to Antony for a followup patch.

> 
> >  		/*
> > -		 * Care is needed when the destination address of the state is
> > -		 * to be updated as it is a part of triplet.
> > +		 * Care is needed when the destination address or mark of the
> > +		 * state is to be updated, as they are part of the lookup
> > +		 * triplet.
> >  		 */
> >  		xfrm_state_insert(xc);
> >  	} else {
> > @@ -2440,6 +2442,38 @@ xfrm_state_lookup_exact(struct net *net, const struct xfrm_mark *mark,
> >  }
> >  EXPORT_SYMBOL(xfrm_state_lookup_exact);
> >  
> > +/* True if some OTHER state at this tuple would wildcard-match "mark".
> > + * Used by MIGRATE_STATE, which must exclude the state being migrated.
> > + */
> > +bool xfrm_state_mark_collides(struct net *net, u32 mark,
> > +			      const xfrm_address_t *daddr, __be32 spi,
> > +			      u8 proto, unsigned short family,
> > +			      const struct xfrm_state *self)
> > +{
> > +	struct xfrm_hash_state_ptrs state_ptrs;
> > +	unsigned int h;
> > +	struct xfrm_state *x;
> > +	bool collides = false;
> > +
> > +	rcu_read_lock();
> > +	xfrm_hash_ptrs_get(net, &state_ptrs);
> > +	h = __xfrm_spi_hash(daddr, spi, proto, family, state_ptrs.hmask);
> > +
> > +	hlist_for_each_entry_rcu(x, state_ptrs.byspi + h, byspi) {
> > +		if (x != self && x->props.family == family &&
> > +		    x->id.spi == spi && x->id.proto == proto &&
> > +		    xfrm_addr_equal(&x->id.daddr, daddr, family) &&
> > +		    (mark & x->mark.m) == x->mark.v) {
> > +			collides = true;
> > +			break;
> > +		}
> > +	}
> > +	rcu_read_unlock();
> > +
> > +	return collides;
> 
> [Severity: Medium]
> 
> This walk runs under rcu_read_lock() only. The result is stale as soon as
> the RCU lock is dropped, but xfrm_do_migrate_state() treats it as
> authoritative. The check that actually decides the outcome, in
> xfrm_state_add(), only runs after __xfrm_state_delete(x). Is that
> ordering safe?
> 
> xfrm_hash_ptrs_get() takes a seqcount-protected snapshot of the table
> pointers. The bucket walk itself is not covered by the seqcount, and
> nothing re-checks the generation afterwards.
> 
> The comment in xfrm_do_migrate_state() relies on xfrm_cfg_mutex. At least
> two byspi writers don't take that mutex.
> 
> The first is xfrm_hash_resize(). It runs from a work item and holds only
> xfrm_state_lock and the hash generation seqcount. xfrm_hash_transfer()
> relinks every node into the new tables, so a reader in the old bucket can
> follow a rewritten next pointer and skip the colliding entry.
> 
> The second is xfrm_state_find() on the packet path. When the template has
> a static SPI, it inserts ACQ states into byspi under xfrm_state_lock only:
> 
>     if (x->id.spi) {
>         h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
>         XFRM_STATE_INSERT(byspi, &x->byspi, ...
>     }
> 
> __xfrm_state_lookup() doesn't filter out ACQ states, so xfrm_state_add()
> would hit one that was inserted after the pre-check.
> 
> If either race happens:
> 
> xfrm_do_migrate_state()
>   xfrm_state_mark_collides()      /* returns false */
>   xfrm_state_migrate_create()     /* allocations, xfrm_init_state(), offload */
>   __xfrm_state_delete(x)
>   xfrm_state_migrate_install()
>     xfrm_state_add(xc)            /* returns -EEXIST */
>   xfrm_state_put(xc)
> 
> both the original SA and the clone are lost.
> 
> Before this patch, mark-only migrations used xfrm_state_insert(), which
> can't fail. They now reach this path too. The same lockless pre-check
> followed by a destructive install already existed for daddr and family
> changes.

This looks valid. But as this issue is not new (daddr and family changes
have it already), I think we should fix that all together with a
followup. Antony?

Btw. none of these issues were flaged by sashiko on the original
patchset submittion.