diff mbox series

[ipsec-next,v4,17/18] xfrm: iptfs: only send the NL attrs that corr. to the SA dir

Message ID 20240617205316.939774-18-chopps@chopps.org (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Add IP-TFS mode to xfrm | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Christian Hopps June 17, 2024, 8:53 p.m. UTC 
From: Christian Hopps <chopps@labn.net>

When sending the netlink attributes to the user for a given SA, only
send those NL attributes which correspond to the SA's direction.

Signed-off-by: Christian Hopps <chopps@labn.net>
---
 net/xfrm/xfrm_iptfs.c | 64 ++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

Comments

Antony Antony June 24, 2024, 3:27 p.m. UTC | #1 
On Mon, Jun 17, 2024 at 04:53:15PM -0400, Christian Hopps via Devel wrote:
> From: Christian Hopps <chopps@labn.net>
> 
> When sending the netlink attributes to the user for a given SA, only
> send those NL attributes which correspond to the SA's direction.
> 
> Signed-off-by: Christian Hopps <chopps@labn.net>
> ---
>  net/xfrm/xfrm_iptfs.c | 64 ++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 59fd8ee49cd4..049a94a5531b 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -2498,13 +2498,16 @@ static unsigned int iptfs_sa_len(const struct xfrm_state *x)
>  	struct xfrm_iptfs_config *xc = &xtfs->cfg;
>  	unsigned int l = 0;
>  
> -	if (xc->dont_frag)
> -		l += nla_total_size(0);
> -	l += nla_total_size(sizeof(xc->reorder_win_size));
> -	l += nla_total_size(sizeof(xc->pkt_size));
> -	l += nla_total_size(sizeof(xc->max_queue_size));
> -	l += nla_total_size(sizeof(u32)); /* drop time usec */
> -	l += nla_total_size(sizeof(u32)); /* init delay usec */
> +	if (x->dir == XFRM_SA_DIR_IN) {
> +		l += nla_total_size(sizeof(u32)); /* drop time usec */
> +		l += nla_total_size(sizeof(xc->reorder_win_size));
> +	} else {
> +		if (xc->dont_frag)
> +			l += nla_total_size(0);	  /* dont-frag flag */
> +		l += nla_total_size(sizeof(u32)); /* init delay usec */
> +		l += nla_total_size(sizeof(xc->max_queue_size));
> +		l += nla_total_size(sizeof(xc->pkt_size));
> +	}
>  
>  	return l;
>  }
> @@ -2516,30 +2519,35 @@ static int iptfs_copy_to_user(struct xfrm_state *x, struct sk_buff *skb)
>  	int ret;
>  	u64 q;
>  
> -	if (xc->dont_frag) {
> -		ret = nla_put_flag(skb, XFRMA_IPTFS_DONT_FRAG);
> +	if (x->dir == XFRM_SA_DIR_IN) {
> +		q = xtfs->drop_time_ns;
> +		(void)do_div(q, NSECS_IN_USEC);
> +		ret = nla_put_u32(skb, XFRMA_IPTFS_DROP_TIME, q);
> +		if (ret)
> +			return ret;
> +
> +		ret = nla_put_u16(skb, XFRMA_IPTFS_REORDER_WINDOW,
> +				  xc->reorder_win_size);
> +	} else {
> +		if (xc->dont_frag) {
> +			ret = nla_put_flag(skb, XFRMA_IPTFS_DONT_FRAG);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		q = xtfs->init_delay_ns;
> +		(void)do_div(q, NSECS_IN_USEC);
> +		ret = nla_put_u32(skb, XFRMA_IPTFS_INIT_DELAY, q);
> +		if (ret)
> +			return ret;
> +
> +		ret = nla_put_u32(skb, XFRMA_IPTFS_MAX_QSIZE,
> +				  xc->max_queue_size);
>  		if (ret)
>  			return ret;
> +
> +		ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size);
>  	}
> -	ret = nla_put_u16(skb, XFRMA_IPTFS_REORDER_WINDOW, xc->reorder_win_size);
> -	if (ret)
> -		return ret;
> -	ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size);
> -	if (ret)
> -		return ret;
> -	ret = nla_put_u32(skb, XFRMA_IPTFS_MAX_QSIZE, xc->max_queue_size);
> -	if (ret)
> -		return ret;
> -
> -	q = xtfs->drop_time_ns;
> -	(void)do_div(q, NSECS_IN_USEC);
> -	ret = nla_put_u32(skb, XFRMA_IPTFS_DROP_TIME, q);
> -	if (ret)
> -		return ret;
> -
> -	q = xtfs->init_delay_ns;
> -	(void)do_div(q, NSECS_IN_USEC);
> -	ret = nla_put_u32(skb, XFRMA_IPTFS_INIT_DELAY, q);
>  
>  	return ret;
>  }

