[PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[Antony Antony]

This patch introduces the 'dir' attribute, 'in' or 'out', to the
xfrm_state, SA, enhancing usability by delineating the scope of values
based on direction. An input SA will now exclusively encompass values
pertinent to input, effectively segregating them from output-related
values. This change aims to streamline the configuration process and
improve the overall clarity of SA attributes.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v3->v4:
 - improve HW OFFLOAD DIR check check other direction

v2->v3:
 - delete redundant XFRM_SA_DIR_USET
 - use u8 for "dir"
 - fix HW OFFLOAD DIR check

v1->v2:
 - use .strict_start_type in struct nla_policy xfrma_policy
 - delete redundant XFRM_SA_DIR_MAX enum
---
 include/net/xfrm.h        |  1 +
 include/uapi/linux/xfrm.h |  6 +++++
 net/xfrm/xfrm_compat.c    |  7 ++++--
 net/xfrm/xfrm_device.c    |  6 +++++
 net/xfrm/xfrm_state.c     |  1 +
 net/xfrm/xfrm_user.c      | 46 +++++++++++++++++++++++++++++++++++----
 6 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 1d107241b901..9ff8a0e0f477 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -289,6 +289,7 @@ struct xfrm_state {
 	/* Private data of this transformer, format is opaque,
 	 * interpreted by xfrm_type methods. */
 	void			*data;
+	u8			dir;
 };

 static inline struct net *xs_net(struct xfrm_state *x)
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 6a77328be114..18ceaba8486e 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -141,6 +141,11 @@ enum {
 	XFRM_POLICY_MAX	= 3
 };

+enum xfrm_sa_dir {
+	XFRM_SA_DIR_IN	= 1,
+	XFRM_SA_DIR_OUT = 2
+};
+
 enum {
 	XFRM_SHARE_ANY,		/* No limitations */
 	XFRM_SHARE_SESSION,	/* For this session only */
@@ -315,6 +320,7 @@ enum xfrm_attr_type_t {
 	XFRMA_SET_MARK_MASK,	/* __u32 */
 	XFRMA_IF_ID,		/* __u32 */
 	XFRMA_MTIMER_THRESH,	/* __u32 in seconds for input SA */
+	XFRMA_SA_DIR,		/* __u8 */
 	__XFRMA_MAX

 #define XFRMA_OUTPUT_MARK XFRMA_SET_MARK	/* Compatibility */
diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
index 655fe4ff8621..007dee03b1bc 100644
--- a/net/xfrm/xfrm_compat.c
+++ b/net/xfrm/xfrm_compat.c
@@ -98,6 +98,7 @@ static const int compat_msg_min[XFRM_NR_MSGTYPES] = {
 };

 static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
+	[XFRMA_UNSPEC]          = { .strict_start_type = XFRMA_SA_DIR },
 	[XFRMA_SA]		= { .len = XMSGSIZE(compat_xfrm_usersa_info)},
 	[XFRMA_POLICY]		= { .len = XMSGSIZE(compat_xfrm_userpolicy_info)},
 	[XFRMA_LASTUSED]	= { .type = NLA_U64},
@@ -129,6 +130,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
 	[XFRMA_SET_MARK_MASK]	= { .type = NLA_U32 },
 	[XFRMA_IF_ID]		= { .type = NLA_U32 },
 	[XFRMA_MTIMER_THRESH]	= { .type = NLA_U32 },
+	[XFRMA_SA_DIR]          = { .type = NLA_U8}
 };

 static struct nlmsghdr *xfrm_nlmsg_put_compat(struct sk_buff *skb,
@@ -277,9 +279,10 @@ static int xfrm_xlate64_attr(struct sk_buff *dst, const struct nlattr *src)
 	case XFRMA_SET_MARK_MASK:
 	case XFRMA_IF_ID:
 	case XFRMA_MTIMER_THRESH:
+	case XFRMA_SA_DIR:
 		return xfrm_nla_cpy(dst, src, nla_len(src));
 	default:
-		BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
+		BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
 		pr_warn_once("unsupported nla_type %d\n", src->nla_type);
 		return -EOPNOTSUPP;
 	}
@@ -434,7 +437,7 @@ static int xfrm_xlate32_attr(void *dst, const struct nlattr *nla,
 	int err;

 	if (type > XFRMA_MAX) {
-		BUILD_BUG_ON(XFRMA_MAX != XFRMA_MTIMER_THRESH);
+		BUILD_BUG_ON(XFRMA_MAX != XFRMA_SA_DIR);
 		NL_SET_ERR_MSG(extack, "Bad attribute");
 		return -EOPNOTSUPP;
 	}
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 3784534c9185..1d1b1494b71f 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -253,6 +253,12 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		return -EINVAL;
 	}

+	if ((xuo->flags & XFRM_OFFLOAD_INBOUND && x->dir == XFRM_SA_DIR_OUT) ||
+	    (!(xuo->flags & XFRM_OFFLOAD_INBOUND) && x->dir == XFRM_SA_DIR_IN)) {
+		NL_SET_ERR_MSG(extack, "Mismatched SA and offload direction");
+		return -EINVAL;
+	}
+
 	is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;

 	/* We don't yet support UDP encapsulation and TFC padding. */
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index bda5327bf34d..0d6f5a49002f 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1744,6 +1744,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
 	x->lastused = orig->lastused;
 	x->new_mapping = 0;
 	x->new_mapping_sport = 0;
+	x->dir = orig->dir;

 	return x;

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index ad01997c3aa9..e2b734c6eb3d 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -360,6 +360,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 		}
 	}

