diff mbox series

[ipsec-next,v3,3/3] xfrm: lwtunnel: add lwtunnel support for xfrm interfaces in collect_md mode

Message ID 20220825154630.2174742-4-eyal.birger@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series xfrm: support collect metadata mode for xfrm interfaces | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1852 this patch: 1852
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 250 this patch: 250
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1983 this patch: 1983
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 154 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eyal Birger Aug. 25, 2022, 3:46 p.m. UTC
Allow specifying the xfrm interface if_id and link as part of a route
metadata using the lwtunnel infrastructure.

This allows for example using a single xfrm interface in collect_md
mode as the target of multiple routes each specifying a different if_id.

With the appropriate changes to iproute2, considering an xfrm device
ipsec1 in collect_md mode one can for example add a route specifying
an if_id like so:

ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1

In which case traffic routed to the device via this route would use
if_id in the xfrm interface policy lookup.

Or in the context of vrf, one can also specify the "link" property:

ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1 link_dev eth15

Signed-off-by: Eyal Birger <eyal.birger@gmail.com>

----

v3: netlink improvements as suggested by Nikolay Aleksandrov and
    Nicolas Dichtel

v2:
  - move lwt_xfrm_info() helper to dst_metadata.h
  - add "link" property as suggested by Nicolas Dichtel
---
 include/net/dst_metadata.h    | 11 +++++
 include/uapi/linux/lwtunnel.h | 10 +++++
 net/core/lwtunnel.c           |  1 +
 net/xfrm/xfrm_interface.c     | 85 +++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+)

Comments

