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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 24 Jun 2024 11:46:14 -0400
From: Christian Hopps <chopps@...pps.org>
To: Antony Antony <antony@...nome.org>
Cc: Christian Hopps <chopps@...pps.org>,
 devel@...ux-ipsec.org,
 Steffen Klassert <steffen.klassert@...unet.com>,
 netdev@...r.kernel.org
Subject: Re: [devel-ipsec] [PATCH ipsec-next v4 17/18] xfrm: iptfs: only send
 the NL attrs that corr. to the SA dir



> On Jun 24, 2024, at 11:27, Antony Antony <antony@...nome.org> wrote:
> 
> On Mon, Jun 17, 2024 at 04:53:15PM -0400, Christian Hopps via Devel wrote:
>> From: Christian Hopps <chopps@...n.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@...n.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