lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89i+q_0e3ztiHD5YE4LBJCSeaETk3VyJ0TPuJYP9By1_1Tg@mail.gmail.com>
Date: Sat, 30 Sep 2023 17:29:50 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Alce Lafranque <alce@...ranque.net>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, Ido Schimmel <idosch@...dia.com>, 
	netdev@...r.kernel.org, vincent@...nat.ch
Subject: Re: [PATCH net-next] vxlan: add support for flowlabel inherit

On Sat, Sep 30, 2023 at 5:13 PM Alce Lafranque <alce@...ranque.net> wrote:
>
> By default, VXLAN encapsulation over IPv6 sets the flow label to 0, with an
> option for a fixed value. This commits add the ability to inherit the flow
> label from the inner packet, like for other tunnel implementations.
>
> ```
> $ ./ip/ip addr add 2001:db8::2/64 dev dummy1
> $ ./ip/ip link set up dev dummy1
> $ ./ip/ip link add vxlan1 type vxlan id 100 flowlabel inherit remote 2001:db8::1 local 2001:db8::2

Side question : How can "flowlabel inherit" can be turned off later
with an "ip link change ..." ?

It seems vxlan_nl2flag() would always turn it 'on' for NLA_FLAG type :

if (vxlan_policy[attrtype].type == NLA_FLAG)
    flags = conf->flags | mask;  // always turn on
else if (nla_get_u8(tb[attrtype]))    // dead code for NLA_FLAG
    flags = conf->flags | mask;
else
    flags = conf->flags & ~mask;

conf->flags = flags;


> $ ./ip/ip link set up dev vxlan1
> $ ./ip/ip addr add 2001:db8:1::2/64 dev vxlan1
> $ ./ip/ip link set arp off dev vxlan1
> $ ping -q 2001:db8:1::1 &
> $ tshark -d udp.port==8472,vxlan -Vpni dummy1 -c1
> [...]
> Internet Protocol Version 6, Src: 2001:db8::2, Dst: 2001:db8::1
>     0110 .... = Version: 6
>     .... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
>     .... 1011 0001 1010 1111 1011 = Flow Label: 0xb1afb
> [...]
> Virtual eXtensible Local Area Network
>     Flags: 0x0800, VXLAN Network ID (VNI)
>     Group Policy ID: 0
>     VXLAN Network Identifier (VNI): 100
> [...]
> Internet Protocol Version 6, Src: 2001:db8:1::2, Dst: 2001:db8:1::1
>     0110 .... = Version: 6
>     .... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
>     .... 1011 0001 1010 1111 1011 = Flow Label: 0xb1afb
> ```
>
> Signed-off-by: Alce Lafranque <alce@...ranque.net>
> Co-developed-by: Vincent Bernat <vincent@...nat.ch>
> Signed-off-by: Vincent Bernat <vincent@...nat.ch>
> ---
>  drivers/net/vxlan/vxlan_core.c | 20 ++++++++++++++++++--
>  include/net/ip_tunnels.h       | 11 +++++++++++
>  include/net/vxlan.h            |  2 ++
>  include/uapi/linux/if_link.h   |  1 +
>  4 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 5b5597073b00..aa7fbfdd93b1 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2475,7 +2475,11 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>                 else
>                         udp_sum = !(flags & VXLAN_F_UDP_ZERO_CSUM6_TX);
>  #if IS_ENABLED(CONFIG_IPV6)
> -               label = vxlan->cfg.label;
> +               if (flags & VXLAN_F_LABEL_INHERIT) {
> +                       label = ip_tunnel_get_flowlabel(old_iph, skb);
> +               } else {
> +                       label = vxlan->cfg.label;
> +               }

You can remove the braces.

>  #endif
>         } else {
>                 if (!info) {
> @@ -3286,6 +3290,7 @@ static const struct nla_policy vxlan_policy[IFLA_VXLAN_MAX + 1] = {
>         [IFLA_VXLAN_DF]         = { .type = NLA_U8 },
>         [IFLA_VXLAN_VNIFILTER]  = { .type = NLA_U8 },
>         [IFLA_VXLAN_LOCALBYPASS]        = NLA_POLICY_MAX(NLA_U8, 1),
> +       [IFLA_VXLAN_LABEL_INHERIT]      = { .type = NLA_FLAG },
>  };
>
>  static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -4001,7 +4006,15 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
>
>         if (data[IFLA_VXLAN_LABEL])
>                 conf->label = nla_get_be32(data[IFLA_VXLAN_LABEL]) &
> -                            IPV6_FLOWLABEL_MASK;
> +                             IPV6_FLOWLABEL_MASK;
> +
> +       if (data[IFLA_VXLAN_LABEL_INHERIT]) {
> +               err = vxlan_nl2flag(conf, data, IFLA_VXLAN_LABEL_INHERIT,
> +                                   VXLAN_F_LABEL_INHERIT, changelink, false,
> +                                   extack);
> +               if (err)
> +                       return err;
> +       }
>
>         if (data[IFLA_VXLAN_LEARNING]) {
>                 err = vxlan_nl2flag(conf, data, IFLA_VXLAN_LEARNING,
> @@ -4315,6 +4328,7 @@ static size_t vxlan_get_size(const struct net_device *dev)
>                 nla_total_size(sizeof(__u8)) +  /* IFLA_VXLAN_TOS */
>                 nla_total_size(sizeof(__u8)) +  /* IFLA_VXLAN_DF */
>                 nla_total_size(sizeof(__be32)) + /* IFLA_VXLAN_LABEL */
> +               nla_total_size(sizeof(__u8)) +  /* IFLA_VXLAN_LABEL_INHERIT */
>                 nla_total_size(sizeof(__u8)) +  /* IFLA_VXLAN_LEARNING */
>                 nla_total_size(sizeof(__u8)) +  /* IFLA_VXLAN_PROXY */
>                 nla_total_size(sizeof(__u8)) +  /* IFLA_VXLAN_RSC */
> @@ -4387,6 +4401,8 @@ static int vxlan_fill_info(struct sk_buff *skb, const struct net_device *dev)
>             nla_put_u8(skb, IFLA_VXLAN_TOS, vxlan->cfg.tos) ||
>             nla_put_u8(skb, IFLA_VXLAN_DF, vxlan->cfg.df) ||
>             nla_put_be32(skb, IFLA_VXLAN_LABEL, vxlan->cfg.label) ||
> +           nla_put_u8(skb, IFLA_VXLAN_LABEL_INHERIT,
> +                      !!(vxlan->cfg.flags & VXLAN_F_LABEL_INHERIT)) ||

This seems in contradiction with NLA_FLAG semantics if the flag can
not be turned off.

Look for nla_put_flag(). User space could get confused.

>             nla_put_u8(skb, IFLA_VXLAN_LEARNING,
>                        !!(vxlan->cfg.flags & VXLAN_F_LEARN)) ||
>             nla_put_u8(skb, IFLA_VXLAN_PROXY,
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