Re: [REGRESSION] Discussion on "xfrm: Duplicate SPI Handling"

[Antony Antony <antony.antony@secunet.com>]

[-- Attachment #1: Type: text/plain, Size: 8761 bytes --]

Hi,
I looked into this. I feel a simple solution is use x->dir as Nathan
proposed.
When dir is not set we get  pre
commit 94f39804d891 ("xfrm: Duplicate SPI Handling") behaviour.

When XFRM_SA_DIR is set alloc_spi() returns per direction unique spi. 

Another benfit is, this would also keep PF_KEY use case as it was before that comit.
Here is simple RFC patch attached.

How does this look?

strongswan 6.0.0, from Dec 2024, sets x->dir. 
Aakash would this work for for marvell? 

regads,
-antony

On Fri, Mar 27, 2026 at 17:05:13 -0700, Yan Yan wrote:
>    Hi all,
>    I wanted to send a friendly ping to see if we are aligning on making
>    the strict global SPI uniqueness requirement optional, perhaps via a
>    toggle or by leveraging the XFRM_SA_DIR attribute as previously
>    discussed.
>    Are there any other questions or concerns regarding this approach, or
>    anything else we should clarify to ensure backward compatibility while
>    meeting the needs of modern standards?
>    Best,
>    Yan
> 
>    On Tue, Feb 24, 2026 at 3:53 PM Nathan Harold <[1]nharold@google.com>
>    wrote:
> 
>      > That should still be allowed when using the intended APIs (i.e.
>      ALLOCSPI
>      > for the inbound and NEWSA for the outbound SA).  ALLOCSPI might
>      enforce
>      > a unique SPI without considering the address, as that's intended
>      for
>      > local, inbound SAs, where the kernel has full control (looking at
>      the
>      > the patch, it's certainly not ideal, as it goes through all
>      installed
>      > SAs to find a duplicate and it prevents an inbound SPI that
>      matches an
>      > existing outbound SPI - I guess that could be resolved by using
>      separate
>      > tables for in- and outbound SAs).  But that must not prevent
>      installing
>      > outbound SAs with the same SPI to another peer using NEWSA, which
>      still
>      > uses a hash that includes the destination address (that must
>      always be
>      > the case because peers are free to allocate whatever SPI they
>      want).
>      Agreed that there are some unfortunate limitations with the current
>      patch. Keying off the inclusion of XFRM_SA_DIR would resolve the
>      issue
>      you noted (conflating inbound and outbound SPIs) and function as an
>      opt-in for this enforcement. Whatever the mechanism though, the new
>      behavior should be opt-in rather than opt-out in order to maintain
>      backwards compatibility.
>      > In my opinion, you are using the API incorrectly...  I also
>      > don't think there are any benefits in that "consistent larval
>      lifecycle"
>      > (if you found any, please let us know).
>      The Android architecture is multi-tenant and allows userspace apps
>      to
>      establish SAs. At the time we designed it, this felt like the
>      cleanest
>      way to facilitate leak-free resource management because the chain of
>      associations between kernel resources could be symmetrical (and
>      managing them was already quite complicated). Mea culpa (Nathan).
>      But,
>      correctly or not, it has/had worked for many years.
>      > By the way, are you using the min/max option for inbound SAs as
>      well,
>      > with an SPI generated in userland?  That would seem like a
>      violation of
>      > the intention of the API as well (i.e. letting the kernel control
>      the
>      > local SPIs).
>      We provide following two Android APIs for app developers:
>      [2]https://developer.android.com/reference/android/net/IpSecManager#
>      allocateSecurityParameterIndex(java.net.InetAddress)
>      [3]https://developer.android.com/reference/android/net/IpSecManager#
>      allocateSecurityParameterIndex(java.net.InetAddress,%20int)
>      Indeed, allocateSecurityParameterIndex is direction-agnostic; both
>      overloads are implemented internally by including the min and max
>      values in ALLOCSPI. For the variant where app developers provide a
>      specific SPI (which is also useful in testing), Android simply sets
>      both the min and max parameters to that exact value. Our
>      understanding
>      of #xfrm_alloc_spi  is that min/max are required for ALLOCSPI, and
>      otherwise ENOENT will be returned.
>      Note that we also use the DADDR as a mandatory part of the tuple
>      because of the issue mentioned above: SPIs are only unique in
>      conjunction with a DADDR, regardless of direction, and accordingly,
>      that’s how Android is expecting the uniqueness requirement be
>      enforced. In this way, 5 duplicate SPIs can be used on 5 unique IP
>      addresses on the same machine; therefore, a strict "SPI only"
>      interpretation for ALLOCSPI (or SPI handling in general) is curious.
>      We feel that ALLOCSPI should really enforce the same uniqueness
>      requirements as the SAD.
>      Best,
>      Nathan and Yan
>      -Nathan
>      On Wed, Feb 18, 2026 at 12:42 AM Tobias Brunner
>      <[4]tobias@strongswan.org> wrote:
>      >
>      > Hi Yan,
>      >
>      > > For every inbound SA, we allocate SPIs before negotiation. For
>      > > outbound SAs, we allocate SPIs once requested by the peer. We
>      only
>      > > require the (SPI, destination address) combo to be unique. Thus,
>      we
>      > > may have an inbound and outbound SA sharing an SPI with
>      different
>      > > destinations, or multiple outbound SAs to different peers
>      sharing an
>      > > SPI.
>      >
>      > That should still be allowed when using the intended APIs (i.e.
>      ALLOCSPI
>      > for the inbound and NEWSA for the outbound SA).  ALLOCSPI might
>      enforce
>      > a unique SPI without considering the address, as that's intended
>      for
>      > local, inbound SAs, where the kernel has full control (looking at
>      the
>      > the patch, it's certainly not ideal, as it goes through all
>      installed
>      > SAs to find a duplicate and it prevents an inbound SPI that
>      matches an
>      > existing outbound SPI - I guess that could be resolved by using
>      separate
>      > tables for in- and outbound SAs).  But that must not prevent
>      installing
>      > outbound SAs with the same SPI to another peer using NEWSA, which
>      still
>      > uses a hash that includes the destination address (that must
>      always be
>      > the case because peers are free to allocate whatever SPI they
>      want).
>      >
>      > >> If so, why would you use ALLOCSPI and not just install the
>      outbound SA? Is it to avoid differences for in- and outbound SAs
>      (ALLOCSPI+UPDSA vs. NEWSA)?"
>      > >
>      > > Exactly—it is primarily for code symmetry. By using ALLOCSPI +
>      UPDSA
>      > > for both directions, we maintain a consistent larval lifecycle
>      and
>      > > make it easier to maintain.
>      >
>      > In my opinion, you are using the API incorrectly.  ALLOCSPI is
>      intended
>      > to allocate a free local SPI for an inbound SA.  That is, reserve
>      it
>      > before and while the details of the SA are negotiated with the
>      peer
>      > using IKE.  This step isn't necessary for outbound SAs and forcing
>      such
>      > an allocation, after all the details are known, to the responder's
>      SPI
>      > (which I assume you do via min/max option) doesn't feel right.  I
>      also
>      > don't think there are any benefits in that "consistent larval
>      lifecycle"
>      > (if you found any, please let us know).  And the difference
>      between
>      > UPDSA and NEWSA is the nlmsg_type (there are some attributes that
>      are
>      > different for in- and outbound SAs, especially if you set the
>      direction
>      > in newer kernels, but that's the case regardless of the message
>      type).
>      >
>      > By the way, are you using the min/max option for inbound SAs as
>      well,
>      > with an SPI generated in userland?  That would seem like a
>      violation of
>      > the intention of the API as well (i.e. letting the kernel control
>      the
>      > local SPIs).
>      >
>      > As the XFRM API basically mirrors PF_KEYv2 here, you can find more
>      about
>      > the two ways to install SAs in RFC 2367 (SADB_GETSPI/UPDATE vs.
>      SADB_ADD).
>      >
>      > Regards,
>      > Tobias
>      >
> 
>    --
> 
>    --
>    Best,
>    Yan
> 
> References
> 
>    1. mailto:nharold@google.com
>    2. https://developer.android.com/reference/android/net/IpSecManager#allocateSecurityParameterIndex(java.net.InetAddress)
>    3. https://developer.android.com/reference/android/net/IpSecManager#allocateSecurityParameterIndex(java.net.InetAddress, int)
>    4. mailto:tobias@strongswan.org

[-- Attachment #2: 0001-xfrm-allow-same-SPI-value-for-inbound-and-outbound-S.patch --]
[-- Type: text/x-diff, Size: 3548 bytes --]

From b712c3ff7d586c8ed62005e6a37ae01c4a771ce8 Mon Sep 17 00:00:00 2001
From: Antony Antony <antony.antony@secunet.com>
Date: Wed, 25 Mar 2026 06:02:07 +0100
Subject: [PATCH] xfrm: allow same SPI value for inbound and outbound SAs

Commit 94f39804d891 ("xfrm: Duplicate SPI Handling") introduced
xfrm_state_lookup_spi_proto() to fix duplicate SPI allocation for
inbound SAs with different destination addresses.  It enforces global
uniqueness by (spi, proto) across all states regardless of direction.

When x->dir is set, use xfrm_state_lookup_spi_proto() with a strict
per-direction match so that IN and OUT SAs can share a SPI value.

When x->dir is not set, also legacy states created via PF_KEY or without
direction, restore pre-94f39804d891 behavior and check uniqueness by
(daddr, spi, proto) via xfrm_state_lookup(). Extract the mark from
x->mark as required by xfrm_state_lookup().

Move the x->dir assignment in xfrm_alloc_userspi() to before
xfrm_alloc_spi() lookup using dir when it is set.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 net/xfrm/xfrm_state.c | 14 ++++++++++++--
 net/xfrm/xfrm_user.c  |  6 +++---
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 98b362d51836..1cc4a06f9163 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1698,7 +1698,9 @@ struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
 }
 EXPORT_SYMBOL(xfrm_state_lookup_byspi);
 
-static struct xfrm_state *xfrm_state_lookup_spi_proto(struct net *net, __be32 spi, u8 proto)
+static struct xfrm_state *xfrm_state_lookup_spi_proto(struct net *net,
+						      __be32 spi, u8 proto,
+						      u8 dir)
 {
 	struct xfrm_state *x;
 	unsigned int i;
@@ -1707,6 +1709,8 @@ static struct xfrm_state *xfrm_state_lookup_spi_proto(struct net *net, __be32 sp
 	for (i = 0; i <= net->xfrm.state_hmask; i++) {
 		hlist_for_each_entry_rcu(x, &net->xfrm.state_byspi[i], byspi) {
 			if (x->id.spi == spi && x->id.proto == proto) {
+				if (x->dir != dir)
+					continue;
 				if (!xfrm_state_hold_rcu(x))
 					continue;
 				rcu_read_unlock();
@@ -2577,6 +2581,7 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 	struct xfrm_state *x0;
 	int err = -ENOENT;
 	u32 range = high - low + 1;
+	u32 mark = x->mark.v & x->mark.m;
 	__be32 newspi = 0;
 
 	spin_lock_bh(&x->lock);
@@ -2598,7 +2603,12 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 		newspi = htonl(spi);
 
 		spin_lock_bh(&net->xfrm.xfrm_state_lock);
-		x0 = xfrm_state_lookup_spi_proto(net, newspi, x->id.proto);
+		if (x->dir)
+			x0 = xfrm_state_lookup_spi_proto(net, newspi,
+							 x->id.proto, x->dir);
+		else
+			x0 = xfrm_state_lookup(net, mark, &x->id.daddr, newspi,
+					       x->id.proto, x->props.family);
 		if (!x0) {
 			x->id.spi = newspi;
 			h = xfrm_spi_hash(net, &x->id.daddr, newspi, x->id.proto, x->props.family);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 403b5ecac2c5..98b90c747aad 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1873,13 +1873,13 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out_noput;
 	}
 
+	if (attrs[XFRMA_SA_DIR])
+		x->dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
+
 	err = xfrm_alloc_spi(x, p->min, p->max, extack);
 	if (err)
 		goto out;
 
-	if (attrs[XFRMA_SA_DIR])
-		x->dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
-
 	resp_skb = xfrm_state_netlink(skb, x, nlh->nlmsg_seq);
 	if (IS_ERR(resp_skb)) {
 		err = PTR_ERR(resp_skb);
-- 
2.39.5