[ipsec,v3,0/2] xfrm: fix uapi for the default policy

Message

Nicolas Dichtel Sept. 14, 2021, 2:46 p.m. UTC

This feature has just been merged after the last release, thus it's still
time to fix the uapi.
As stated in the thread, the uapi is based on some magic values (from the
userland POV).
Here is a proposal to simplify this uapi and make it clear how to use it.
The other problem was the notification: changing the default policy may
radically change the packets flows.

v2 -> v3: rebase on top of ipsec tree

v1 -> v2: fix warnings reported by the kernel test robot

Nicolas Dichtel (2):
  xfrm: make user policy API complete
  xfrm: notify default policy on update

 include/uapi/linux/xfrm.h |  9 ++++--
 net/xfrm/xfrm_user.c      | 67 +++++++++++++++++++++++++++++----------
 2 files changed, 56 insertions(+), 20 deletions(-)

Comments are welcome,
Nicolas

Comments

Antony Antony Sept. 15, 2021, 9:19 a.m. UTC | #1

Hi Nicolas,

On Tue, Sep 14, 2021 at 16:46:32 +0200, Nicolas Dichtel wrote:
> This feature has just been merged after the last release, thus it's still
> time to fix the uapi.
> As stated in the thread, the uapi is based on some magic values (from the
> userland POV).

I like your proposal to make uapi 3 different variables, instead of flags.

This fix leave kernel internal representation as a flags in
struct netns_xfrm
	u8 policy_default;

I have a concern. If your patch is applied, the uapi and xfrm internal representations would be inconsistant. I think they should be the same in this case.
It would easier to follow the code path.
On the other hand we should apply this uapi change ASAP, in 5.15 release cycle, to avoid ABI change.

Could you also change xfrm policy_default to three variables?

> Here is a proposal to simplify this uapi and make it clear how to use it.
> The other problem was the notification: changing the default policy may
> radically change the packets flows.
> 
> v2 -> v3: rebase on top of ipsec tree
> 
> v1 -> v2: fix warnings reported by the kernel test robot
>

Nicolas Dichtel Sept. 15, 2021, 9:55 a.m. UTC | #2

Le 15/09/2021 à 11:19, Antony Antony a écrit :
> Hi Nicolas,
> 
> On Tue, Sep 14, 2021 at 16:46:32 +0200, Nicolas Dichtel wrote:
>> This feature has just been merged after the last release, thus it's still
>> time to fix the uapi.
>> As stated in the thread, the uapi is based on some magic values (from the
>> userland POV).
> 
> I like your proposal to make uapi 3 different variables, instead of flags.
> 
> This fix leave kernel internal representation as a flags in
> struct netns_xfrm
> 	u8 policy_default;
> 
> I have a concern. If your patch is applied, the uapi and xfrm internal representations would be inconsistant. I think they should be the same in this case.
I agree.

> It would easier to follow the code path.
> On the other hand we should apply this uapi change ASAP, in 5.15 release cycle, to avoid ABI change.
I also agree.

> 
> Could you also change xfrm policy_default to three variables?
Yes, I propose to send a follow up on ipsec-next once this series is applied.
The internal representation could be changed later, I prefer to keep this change
minimal for the ipsec tree.


Thank you,
Nicolas

Steffen Klassert Sept. 17, 2021, 7:06 a.m. UTC | #3

On Tue, Sep 14, 2021 at 04:46:32PM +0200, Nicolas Dichtel wrote:
> This feature has just been merged after the last release, thus it's still
> time to fix the uapi.
> As stated in the thread, the uapi is based on some magic values (from the
> userland POV).
> Here is a proposal to simplify this uapi and make it clear how to use it.
> The other problem was the notification: changing the default policy may
> radically change the packets flows.
> 
> v2 -> v3: rebase on top of ipsec tree
> 
> v1 -> v2: fix warnings reported by the kernel test robot
> 
> Nicolas Dichtel (2):
>   xfrm: make user policy API complete
>   xfrm: notify default policy on update

Applied, thanks a lot Nicolas!

Nicolas Dichtel Sept. 17, 2021, 7:54 a.m. UTC | #4

Le 17/09/2021 à 09:06, Steffen Klassert a écrit :
> On Tue, Sep 14, 2021 at 04:46:32PM +0200, Nicolas Dichtel wrote:
>> This feature has just been merged after the last release, thus it's still
>> time to fix the uapi.
>> As stated in the thread, the uapi is based on some magic values (from the
>> userland POV).
>> Here is a proposal to simplify this uapi and make it clear how to use it.
>> The other problem was the notification: changing the default policy may
>> radically change the packets flows.
>>
>> v2 -> v3: rebase on top of ipsec tree
>>
>> v1 -> v2: fix warnings reported by the kernel test robot
>>
>> Nicolas Dichtel (2):
>>   xfrm: make user policy API complete
>>   xfrm: notify default policy on update
> 
> Applied, thanks a lot Nicolas!
> 
Thanks Steffen. I will write the follow up patch once the ipsec tree is merged
into ipsec-next.


Regards,
Nicolas