[PATCH ipsec-next] xfrm: delete not-used XFRM_OFFLOAD_IPV6 define

[PATCH ipsec-next] xfrm: delete not-used XFRM_OFFLOAD_IPV6 define

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

XFRM_OFFLOAD_IPV6 define was exposed in the commit mentioned in the
fixes line, but it is never been used both in the kernel and in the
user space. So delete it.

Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/uapi/linux/xfrm.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 4e29d7851890..2c822671cc32 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -511,7 +511,6 @@ struct xfrm_user_offload {
 	int				ifindex;
 	__u8				flags;
 };
-#define XFRM_OFFLOAD_IPV6	1
 #define XFRM_OFFLOAD_INBOUND	2
 
 struct xfrm_userpolicy_default {
-- 
2.34.1

Re: [PATCH ipsec-next] xfrm: delete not-used XFRM_OFFLOAD_IPV6 define

[Steffen Klassert]

On Thu, Jan 27, 2022 at 08:24:58PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> XFRM_OFFLOAD_IPV6 define was exposed in the commit mentioned in the
> fixes line, but it is never been used both in the kernel and in the
> user space. So delete it.

How can you be sure that is is not used in userspace? At least some
versions of strongswan set that flag. So even if it is meaningless
in the kernel, we can't remove it.

> 
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  include/uapi/linux/xfrm.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index 4e29d7851890..2c822671cc32 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -511,7 +511,6 @@ struct xfrm_user_offload {
>  	int				ifindex;
>  	__u8				flags;
>  };
> -#define XFRM_OFFLOAD_IPV6	1
>  #define XFRM_OFFLOAD_INBOUND	2
>  
>  struct xfrm_userpolicy_default {
> -- 
> 2.34.1

Re: [PATCH ipsec-next] xfrm: delete not-used XFRM_OFFLOAD_IPV6 define

[Leon Romanovsky]

On Tue, Feb 01, 2022 at 07:58:36AM +0100, Steffen Klassert wrote:
> On Thu, Jan 27, 2022 at 08:24:58PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > XFRM_OFFLOAD_IPV6 define was exposed in the commit mentioned in the
> > fixes line, but it is never been used both in the kernel and in the
> > user space. So delete it.
> 
> How can you be sure that is is not used in userspace? At least some
> versions of strongswan set that flag. So even if it is meaningless
> in the kernel, we can't remove it.

I looked over all net/* and include/uapi/* code with "git log -p" and didn't
see any use of this flag ever. 

Looking in strongsswan, I see that they bring kernel header files [1] for the build
and removal won't break build of old strongsswan versions.

We just can't use this bit anymore, because of this commit [2]. I have
no clue why it was used there.

So yes, we can remove, but worth to add a comment about old strongsswan.

And if we are talking about xfrm_user_offload flags, there is a
well-known API mistake in xfrm_dev_state_add() of not checking validity
of flags. So *theoretically* we can find some software in the wild that
uses other bits too.

I would like to see it is fixed.

[1] 5bfae68670f2 ("include: Update xfrm.h to include hardware offloading extensions")
[2] d42948fc057e ("kernel-netlink: Enable hardware offloading if configured for an SA")

Thanks

> 
> > 
> > Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  include/uapi/linux/xfrm.h | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > index 4e29d7851890..2c822671cc32 100644
> > --- a/include/uapi/linux/xfrm.h
> > +++ b/include/uapi/linux/xfrm.h
> > @@ -511,7 +511,6 @@ struct xfrm_user_offload {
> >  	int				ifindex;
> >  	__u8				flags;
> >  };
> > -#define XFRM_OFFLOAD_IPV6	1
> >  #define XFRM_OFFLOAD_INBOUND	2
> >  
> >  struct xfrm_userpolicy_default {
> > -- 
> > 2.34.1

Re: [PATCH ipsec-next] xfrm: delete not-used XFRM_OFFLOAD_IPV6 define

[Leon Romanovsky]

On Tue, Feb 01, 2022 at 09:22:17AM +0200, Leon Romanovsky wrote:
> On Tue, Feb 01, 2022 at 07:58:36AM +0100, Steffen Klassert wrote:
> > On Thu, Jan 27, 2022 at 08:24:58PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > XFRM_OFFLOAD_IPV6 define was exposed in the commit mentioned in the
> > > fixes line, but it is never been used both in the kernel and in the
> > > user space. So delete it.
> > 
> > How can you be sure that is is not used in userspace? At least some
> > versions of strongswan set that flag. So even if it is meaningless
> > in the kernel, we can't remove it.
> 
> I looked over all net/* and include/uapi/* code with "git log -p" and didn't
> see any use of this flag ever. 

And in my search on github, I didn't see anyone except strongswan who
used this flag.

Thanks

Re: [PATCH ipsec-next] xfrm: delete not-used XFRM_OFFLOAD_IPV6 define

[Steffen Klassert]

On Tue, Feb 01, 2022 at 09:22:17AM +0200, Leon Romanovsky wrote:
> On Tue, Feb 01, 2022 at 07:58:36AM +0100, Steffen Klassert wrote:
> > On Thu, Jan 27, 2022 at 08:24:58PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > XFRM_OFFLOAD_IPV6 define was exposed in the commit mentioned in the
> > > fixes line, but it is never been used both in the kernel and in the
> > > user space. So delete it.
> > 
> > How can you be sure that is is not used in userspace? At least some
> > versions of strongswan set that flag. So even if it is meaningless
> > in the kernel, we can't remove it.
> 
> I looked over all net/* and include/uapi/* code with "git log -p" and didn't
> see any use of this flag ever. 
> 
> Looking in strongsswan, I see that they bring kernel header files [1] for the build
> and removal won't break build of old strongsswan versions.
> 
> We just can't use this bit anymore, because of this commit [2]. I have
> no clue why it was used there.
> 
> So yes, we can remove, but worth to add a comment about old strongsswan.

It is always problematic to remove something that was exposed
to userspace by the uapi, that should not happen. We can't
know that nobody uses that, even if unlikely. So please keep
it and maybe mark it as unused with a comment.

> 
> And if we are talking about xfrm_user_offload flags, there is a
> well-known API mistake in xfrm_dev_state_add() of not checking validity
> of flags. So *theoretically* we can find some software in the wild that
> uses other bits too.
> 
> I would like to see it is fixed.

Yes, catching usage of undefined flags should be indeed implemented.

Thanks!