looking at this patch, why this should be seperate patch? why not squash 
into [PATCH ipsec-next v4 08/18] xfrm: iptfs: add new iptfs xfrm mode impl

I also think in the v3 it was squashed into some other patch.

-antony
Christian Hopps June 24, 2024, 3:46 p.m. UTC | #2 
> On Jun 24, 2024, at 11:27, Antony Antony <antony@phenome.org> wrote:
> 
> On Mon, Jun 17, 2024 at 04:53:15PM -0400, Christian Hopps via Devel wrote:
>> From: Christian Hopps <chopps@labn.net>
>> 
>> When sending the netlink attributes to the user for a given SA, only
>> send those NL attributes which correspond to the SA's direction.
>> 
>> Signed-off-by: Christian Hopps <chopps@labn.net>
>> ---
>> net/xfrm/xfrm_iptfs.c | 64 ++++++++++++++++++++++++-------------------
>> 1 file changed, 36 insertions(+), 28 deletions(-)
>> 
>> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
>> index 59fd8ee49cd4..049a94a5531b 100644
>> --- a/net/xfrm/xfrm_iptfs.c
>> +++ b/net/xfrm/xfrm_iptfs.c
>> @@ -2498,13 +2498,16 @@ static unsigned int iptfs_sa_len(const struct xfrm_state *x)
>> struct xfrm_iptfs_config *xc = &xtfs->cfg;
>> unsigned int l = 0;
>> 
>> - if (xc->dont_frag)
>> - l += nla_total_size(0);
>> - l += nla_total_size(sizeof(xc->reorder_win_size));
>> - l += nla_total_size(sizeof(xc->pkt_size));
>> - l += nla_total_size(sizeof(xc->max_queue_size));
>> - l += nla_total_size(sizeof(u32)); /* drop time usec */
>> - l += nla_total_size(sizeof(u32)); /* init delay usec */
>> + if (x->dir == XFRM_SA_DIR_IN) {
>> + l += nla_total_size(sizeof(u32)); /* drop time usec */
>> + l += nla_total_size(sizeof(xc->reorder_win_size));
>> + } else {
>> + if (xc->dont_frag)
>> + l += nla_total_size(0);   /* dont-frag flag */
>> + l += nla_total_size(sizeof(u32)); /* init delay usec */
>> + l += nla_total_size(sizeof(xc->max_queue_size));
>> + l += nla_total_size(sizeof(xc->pkt_size));
>> + }
>> 
>> return l;
>> }
>> @@ -2516,30 +2519,35 @@ static int iptfs_copy_to_user(struct xfrm_state *x, struct sk_buff *skb)
>> int ret;
>> u64 q;
>> 
>> - if (xc->dont_frag) {
>> - ret = nla_put_flag(skb, XFRMA_IPTFS_DONT_FRAG);
>> + if (x->dir == XFRM_SA_DIR_IN) {
>> + q = xtfs->drop_time_ns;
>> + (void)do_div(q, NSECS_IN_USEC);
>> + ret = nla_put_u32(skb, XFRMA_IPTFS_DROP_TIME, q);
>> + if (ret)
>> + return ret;
>> +
>> + ret = nla_put_u16(skb, XFRMA_IPTFS_REORDER_WINDOW,
>> +   xc->reorder_win_size);
>> + } else {
>> + if (xc->dont_frag) {
>> + ret = nla_put_flag(skb, XFRMA_IPTFS_DONT_FRAG);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + q = xtfs->init_delay_ns;
>> + (void)do_div(q, NSECS_IN_USEC);
>> + ret = nla_put_u32(skb, XFRMA_IPTFS_INIT_DELAY, q);
>> + if (ret)
>> + return ret;
>> +
>> + ret = nla_put_u32(skb, XFRMA_IPTFS_MAX_QSIZE,
>> +   xc->max_queue_size);
>> if (ret)
>> return ret;
>> +
>> + ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size);
>> }
>> - ret = nla_put_u16(skb, XFRMA_IPTFS_REORDER_WINDOW, xc->reorder_win_size);
>> - if (ret)
>> - return ret;
>> - ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size);
>> - if (ret)
>> - return ret;
>> - ret = nla_put_u32(skb, XFRMA_IPTFS_MAX_QSIZE, xc->max_queue_size);
>> - if (ret)
>> - return ret;
>> -
>> - q = xtfs->drop_time_ns;
>> - (void)do_div(q, NSECS_IN_USEC);
>> - ret = nla_put_u32(skb, XFRMA_IPTFS_DROP_TIME, q);
>> - if (ret)
>> - return ret;
>> -
>> - q = xtfs->init_delay_ns;
>> - (void)do_div(q, NSECS_IN_USEC);
>> - ret = nla_put_u32(skb, XFRMA_IPTFS_INIT_DELAY, q);
>> 
>> return ret;
>> }
> 
> looking at this patch, why this should be seperate patch? why not squash 
> into [PATCH ipsec-next v4 08/18] xfrm: iptfs: add new iptfs xfrm mode impl

