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

[Sabrina Dubroca <sd@queasysnail.net>]

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@secunet.com>
> Reviewed-by: Leon Romanovsky <leonro@nvidia.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