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

From: Christian Hopps <[email-masked]>
Date: Wed, 10 Apr 2024 17:41:54 -0400

Antony Antony via Devel <[email-masked]> writes:

> On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote:
>> 2024-04-09, 19:23:04 +0200, Antony Antony wrote:
>> > On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote:
>> > > 2024-04-07, 10:23:21 +0200, Antony Antony wrote:
>> > > I think it would also make sense to only accept this attribute in
>> > > xfrm_add_sa, and not for any of the other message types. Sharing
>> >
>> > > xfrma_policy is convenient but not all attributes are valid for all
>> > > requests. Old attributes can't be changed, but we should try to be
>> > > more strict when we introduce new attributes.
>> >
>> > To clarify your feedback, are you suggesting the API should not permit
>> > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for
>> > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used
>> > equivalently to XFRM_MSG_NEWSA by *swan.
>>
>> Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA
>> and UPDSA should accept it, but I'm thinking none of the other
>> operations should. It's a property of SAs, not of other xfrm objects.
>
> For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in
> DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in
> DELSA, it would also be disregarded. Attempting to introduce validations for
> DELSA and other methods seems like an extensive cleanup task. Do we consider
> this level of validation within the scope of our current patch? It feels
> like we are going too far.

I think a general clean up feels like a distinct patch to me.

Thanks,
Chris.