The various attributes get modified by the layered functionality commits, so it ends up needing to be worked into a bunch of commits. So given it was a simple patch addressing a review comment I thought it would be OK to just leave it a single simple patch. If it's important I will do the work to incorporate the change into the N different commits :)

Thanks,
Chris.

> 
> I also think in the v3 it was squashed into some other patch.
> 
> -antony
diff mbox series

Patch

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 59fd8ee49cd4..049a94a5531b 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -2498,13 +2498,16 @@  static unsigned int iptfs_sa_len(const struct xfrm_state *x)
 	struct xfrm_iptfs_config *xc = &xtfs->cfg;
 	unsigned int l = 0;
 
-	if (xc->dont_frag)
-		l += nla_total_size(0);
-	l += nla_total_size(sizeof(xc->reorder_win_size));
-	l += nla_total_size(sizeof(xc->pkt_size));
-	l += nla_total_size(sizeof(xc->max_queue_size));
-	l += nla_total_size(sizeof(u32)); /* drop time usec */
-	l += nla_total_size(sizeof(u32)); /* init delay usec */
+	if (x->dir == XFRM_SA_DIR_IN) {
+		l += nla_total_size(sizeof(u32)); /* drop time usec */
+		l += nla_total_size(sizeof(xc->reorder_win_size));
+	} else {
+		if (xc->dont_frag)
+			l += nla_total_size(0);	  /* dont-frag flag */
+		l += nla_total_size(sizeof(u32)); /* init delay usec */
+		l += nla_total_size(sizeof(xc->max_queue_size));
+		l += nla_total_size(sizeof(xc->pkt_size));
+	}
 
 	return l;
 }
@@ -2516,30 +2519,35 @@  static int iptfs_copy_to_user(struct xfrm_state *x, struct sk_buff *skb)
 	int ret;
 	u64 q;
 
-	if (xc->dont_frag) {
-		ret = nla_put_flag(skb, XFRMA_IPTFS_DONT_FRAG);
+	if (x->dir == XFRM_SA_DIR_IN) {
+		q = xtfs->drop_time_ns;
+		(void)do_div(q, NSECS_IN_USEC);
+		ret = nla_put_u32(skb, XFRMA_IPTFS_DROP_TIME, q);
+		if (ret)
+			return ret;
+
+		ret = nla_put_u16(skb, XFRMA_IPTFS_REORDER_WINDOW,
+				  xc->reorder_win_size);
+	} else {
+		if (xc->dont_frag) {
+			ret = nla_put_flag(skb, XFRMA_IPTFS_DONT_FRAG);
+			if (ret)
+				return ret;
+		}
+
+		q = xtfs->init_delay_ns;
+		(void)do_div(q, NSECS_IN_USEC);
+		ret = nla_put_u32(skb, XFRMA_IPTFS_INIT_DELAY, q);
+		if (ret)
+			return ret;
+
+		ret = nla_put_u32(skb, XFRMA_IPTFS_MAX_QSIZE,
+				  xc->max_queue_size);
 		if (ret)
 			return ret;
+
+		ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size);
 	}
-	ret = nla_put_u16(skb, XFRMA_IPTFS_REORDER_WINDOW, xc->reorder_win_size);
-	if (ret)
-		return ret;
-	ret = nla_put_u32(skb, XFRMA_IPTFS_PKT_SIZE, xc->pkt_size);
-	if (ret)
-		return ret;
-	ret = nla_put_u32(skb, XFRMA_IPTFS_MAX_QSIZE, xc->max_queue_size);
-	if (ret)
-		return ret;
-
-	q = xtfs->drop_time_ns;
-	(void)do_div(q, NSECS_IN_USEC);
-	ret = nla_put_u32(skb, XFRMA_IPTFS_DROP_TIME, q);
-	if (ret)
-		return ret;
-
-	q = xtfs->init_delay_ns;
-	(void)do_div(q, NSECS_IN_USEC);
-	ret = nla_put_u32(skb, XFRMA_IPTFS_INIT_DELAY, q);
 
 	return ret;
 }