+	if (attrs[XFRMA_SA_DIR]) {
+		u8 sa_dir = nla_get_u8(attrs[XFRMA_SA_DIR]);
+
+		if (sa_dir != XFRM_SA_DIR_IN && sa_dir != XFRM_SA_DIR_OUT)  {
+			NL_SET_ERR_MSG(extack, "XFRMA_SA_DIR attribute is out of range");
+			err = -EINVAL;
+			goto out;
+		}
+	}
+
 out:
 	return err;
 }
@@ -627,6 +637,7 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
 	struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
 	struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
 	struct nlattr *mt = attrs[XFRMA_MTIMER_THRESH];
+	struct nlattr *dir = attrs[XFRMA_SA_DIR];

 	if (re && x->replay_esn && x->preplay_esn) {
 		struct xfrm_replay_state_esn *replay_esn;
@@ -661,6 +672,9 @@ static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,

 	if (mt)
 		x->mapping_maxage = nla_get_u32(mt);
+
+	if (dir)
+		x->dir = nla_get_u8(dir);
 }

 static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
@@ -1182,8 +1196,13 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 		if (ret)
 			goto out;
 	}
-	if (x->mapping_maxage)
+	if (x->mapping_maxage) {
 		ret = nla_put_u32(skb, XFRMA_MTIMER_THRESH, x->mapping_maxage);
+		if (ret)
+			goto out;
+	}
+	if (x->dir)
+		ret = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
 out:
 	return ret;
 }
@@ -2399,7 +2418,8 @@ static inline unsigned int xfrm_aevent_msgsize(struct xfrm_state *x)
 	       + nla_total_size_64bit(sizeof(struct xfrm_lifetime_cur))
 	       + nla_total_size(sizeof(struct xfrm_mark))
 	       + nla_total_size(4) /* XFRM_AE_RTHR */
-	       + nla_total_size(4); /* XFRM_AE_ETHR */
+	       + nla_total_size(4) /* XFRM_AE_ETHR */
+	       + nla_total_size(sizeof(x->dir)); /* XFRMA_SA_DIR */
 }

 static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
@@ -2456,6 +2476,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
 	if (err)
 		goto out_cancel;

+	if (x->dir) {
+		err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
+		if (err)
+			goto out_cancel;
+	}
+
 	nlmsg_end(skb, nlh);
 	return 0;

@@ -3015,6 +3041,7 @@ EXPORT_SYMBOL_GPL(xfrm_msg_min);
 #undef XMSGSIZE

 const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
+	[XFRMA_UNSPEC]		= { .strict_start_type = XFRMA_SA_DIR },
 	[XFRMA_SA]		= { .len = sizeof(struct xfrm_usersa_info)},
 	[XFRMA_POLICY]		= { .len = sizeof(struct xfrm_userpolicy_info)},
 	[XFRMA_LASTUSED]	= { .type = NLA_U64},
@@ -3046,6 +3073,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
 	[XFRMA_SET_MARK_MASK]	= { .type = NLA_U32 },
 	[XFRMA_IF_ID]		= { .type = NLA_U32 },
 	[XFRMA_MTIMER_THRESH]   = { .type = NLA_U32 },
+	[XFRMA_SA_DIR]          = { .type = NLA_U8 }
 };
 EXPORT_SYMBOL_GPL(xfrma_policy);

@@ -3186,8 +3214,9 @@ static void xfrm_netlink_rcv(struct sk_buff *skb)

 static inline unsigned int xfrm_expire_msgsize(void)
 {
-	return NLMSG_ALIGN(sizeof(struct xfrm_user_expire))
-	       + nla_total_size(sizeof(struct xfrm_mark));
+	return NLMSG_ALIGN(sizeof(struct xfrm_user_expire)) +
+	       nla_total_size(sizeof(struct xfrm_mark)) +
+	       nla_total_size(sizeof_field(struct xfrm_state, dir));
 }

 static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct km_event *c)
