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

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]

Message-ID: <ZhBzcMrpBCNXXVBV@hog>
Date: Fri, 5 Apr 2024 23:56:00 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antony Antony <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, Leon Romanovsky <leon@...nel.org>,
	Eyal Birger <eyal.birger@...il.com>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>
Subject: Re: [PATCH ipsec-next v6] xfrm: Add Direction to the SA in or out

Hi Antony,

2024-04-05, 14:40:07 +0200, 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. 

But this patch isn't doing that for existing properties (I'm thinking
of replay window, not sure if any others are relevant [1]). Why not?

[1] that should include values passed via xfrm_usersa_info too,
    not just XFRMA_* attributes

Adding these checks should be safe (wrt breakage of API): Old software
would not be passing XFRMA_SA_DIR, so adding checks when it is provided
would not break anything there. Only new software using the attribute
would benefit from having directed SAs and restriction on which attributes
can be used (and that's fine).

Right now the new attribute is 100% duplicate of the existing offload
direction, so I don't see much point.

> 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.
> 
> Currently, dir is only allowed when HW OFFLOAD is set.
> 
> ---

BTW, everything after this '---' will get cut, including your sign-off.

> v5->v6:
>  - XFRMA_SA_DIR only allowed with HW OFFLOAD
> 
> v4->v5:
>  - add details to commit message
> 
> v3->v4:
>  - improve HW OFFLOAD DIR check check 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
> ---
> 
> Signed-off-by: Antony Antony <antony.antony@...unet.com>
> Reviewed-by: Leon Romanovsky <leonro@...dia.com>

nit: If I'm making non-trivial changes to the contents of the patch, I
typically drop the review (and test) tags I got on previous versions,
since they may no longer apply.

-- 
Sabrina