Nicolas Dichtel Aug. 26, 2022, 8:05 a.m. UTC | #1
Le 25/08/2022 à 17:46, Eyal Birger a écrit :
> Allow specifying the xfrm interface if_id and link as part of a route
> metadata using the lwtunnel infrastructure.
> 
> This allows for example using a single xfrm interface in collect_md
> mode as the target of multiple routes each specifying a different if_id.
> 
> With the appropriate changes to iproute2, considering an xfrm device
> ipsec1 in collect_md mode one can for example add a route specifying
> an if_id like so:
> 
> ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1
> 
> In which case traffic routed to the device via this route would use
> if_id in the xfrm interface policy lookup.
> 
> Or in the context of vrf, one can also specify the "link" property:
> 
> ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1 link_dev eth15
> 
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> 
> ----
> 
> v3: netlink improvements as suggested by Nikolay Aleksandrov and
>     Nicolas Dichtel
> 
> v2:
>   - move lwt_xfrm_info() helper to dst_metadata.h
>   - add "link" property as suggested by Nicolas Dichtel
> ---
>  include/net/dst_metadata.h    | 11 +++++
>  include/uapi/linux/lwtunnel.h | 10 +++++
>  net/core/lwtunnel.c           |  1 +
>  net/xfrm/xfrm_interface.c     | 85 +++++++++++++++++++++++++++++++++++
>  4 files changed, 107 insertions(+)
> 
> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index e4b059908cc7..57f75960fa28 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -60,13 +60,24 @@ skb_tunnel_info(const struct sk_buff *skb)
>  	return NULL;
>  }
>  
> +static inline struct xfrm_md_info *lwt_xfrm_info(struct lwtunnel_state *lwt)
> +{
> +	return (struct xfrm_md_info *)lwt->data;
> +}
> +
>  static inline struct xfrm_md_info *skb_xfrm_md_info(const struct sk_buff *skb)
>  {
>  	struct metadata_dst *md_dst = skb_metadata_dst(skb);
> +	struct dst_entry *dst;
>  
>  	if (md_dst && md_dst->type == METADATA_XFRM)
>  		return &md_dst->u.xfrm_info;
>  
> +	dst = skb_dst(skb);
> +	if (dst && dst->lwtstate &&
> +	    dst->lwtstate->type == LWTUNNEL_ENCAP_XFRM)
> +		return lwt_xfrm_info(dst->lwtstate);
> +
>  	return NULL;
>  }
>  
> diff --git a/include/uapi/linux/lwtunnel.h b/include/uapi/linux/lwtunnel.h
> index 2e206919125c..229655ef792f 100644
> --- a/include/uapi/linux/lwtunnel.h
> +++ b/include/uapi/linux/lwtunnel.h
> @@ -15,6 +15,7 @@ enum lwtunnel_encap_types {
>  	LWTUNNEL_ENCAP_SEG6_LOCAL,
>  	LWTUNNEL_ENCAP_RPL,
>  	LWTUNNEL_ENCAP_IOAM6,
> +	LWTUNNEL_ENCAP_XFRM,
>  	__LWTUNNEL_ENCAP_MAX,
>  };
>  
> @@ -111,4 +112,13 @@ enum {
>  
>  #define LWT_BPF_MAX_HEADROOM 256
>  
> +enum {
> +	LWT_XFRM_UNSPEC,
> +	LWT_XFRM_IF_ID,
> +	LWT_XFRM_LINK,
> +	__LWT_XFRM_MAX,
> +};
> +
> +#define LWT_XFRM_MAX (__LWT_XFRM_MAX - 1)
> +
>  #endif /* _UAPI_LWTUNNEL_H_ */
> diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
> index 9ccd64e8a666..6fac2f0ef074 100644
> --- a/net/core/lwtunnel.c
> +++ b/net/core/lwtunnel.c
> @@ -50,6 +50,7 @@ static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type)
>  		return "IOAM6";
>  	case LWTUNNEL_ENCAP_IP6:
>  	case LWTUNNEL_ENCAP_IP:
> +	case LWTUNNEL_ENCAP_XFRM:
>  	case LWTUNNEL_ENCAP_NONE:
>  	case __LWTUNNEL_ENCAP_MAX:
>  		/* should not have got here */
> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> index e9a355047468..495dee8b0764 100644
> --- a/net/xfrm/xfrm_interface.c
> +++ b/net/xfrm/xfrm_interface.c
> @@ -60,6 +60,88 @@ struct xfrmi_net {
>  	struct xfrm_if __rcu *collect_md_xfrmi;
>  };
>  
> +static const struct nla_policy xfrm_lwt_policy[LWT_XFRM_MAX + 1] = {
> +	[LWT_XFRM_IF_ID]	= NLA_POLICY_MIN(NLA_U32, 1),
> +	[LWT_XFRM_LINK]		= NLA_POLICY_MIN(NLA_S32, 1),
IMHO, it would be better to keep consistency with IFLA_XFRM_LINK.

$ git grep _LINK.*NLA_U32 net/ drivers/net/
drivers/net/gtp.c:      [GTPA_LINK]             = { .type = NLA_U32, },
drivers/net/vxlan/vxlan_core.c: [IFLA_VXLAN_LINK]       = { .type = NLA_U32 },
...
net/core/rtnetlink.c:   [IFLA_LINK]             = { .type = NLA_U32 },
...
net/ipv4/ip_gre.c:      [IFLA_GRE_LINK]         = { .type = NLA_U32 },
net/ipv4/ip_vti.c:      [IFLA_VTI_LINK]         = { .type = NLA_U32 },
net/ipv4/ipip.c:        [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
net/ipv6/ip6_gre.c:     [IFLA_GRE_LINK]        = { .type = NLA_U32 },
net/ipv6/ip6_tunnel.c:  [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
net/ipv6/ip6_vti.c:     [IFLA_VTI_LINK]         = { .type = NLA_U32 },
net/ipv6/sit.c: [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
net/sched/cls_u32.c:    [TCA_U32_LINK]          = { .type = NLA_U32 },
...
net/xfrm/xfrm_interface.c:      [IFLA_XFRM_LINK]        = { .type = NLA_U32 },
$ git grep _LINK.*NLA_S32 net/ drivers/net/
net/core/rtnetlink.c:   [IFLA_LINK_NETNSID]     = { .type = NLA_S32 },
$

They all are U32. Adding one S32 would just add confusion.
Eyal Birger Aug. 26, 2022, 10:18 a.m. UTC | #2
On Fri, Aug 26, 2022 at 11:05 AM Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
>
>
> Le 25/08/2022 à 17:46, Eyal Birger a écrit :
> > Allow specifying the xfrm interface if_id and link as part of a route
> > metadata using the lwtunnel infrastructure.
> >
> > This allows for example using a single xfrm interface in collect_md
> > mode as the target of multiple routes each specifying a different if_id.
> >
> > With the appropriate changes to iproute2, considering an xfrm device
> > ipsec1 in collect_md mode one can for example add a route specifying
> > an if_id like so:
> >
> > ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1
> >
> > In which case traffic routed to the device via this route would use
> > if_id in the xfrm interface policy lookup.
> >
> > Or in the context of vrf, one can also specify the "link" property:
> >
> > ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1 link_dev eth15
> >
> > Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> >
> > ----
> >
> > v3: netlink improvements as suggested by Nikolay Aleksandrov and
> >     Nicolas Dichtel
> >
> > v2:
> >   - move lwt_xfrm_info() helper to dst_metadata.h
> >   - add "link" property as suggested by Nicolas Dichtel
> > ---
> >  include/net/dst_metadata.h    | 11 +++++
> >  include/uapi/linux/lwtunnel.h | 10 +++++
> >  net/core/lwtunnel.c           |  1 +
> >  net/xfrm/xfrm_interface.c     | 85 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 107 insertions(+)
> >
> > diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> > index e4b059908cc7..57f75960fa28 100644
> > --- a/include/net/dst_metadata.h
> > +++ b/include/net/dst_metadata.h
> > @@ -60,13 +60,24 @@ skb_tunnel_info(const struct sk_buff *skb)
> >       return NULL;
> >  }
> >
> > +static inline struct xfrm_md_info *lwt_xfrm_info(struct lwtunnel_state *lwt)
> > +{
> > +     return (struct xfrm_md_info *)lwt->data;
> > +}
> > +
> >  static inline struct xfrm_md_info *skb_xfrm_md_info(const struct sk_buff *skb)
> >  {
> >       struct metadata_dst *md_dst = skb_metadata_dst(skb);
> > +     struct dst_entry *dst;
> >
> >       if (md_dst && md_dst->type == METADATA_XFRM)
> >               return &md_dst->u.xfrm_info;
> >
> > +     dst = skb_dst(skb);
> > +     if (dst && dst->lwtstate &&
> > +         dst->lwtstate->type == LWTUNNEL_ENCAP_XFRM)
> > +             return lwt_xfrm_info(dst->lwtstate);
> > +
> >       return NULL;
> >  }
> >
> > diff --git a/include/uapi/linux/lwtunnel.h b/include/uapi/linux/lwtunnel.h
> > index 2e206919125c..229655ef792f 100644
> > --- a/include/uapi/linux/lwtunnel.h
> > +++ b/include/uapi/linux/lwtunnel.h
> > @@ -15,6 +15,7 @@ enum lwtunnel_encap_types {
> >       LWTUNNEL_ENCAP_SEG6_LOCAL,
> >       LWTUNNEL_ENCAP_RPL,
> >       LWTUNNEL_ENCAP_IOAM6,
> > +     LWTUNNEL_ENCAP_XFRM,
> >       __LWTUNNEL_ENCAP_MAX,
> >  };
> >
> > @@ -111,4 +112,13 @@ enum {
> >
> >  #define LWT_BPF_MAX_HEADROOM 256
> >
> > +enum {
> > +     LWT_XFRM_UNSPEC,
> > +     LWT_XFRM_IF_ID,
> > +     LWT_XFRM_LINK,
> > +     __LWT_XFRM_MAX,
> > +};
> > +
> > +#define LWT_XFRM_MAX (__LWT_XFRM_MAX - 1)
> > +
> >  #endif /* _UAPI_LWTUNNEL_H_ */
> > diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
> > index 9ccd64e8a666..6fac2f0ef074 100644
> > --- a/net/core/lwtunnel.c
> > +++ b/net/core/lwtunnel.c
> > @@ -50,6 +50,7 @@ static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type)
> >               return "IOAM6";
> >       case LWTUNNEL_ENCAP_IP6:
> >       case LWTUNNEL_ENCAP_IP:
> > +     case LWTUNNEL_ENCAP_XFRM:
> >       case LWTUNNEL_ENCAP_NONE:
> >       case __LWTUNNEL_ENCAP_MAX:
> >               /* should not have got here */
> > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> > index e9a355047468..495dee8b0764 100644
> > --- a/net/xfrm/xfrm_interface.c
> > +++ b/net/xfrm/xfrm_interface.c
> > @@ -60,6 +60,88 @@ struct xfrmi_net {
> >       struct xfrm_if __rcu *collect_md_xfrmi;
> >  };
> >
> > +static const struct nla_policy xfrm_lwt_policy[LWT_XFRM_MAX + 1] = {
> > +     [LWT_XFRM_IF_ID]        = NLA_POLICY_MIN(NLA_U32, 1),
> > +     [LWT_XFRM_LINK]         = NLA_POLICY_MIN(NLA_S32, 1),
> IMHO, it would be better to keep consistency with IFLA_XFRM_LINK.
>
> $ git grep _LINK.*NLA_U32 net/ drivers/net/
> drivers/net/gtp.c:      [GTPA_LINK]             = { .type = NLA_U32, },
> drivers/net/vxlan/vxlan_core.c: [IFLA_VXLAN_LINK]       = { .type = NLA_U32 },
> ...
> net/core/rtnetlink.c:   [IFLA_LINK]             = { .type = NLA_U32 },
> ...
> net/ipv4/ip_gre.c:      [IFLA_GRE_LINK]         = { .type = NLA_U32 },
> net/ipv4/ip_vti.c:      [IFLA_VTI_LINK]         = { .type = NLA_U32 },
> net/ipv4/ipip.c:        [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
> net/ipv6/ip6_gre.c:     [IFLA_GRE_LINK]        = { .type = NLA_U32 },
> net/ipv6/ip6_tunnel.c:  [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
> net/ipv6/ip6_vti.c:     [IFLA_VTI_LINK]         = { .type = NLA_U32 },
> net/ipv6/sit.c: [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
> net/sched/cls_u32.c:    [TCA_U32_LINK]          = { .type = NLA_U32 },
> ...
> net/xfrm/xfrm_interface.c:      [IFLA_XFRM_LINK]        = { .type = NLA_U32 },
> $ git grep _LINK.*NLA_S32 net/ drivers/net/
> net/core/rtnetlink.c:   [IFLA_LINK_NETNSID]     = { .type = NLA_S32 },
> $
>
> They all are U32. Adding one S32 would just add confusion.

Thanks for this input!

Indeed going over the other references it seems ifindex is treated as U32
when interfacing with userspace almost everywhere including netlink and
bpf. In the IOCTL interface it seems to be implemented as int, but at
least on my Ubuntu machine the manpage for e.g. if_nametoindex() describes
it as returning unsigned int.

Therefore I intend to resubmit this as U32.

Thanks,
Eyal.
Nikolay Aleksandrov Aug. 26, 2022, 10:50 a.m. UTC | #3
On 26/08/2022 13:18, Eyal Birger wrote:
> On Fri, Aug 26, 2022 at 11:05 AM Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>>
>>
>> Le 25/08/2022 à 17:46, Eyal Birger a écrit :
>>> Allow specifying the xfrm interface if_id and link as part of a route
>>> metadata using the lwtunnel infrastructure.
>>>
>>> This allows for example using a single xfrm interface in collect_md
>>> mode as the target of multiple routes each specifying a different if_id.
>>>
>>> With the appropriate changes to iproute2, considering an xfrm device
>>> ipsec1 in collect_md mode one can for example add a route specifying
>>> an if_id like so:
>>>
>>> ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1
>>>
>>> In which case traffic routed to the device via this route would use
>>> if_id in the xfrm interface policy lookup.
>>>
>>> Or in the context of vrf, one can also specify the "link" property:
>>>
>>> ip route add <SUBNET> dev ipsec1 encap xfrm if_id 1 link_dev eth15
>>>
>>> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
>>>
>>> ----
>>>
>>> v3: netlink improvements as suggested by Nikolay Aleksandrov and
>>>     Nicolas Dichtel
>>>
>>> v2:
>>>   - move lwt_xfrm_info() helper to dst_metadata.h
>>>   - add "link" property as suggested by Nicolas Dichtel
>>> ---
>>>  include/net/dst_metadata.h    | 11 +++++
>>>  include/uapi/linux/lwtunnel.h | 10 +++++
>>>  net/core/lwtunnel.c           |  1 +
>>>  net/xfrm/xfrm_interface.c     | 85 +++++++++++++++++++++++++++++++++++
>>>  4 files changed, 107 insertions(+)
>>>
>>> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
>>> index e4b059908cc7..57f75960fa28 100644
>>> --- a/include/net/dst_metadata.h
>>> +++ b/include/net/dst_metadata.h
>>> @@ -60,13 +60,24 @@ skb_tunnel_info(const struct sk_buff *skb)
>>>       return NULL;
>>>  }
>>>
>>> +static inline struct xfrm_md_info *lwt_xfrm_info(struct lwtunnel_state *lwt)
>>> +{
>>> +     return (struct xfrm_md_info *)lwt->data;
>>> +}
>>> +
>>>  static inline struct xfrm_md_info *skb_xfrm_md_info(const struct sk_buff *skb)
>>>  {
>>>       struct metadata_dst *md_dst = skb_metadata_dst(skb);
>>> +     struct dst_entry *dst;
>>>
>>>       if (md_dst && md_dst->type == METADATA_XFRM)
>>>               return &md_dst->u.xfrm_info;
>>>
>>> +     dst = skb_dst(skb);
>>> +     if (dst && dst->lwtstate &&
>>> +         dst->lwtstate->type == LWTUNNEL_ENCAP_XFRM)
>>> +             return lwt_xfrm_info(dst->lwtstate);
>>> +
>>>       return NULL;
>>>  }
>>>
>>> diff --git a/include/uapi/linux/lwtunnel.h b/include/uapi/linux/lwtunnel.h
>>> index 2e206919125c..229655ef792f 100644
>>> --- a/include/uapi/linux/lwtunnel.h
>>> +++ b/include/uapi/linux/lwtunnel.h
>>> @@ -15,6 +15,7 @@ enum lwtunnel_encap_types {
>>>       LWTUNNEL_ENCAP_SEG6_LOCAL,
>>>       LWTUNNEL_ENCAP_RPL,
>>>       LWTUNNEL_ENCAP_IOAM6,
>>> +     LWTUNNEL_ENCAP_XFRM,
>>>       __LWTUNNEL_ENCAP_MAX,
>>>  };
>>>
>>> @@ -111,4 +112,13 @@ enum {
>>>
>>>  #define LWT_BPF_MAX_HEADROOM 256
>>>
>>> +enum {
>>> +     LWT_XFRM_UNSPEC,
>>> +     LWT_XFRM_IF_ID,
>>> +     LWT_XFRM_LINK,
>>> +     __LWT_XFRM_MAX,
>>> +};
>>> +
>>> +#define LWT_XFRM_MAX (__LWT_XFRM_MAX - 1)
>>> +
>>>  #endif /* _UAPI_LWTUNNEL_H_ */
>>> diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
>>> index 9ccd64e8a666..6fac2f0ef074 100644
>>> --- a/net/core/lwtunnel.c
>>> +++ b/net/core/lwtunnel.c
>>> @@ -50,6 +50,7 @@ static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type)
>>>               return "IOAM6";
>>>       case LWTUNNEL_ENCAP_IP6:
>>>       case LWTUNNEL_ENCAP_IP:
>>> +     case LWTUNNEL_ENCAP_XFRM:
>>>       case LWTUNNEL_ENCAP_NONE:
>>>       case __LWTUNNEL_ENCAP_MAX:
>>>               /* should not have got here */
>>> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
>>> index e9a355047468..495dee8b0764 100644
>>> --- a/net/xfrm/xfrm_interface.c
>>> +++ b/net/xfrm/xfrm_interface.c
>>> @@ -60,6 +60,88 @@ struct xfrmi_net {
>>>       struct xfrm_if __rcu *collect_md_xfrmi;
>>>  };
>>>
>>> +static const struct nla_policy xfrm_lwt_policy[LWT_XFRM_MAX + 1] = {
>>> +     [LWT_XFRM_IF_ID]        = NLA_POLICY_MIN(NLA_U32, 1),
>>> +     [LWT_XFRM_LINK]         = NLA_POLICY_MIN(NLA_S32, 1),
>> IMHO, it would be better to keep consistency with IFLA_XFRM_LINK.
>>
>> $ git grep _LINK.*NLA_U32 net/ drivers/net/
>> drivers/net/gtp.c:      [GTPA_LINK]             = { .type = NLA_U32, },
>> drivers/net/vxlan/vxlan_core.c: [IFLA_VXLAN_LINK]       = { .type = NLA_U32 },
>> ...
>> net/core/rtnetlink.c:   [IFLA_LINK]             = { .type = NLA_U32 },
>> ...
>> net/ipv4/ip_gre.c:      [IFLA_GRE_LINK]         = { .type = NLA_U32 },
>> net/ipv4/ip_vti.c:      [IFLA_VTI_LINK]         = { .type = NLA_U32 },
>> net/ipv4/ipip.c:        [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
>> net/ipv6/ip6_gre.c:     [IFLA_GRE_LINK]        = { .type = NLA_U32 },
>> net/ipv6/ip6_tunnel.c:  [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
>> net/ipv6/ip6_vti.c:     [IFLA_VTI_LINK]         = { .type = NLA_U32 },
>> net/ipv6/sit.c: [IFLA_IPTUN_LINK]               = { .type = NLA_U32 },
>> net/sched/cls_u32.c:    [TCA_U32_LINK]          = { .type = NLA_U32 },
>> ...
>> net/xfrm/xfrm_interface.c:      [IFLA_XFRM_LINK]        = { .type = NLA_U32 },
>> $ git grep _LINK.*NLA_S32 net/ drivers/net/
>> net/core/rtnetlink.c:   [IFLA_LINK_NETNSID]     = { .type = NLA_S32 },
>> $
>>
>> They all are U32. Adding one S32 would just add confusion.
> 
> Thanks for this input!
> 
> Indeed going over the other references it seems ifindex is treated as U32
> when interfacing with userspace almost everywhere including netlink and
> bpf. In the IOCTL interface it seems to be implemented as int, but at
> least on my Ubuntu machine the manpage for e.g. if_nametoindex() describes
> it as returning unsigned int.
> 
> Therefore I intend to resubmit this as U32.
> 
> Thanks,
> Eyal.

Ack, good point, note that ifindex is not always a u32 and ifindex itself is usually a
signed integer field in structs (e.g. net_device), as well as flowic_oif (flowi_oif). :) 
rtnetlink.c:	[IFLA_NEW_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
rtnetlink.c:	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),

Just using the old U32 code and making link a u32 should be ok.

Cheers,
 Nik
diff mbox series

Patch

diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
index e4b059908cc7..57f75960fa28 100644
--- a/include/net/dst_metadata.h
+++ b/include/net/dst_metadata.h
@@ -60,13 +60,24 @@  skb_tunnel_info(const struct sk_buff *skb)
 	return NULL;
 }
 
+static inline struct xfrm_md_info *lwt_xfrm_info(struct lwtunnel_state *lwt)
+{
+	return (struct xfrm_md_info *)lwt->data;
+}
+
 static inline struct xfrm_md_info *skb_xfrm_md_info(const struct sk_buff *skb)
 {
 	struct metadata_dst *md_dst = skb_metadata_dst(skb);
+	struct dst_entry *dst;
 
 	if (md_dst && md_dst->type == METADATA_XFRM)
 		return &md_dst->u.xfrm_info;
 
+	dst = skb_dst(skb);
+	if (dst && dst->lwtstate &&
+	    dst->lwtstate->type == LWTUNNEL_ENCAP_XFRM)
+		return lwt_xfrm_info(dst->lwtstate);
+
 	return NULL;
 }
 
diff --git a/include/uapi/linux/lwtunnel.h b/include/uapi/linux/lwtunnel.h
index 2e206919125c..229655ef792f 100644
--- a/include/uapi/linux/lwtunnel.h
+++ b/include/uapi/linux/lwtunnel.h
@@ -15,6 +15,7 @@  enum lwtunnel_encap_types {
 	LWTUNNEL_ENCAP_SEG6_LOCAL,
 	LWTUNNEL_ENCAP_RPL,
 	LWTUNNEL_ENCAP_IOAM6,
+	LWTUNNEL_ENCAP_XFRM,
 	__LWTUNNEL_ENCAP_MAX,
 };
 
@@ -111,4 +112,13 @@  enum {
 
 #define LWT_BPF_MAX_HEADROOM 256
 
+enum {
+	LWT_XFRM_UNSPEC,
+	LWT_XFRM_IF_ID,
+	LWT_XFRM_LINK,
+	__LWT_XFRM_MAX,
+};
+
+#define LWT_XFRM_MAX (__LWT_XFRM_MAX - 1)
+
 #endif /* _UAPI_LWTUNNEL_H_ */
diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
index 9ccd64e8a666..6fac2f0ef074 100644
--- a/net/core/lwtunnel.c
+++ b/net/core/lwtunnel.c
@@ -50,6 +50,7 @@  static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type)
 		return "IOAM6";
 	case LWTUNNEL_ENCAP_IP6:
 	case LWTUNNEL_ENCAP_IP:
+	case LWTUNNEL_ENCAP_XFRM:
 	case LWTUNNEL_ENCAP_NONE:
 	case __LWTUNNEL_ENCAP_MAX:
 		/* should not have got here */
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index e9a355047468..495dee8b0764 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -60,6 +60,88 @@  struct xfrmi_net {
 	struct xfrm_if __rcu *collect_md_xfrmi;
 };
 
+static const struct nla_policy xfrm_lwt_policy[LWT_XFRM_MAX + 1] = {
+	[LWT_XFRM_IF_ID]	= NLA_POLICY_MIN(NLA_U32, 1),
+	[LWT_XFRM_LINK]		= NLA_POLICY_MIN(NLA_S32, 1),
+};
+
+static void xfrmi_destroy_state(struct lwtunnel_state *lwt)
+{
+}
+
+static int xfrmi_build_state(struct net *net, struct nlattr *nla,
+			     unsigned int family, const void *cfg,
+			     struct lwtunnel_state **ts,
+			     struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[LWT_XFRM_MAX + 1];
+	struct lwtunnel_state *new_state;
+	struct xfrm_md_info *info;
+	int ret;
+
+	ret = nla_parse_nested(tb, LWT_XFRM_MAX, nla, xfrm_lwt_policy, extack);
+	if (ret < 0)
+		return ret;
+
+	if (!tb[LWT_XFRM_IF_ID]) {
+		NL_SET_ERR_MSG(extack, "if_id must be set");
+		return -EINVAL;
+	}
+
+	new_state = lwtunnel_state_alloc(sizeof(*info));
+	if (!new_state) {
+		NL_SET_ERR_MSG(extack, "failed to create encap info");
+		return -ENOMEM;
+	}
+
+	new_state->type = LWTUNNEL_ENCAP_XFRM;
+
+	info = lwt_xfrm_info(new_state);
+
+	info->if_id = nla_get_u32(tb[LWT_XFRM_IF_ID]);
+
+	if (tb[LWT_XFRM_LINK])
+		info->link = nla_get_s32(tb[LWT_XFRM_LINK]);
+
+	*ts = new_state;
+	return 0;
+}
+
+static int xfrmi_fill_encap_info(struct sk_buff *skb,
+				 struct lwtunnel_state *lwt)
+{
+	struct xfrm_md_info *info = lwt_xfrm_info(lwt);
+
+	if (nla_put_u32(skb, LWT_XFRM_IF_ID, info->if_id) ||
+	    (info->link && nla_put_s32(skb, LWT_XFRM_LINK, info->link)))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int xfrmi_encap_nlsize(struct lwtunnel_state *lwtstate)
+{
+	return nla_total_size(sizeof(u32)) + /* LWT_XFRM_IF_ID */
+		nla_total_size(sizeof(s32)); /* LWT_XFRM_LINK */
+}
+
+static int xfrmi_encap_cmp(struct lwtunnel_state *a, struct lwtunnel_state *b)
+{
+	struct xfrm_md_info *a_info = lwt_xfrm_info(a);
+	struct xfrm_md_info *b_info = lwt_xfrm_info(b);
+
+	return memcmp(a_info, b_info, sizeof(*a_info));
+}
+
+static const struct lwtunnel_encap_ops xfrmi_encap_ops = {
+	.build_state	= xfrmi_build_state,
+	.destroy_state	= xfrmi_destroy_state,
+	.fill_encap	= xfrmi_fill_encap_info,
+	.get_encap_size = xfrmi_encap_nlsize,
+	.cmp_encap	= xfrmi_encap_cmp,
+	.owner		= THIS_MODULE,
+};
+
 #define for_each_xfrmi_rcu(start, xi) \
 	for (xi = rcu_dereference(start); xi; xi = rcu_dereference(xi->next))
 
@@ -1080,6 +1162,8 @@  static int __init xfrmi_init(void)
 	if (err < 0)
 		goto rtnl_link_failed;
 
+	lwtunnel_encap_add_ops(&xfrmi_encap_ops, LWTUNNEL_ENCAP_XFRM);
+
 	xfrm_if_register_cb(&xfrm_if_cb);
 
 	return err;
@@ -1098,6 +1182,7 @@  static int __init xfrmi_init(void)
 static void __exit xfrmi_fini(void)
 {
 	xfrm_if_unregister_cb();
+	lwtunnel_encap_del_ops(&xfrmi_encap_ops, LWTUNNEL_ENCAP_XFRM);
 	rtnl_link_unregister(&xfrmi_link_ops);
 	xfrmi4_fini();
 	xfrmi6_fini();