Message-ID: <20240411103740.GM4195@unreal>
Date: Thu, 11 Apr 2024 13:37:40 +0300
From: Leon Romanovsky <leon@...nel.org>
To: antony.antony@...unet.com
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	devel@...ux-ipsec.org, Eyal Birger <eyal.birger@...il.com>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	Sabrina Dubroca <sd@...asysnail.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v8] xfrm: Add Direction to the
 SA in or out

On Tue, Apr 09, 2024 at 07:37:20PM +0200, Antony Antony via Devel 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.
> 
> This feature sets the groundwork for future patches, including
> the upcoming IP-TFS patch.
> 
> v7->v8:
>  - add extra validation check on replay window and seq
>  - XFRM_MSG_UPDSA old and new SA should match "dir"

Why? Update is add and delete operation, and one can update any field
he/she wants, including direction.

> 
> v6->v7:
>  - add replay-window check non-esn 0 and ESN 1.
>  - remove :XFRMA_SA_DIR only allowed with HW OFFLOAD
> 
> v5->v6:
>  - XFRMA_SA_DIR only allowed with HW OFFLOAD
> 
> v4->v5:
>  - add details to commit message
> 
> v3->v4:
>  - improve HW OFFLOAD DIR check add the other direction
> 
> v2->v3:
>  - delete redundant XFRM_SA_DIR_USE
>  - 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

Please put changelog after --- trailer.

Thanks

> 
> Signed-off-by: Antony Antony <antony.antony@...unet.com>
> ---
>  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     |  4 ++
>  net/xfrm/xfrm_user.c      | 85 +++++++++++++++++++++++++++++++++++++--
>  6 files changed, 103 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 57c743b7e4fe..7c9be06f8302 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -291,6 +291,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 6346690d5c69..2455a76a1cff 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 0c306473a79d..f7771a69ae2e 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;
>  
> @@ -1857,6 +1858,9 @@ int xfrm_state_update(struct xfrm_state *x)
>  	if (!x1)
>  		goto out;
>  
> +	if (x1->dir != x->dir)
> +		goto out;
> +
>  	if (xfrm_state_kern(x1)) {
>  		to_put = x1;
>  		err = -EEXIST;
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 810b520493f3..4e5256155c73 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -360,6 +360,55 @@ 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;
> +		}
> +
> +		if (sa_dir == XFRM_SA_DIR_OUT)  {
> +			if (p->replay_window > 0) {
> +				NL_SET_ERR_MSG(extack, "Replay window should not be set for OUT SA");
> +				err = -EINVAL;
> +				goto out;
> +			}
> +
> +			if (attrs[XFRMA_REPLAY_VAL]) {
> +				struct xfrm_replay_state *rp;
> +
> +				rp = nla_data(attrs[XFRMA_REPLAY_VAL]);
> +				if (rp->oseq ||  rp->seq || rp->bitmap) {
> +					NL_SET_ERR_MSG(extack,
> +						       "Replay seq, oseq, or bitmap should not be set for OUT SA with ESN");
> +					err = -EINVAL;
> +					goto out;
> +				}
> +			}
> +
> +			if (attrs[XFRMA_REPLAY_ESN_VAL]) {
> +				struct xfrm_replay_state_esn *esn;
> +
> +				esn = nla_data(attrs[XFRMA_REPLAY_ESN_VAL]);
> +				if (esn->replay_window > 1) {
> +					NL_SET_ERR_MSG(extack,
> +						       "Replay window should be 1 for  OUT SA with ESN");
> +					err = -EINVAL;
> +					goto out;
> +				}
> +
> +				if (esn->seq || esn->seq_hi || esn->bmp_len) {
> +					NL_SET_ERR_MSG(extack,
> +						       "Replay seq, seq_hi, bmp_len should not be set for OUT SA with ESN");
> +					err = -EINVAL;
> +					goto out;
> +				}
> +			}
> +		}
> +	}
> +
>  out:
>  	return err;
>  }
> @@ -627,6 +676,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 +711,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 +1235,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;
>  }
> @@ -2402,7 +2460,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)
> @@ -2459,6 +2518,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;
>  
> @@ -3018,6 +3083,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},
> @@ -3049,6 +3115,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);
>  
> @@ -3189,8 +3256,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)
> @@ -3217,6 +3285,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;
>  }
> @@ -3324,6 +3398,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
> 
> -- 
> Devel mailing list
> Devel@...ux-ipsec.org
> https://linux-ipsec.org/mailman/listinfo/devel

Powered by blists - more mailing lists