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

[Leon Romanovsky <leon@kernel.org>]

On Sun, Mar 10, 2024 at 09:03:47PM +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.
> 
> v1->v2:

Please try to avoid replying new patch versions as reply-to previous
versions. It breaks the threading and makes it hard to track the patches.

>  - use .strict_start_type in struct nla_policy xfrma_policy
>  - delete redundant XFRM_SA_DIR_MAX enum

Please put changelog after ---, it will allow us to make sure that
commit message will be clean once the patch is applied.

> 
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
>  include/net/xfrm.h        |  1 +
>  include/uapi/linux/xfrm.h |  7 +++++++
>  net/xfrm/xfrm_compat.c    |  7 +++++--
>  net/xfrm/xfrm_device.c    |  5 +++++
>  net/xfrm/xfrm_state.c     |  1 +
>  net/xfrm/xfrm_user.c      | 44 +++++++++++++++++++++++++++++++++++----
>  6 files changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 1d107241b901..91348a03469c 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;
> +	enum xfrm_sa_dir	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..46a9770c720c 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -141,6 +141,12 @@ enum {
>  	XFRM_POLICY_MAX	= 3
>  };
> 
> +enum xfrm_sa_dir {
> +	XFRM_SA_DIR_UNSET = 0,

It doesn't belong to UAPI. Kernel and user space use netlink attribute to understand
if direction is set.

> +	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 +321,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..11339d7d7140 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -253,6 +253,11 @@ 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_IN) {

It will break backward compatibility. In old iproute2 XFRM_OFFLOAD_INBOUND is set, but x->dir is not set yet.

> +		NL_SET_ERR_MSG(extack, "Mismatch in SA and offload direction");
> +		return -EINVAL;
> +	}
> +
>  	is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;

Thanks