@@ -3214,6 +3243,12 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
 	if (err)
 		return err;

+	if (x->dir) {
+		err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
+		if (err)
+			return err;
+	}
+
 	nlmsg_end(skb, nlh);
 	return 0;
 }
@@ -3321,6 +3356,9 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x)
 	if (x->mapping_maxage)
 		l += nla_total_size(sizeof(x->mapping_maxage));

+	if (x->dir)
+		l += nla_total_size(sizeof(x->dir));
+
 	return l;
 }

--
2.30.2

Re: [PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[Leon Romanovsky]

On Wed, Mar 13, 2024 at 10:04:51PM +0100, Antony Antony wrote:
> This patch introduces the 'dir' attribute, 'in' or 'out', to the
> xfrm_state, SA, enhancing usability by delineating the scope of values
> based on direction. An input SA will now exclusively encompass values
> pertinent to input, effectively segregating them from output-related
> values. This change aims to streamline the configuration process and
> improve the overall clarity of SA attributes.
> 
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> v3->v4:
>  - improve HW OFFLOAD DIR check check other direction
> 
> v2->v3:
>  - delete redundant XFRM_SA_DIR_USET
>  - use u8 for "dir"
>  - fix HW OFFLOAD DIR check
> 
> v1->v2:
>  - use .strict_start_type in struct nla_policy xfrma_policy
>  - delete redundant XFRM_SA_DIR_MAX enum
> ---
>  include/net/xfrm.h        |  1 +
>  include/uapi/linux/xfrm.h |  6 +++++
>  net/xfrm/xfrm_compat.c    |  7 ++++--
>  net/xfrm/xfrm_device.c    |  6 +++++
>  net/xfrm/xfrm_state.c     |  1 +
>  net/xfrm/xfrm_user.c      | 46 +++++++++++++++++++++++++++++++++++----
>  6 files changed, 61 insertions(+), 6 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [devel-ipsec] [PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[Nicolas Dichtel]

Le 13/03/2024 à 22:04, Antony Antony via Devel a écrit :
> This patch introduces the 'dir' attribute, 'in' or 'out', to the
> xfrm_state, SA, enhancing usability by delineating the scope of values
> based on direction. An input SA will now exclusively encompass values
> pertinent to input, effectively segregating them from output-related
> values. This change aims to streamline the configuration process and
> improve the overall clarity of SA attributes.

If I correctly understand the commit, the direction is ignored if there is no
offload configured, ie an output SA could be used in input. Am I right?

If yes:
 1/ it would be nice to state it explicitly in the commit log.
 2/ it is confusing for users not using offload.

Regards,
Nicolas

> 
> Signed-off-by: Antony Antony <antony.antony@secunet.com>

[PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[Antony Antony]

Hi Nicolas,

On Thu, Mar 14, 2024 at 03:28:57PM +0100, Nicolas Dichtel via Devel wrote:
> Le 13/03/2024 à 22:04, Antony Antony via Devel a écrit :
> > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > xfrm_state, SA, enhancing usability by delineating the scope of values
> > based on direction. An input SA will now exclusively encompass values
> > pertinent to input, effectively segregating them from output-related
> > values. This change aims to streamline the configuration process and
> > improve the overall clarity of SA attributes.
> 
> If I correctly understand the commit, the direction is ignored if there is no
> offload configured, ie an output SA could be used in input. Am I right?
> 
> If yes:
>  1/ it would be nice to state it explicitly in the commit log.
>  2/ it is confusing for users not using offload.


I see why you're asking for clarification. This patch is designed for 
broader use in the future beyond its current application, specifically for 
the HW offload use case. Notably, the upcoming IP-TFS patch, among others, 
will utilize the 'direction' (dir) attribute. The absence of a 'direction' 
for an SA can lead to a confusing user experience. While symmetry is nice, 
configuring values that are not utilized and are direction-specific can be 
very confusing. For instance, many users configure a replay window 
(specifically without ESN) on an outbound SA, even though the replay window 
is only applicable to an inbound SA. With ESN, you can just leave it at 1.  
SAs have historically lacked a direction attribute. It has been brought up 
many times but never implemented.

Following the email I shared earlier 
(https://lore.kernel.org/netdev/ZV0BSBzNh3UIqueZ@Antony2201.local/), I 
discussed this proposal with more users/developers, and there is interest in 
adding a direction to SA for future values. Maybe I will addd one line in 
the message this is in preperartion for upcoming IP-TFS.

-antony

Re: [PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[Nicolas Dichtel]

Le 17/03/2024 à 23:34, Antony Antony a écrit :
> Hi Nicolas,
> 
> On Thu, Mar 14, 2024 at 03:28:57PM +0100, Nicolas Dichtel via Devel wrote:
>> Le 13/03/2024 à 22:04, Antony Antony via Devel a écrit :
>>> This patch introduces the 'dir' attribute, 'in' or 'out', to the
>>> xfrm_state, SA, enhancing usability by delineating the scope of values
>>> based on direction. An input SA will now exclusively encompass values
>>> pertinent to input, effectively segregating them from output-related
>>> values. This change aims to streamline the configuration process and
>>> improve the overall clarity of SA attributes.
>>
>> If I correctly understand the commit, the direction is ignored if there is no
>> offload configured, ie an output SA could be used in input. Am I right?
>>
>> If yes:
>>  1/ it would be nice to state it explicitly in the commit log.
>>  2/ it is confusing for users not using offload.
> 
> 
> I see why you're asking for clarification. This patch is designed for 
> broader use in the future beyond its current application, specifically for 
> the HW offload use case. Notably, the upcoming IP-TFS patch, among others, 
> will utilize the 'direction' (dir) attribute. The absence of a 'direction' 
> for an SA can lead to a confusing user experience. While symmetry is nice,
Thanks for the explanation.

> configuring values that are not utilized and are direction-specific can be 
> very confusing. For instance, many users configure a replay window 
> (specifically without ESN) on an outbound SA, even though the replay window 
> is only applicable to an inbound SA. With ESN, you can just leave it at 1.  
> SAs have historically lacked a direction attribute. It has been brought up 
> many times but never implemented.
Maybe it would be worse to reject this new XFRMA_SA_DIR attribute when it is not
used (for example if there is no offload configured). It will make the API clearer.


Regards,
Nicolas

> 
> Following the email I shared earlier 
> (https://lore.kernel.org/netdev/ZV0BSBzNh3UIqueZ@Antony2201.local/), I 
> discussed this proposal with more users/developers, and there is interest in 
> adding a direction to SA for future values. Maybe I will addd one line in 
> the message this is in preperartion for upcoming IP-TFS.
> 
> -antony

Re: [PATCH ipsec-next v4] xfrm: Add Direction to the SA in or out

[Antony Antony]

On Fri, Mar 22, 2024 at 09:20:05AM +0100, Nicolas Dichtel wrote:
> Le 17/03/2024 à 23:34, Antony Antony a écrit :
> > Hi Nicolas,
> > 
> > On Thu, Mar 14, 2024 at 03:28:57PM +0100, Nicolas Dichtel via Devel wrote:
> >> Le 13/03/2024 à 22:04, Antony Antony via Devel a écrit :
> >>> This patch introduces the 'dir' attribute, 'in' or 'out', to the
> >>> xfrm_state, SA, enhancing usability by delineating the scope of values
> >>> based on direction. An input SA will now exclusively encompass values
> >>> pertinent to input, effectively segregating them from output-related
> >>> values. This change aims to streamline the configuration process and
> >>> improve the overall clarity of SA attributes.
> >>
> >> If I correctly understand the commit, the direction is ignored if there is no
> >> offload configured, ie an output SA could be used in input. Am I right?
> >>
> >> If yes:
> >>  1/ it would be nice to state it explicitly in the commit log.
> >>  2/ it is confusing for users not using offload.
> > 
> > 
> > I see why you're asking for clarification. This patch is designed for 
> > broader use in the future beyond its current application, specifically for 
> > the HW offload use case. Notably, the upcoming IP-TFS patch, among others, 
> > will utilize the 'direction' (dir) attribute. The absence of a 'direction' 
> > for an SA can lead to a confusing user experience. While symmetry is nice,
> Thanks for the explanation.
> 
> > configuring values that are not utilized and are direction-specific can be 
> > very confusing. For instance, many users configure a replay window 
> > (specifically without ESN) on an outbound SA, even though the replay window 
> > is only applicable to an inbound SA. With ESN, you can just leave it at 1.  
> > SAs have historically lacked a direction attribute. It has been brought up 
> > many times but never implemented.
> Maybe it would be worse to reject this new XFRMA_SA_DIR attribute when it is not
> used (for example if there is no offload configured). It will make the API 
> clearer.

there was also interst to use "dir"  for informational only.  It could be 
useful to check incoming traffic on large systems with 100s of xfrm states.
So I keep the "dir"  open for any  xfrm state. 

-antony