On Tue, 10 Nov 2020 16:16:40 -0800 Yi-Hung Wei wrote:
> Currently, we may set the tunnel option flag when the size of metadata
> is zero. For example, we set TUNNEL_GENEVE_OPT in the receive function
> no matter the geneve option is present or not. As this may result in
> issues on the tunnel flags consumers, this patch fixes the issue.
>
> Related discussion:
> *
> https://lore.kernel.org/netdev/1604448694-19351-1-git-send-email-yihung....@gmail.com/T/#u
>
> Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags")
> Signed-off-by: Yi-Hung Wei <yihung....@gmail.com>
Seems fine to me, however BPF (and maybe Netfilter?) can set options
passed by user without checking if they are 0 length.
Daniel, Pablo, are you okay with this change or should we limit it to
just fixing the GENEVE oddness?
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index d07008a818df..1426bfc009bc 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -224,8 +224,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct
> geneve_sock *gs,
> if (ip_tunnel_collect_metadata() || gs->collect_md) {
> __be16 flags;
>
> - flags = TUNNEL_KEY | TUNNEL_GENEVE_OPT |
> - (gnvh->oam ? TUNNEL_OAM : 0) |
> + flags = TUNNEL_KEY | (gnvh->oam ? TUNNEL_OAM : 0) |
> (gnvh->critical ? TUNNEL_CRIT_OPT : 0);
>
> tun_dst = udp_tun_rx_dst(skb, geneve_get_sk_family(gs), flags,
For the minimal fix we'd just need the change above, plus:
- ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
- gnvh->options, gnvh->opt_len * 4,
- TUNNEL_GENEVE_OPT);
+ if (gnvh->opt_len)
+ ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
+ gnvh->options,
+ gnvh->opt_len * 4,
+ TUNNEL_GENEVE_OPT);
Right?
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 02ccd32542d0..61620677b034 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -478,9 +478,11 @@ static inline void ip_tunnel_info_opts_set(struct
> ip_tunnel_info *info,
> const void *from, int len,
> __be16 flags)
> {
> - memcpy(ip_tunnel_info_opts(info), from, len);
> info->options_len = len;
> - info->key.tun_flags |= flags;
> + if (len > 0) {
> + memcpy(ip_tunnel_info_opts(info), from, len);
> + info->key.tun_flags |= flags;
> + }
> }
>
> static inline struct ip_tunnel_info *lwt_tun_info(struct lwtunnel_state
> *lwtstate)
> @@ -526,7 +528,6 @@ static inline void ip_tunnel_info_opts_set(struct
> ip_tunnel_info *info,
> __be16 flags)
> {
> info->options_len = 0;
> - info->key.tun_flags |= flags;
> }
>
> #endif /* CONFIG_INET */
