| Message ID | 1605053800-74072-1-git-send-email-yihung.wei@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ip_tunnels: Set tunnel option flag when tunnel metadata is present | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 432 this patch: 432 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 29 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 458 this patch: 458 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Nov 10, 2020 at 4:17 PM Yi-Hung Wei <yihung.wei@gmail.com> 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.wei@gmail.com/T/#u > > Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags") > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > --- Sorry that I did not indicate in the subject line of the previous email. It should be "[PATCH net]". -Yi-Hung
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.wei@gmail.com/T/#u > > Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags") > Signed-off-by: Yi-Hung Wei <yihung.wei@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 */
On 11/14/20 1:13 AM, Jakub Kicinski wrote: > 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.wei@gmail.com/T/#u >> >> Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags") >> Signed-off-by: Yi-Hung Wei <yihung.wei@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? Verifier will guarantee that buffer passed into helper is > 0, so seems okay from BPF side. Thanks, Daniel
On Sat, 14 Nov 2020 01:46:04 +0100 Daniel Borkmann wrote: > On 11/14/20 1:13 AM, Jakub Kicinski wrote: > > 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.wei@gmail.com/T/#u > >> > >> Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags") > >> Signed-off-by: Yi-Hung Wei <yihung.wei@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? > > Verifier will guarantee that buffer passed into helper is > 0, so seems > okay from BPF side. Okay then, the potential change for netfilter is limited to GENEVE so doesn't move the needle. Applied, thanks!
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, 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 */
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.wei@gmail.com/T/#u Fixes: 256c87c17c53 ("net: check tunnel option type in tunnel flags") Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- drivers/net/geneve.c | 3 +-- include/net/ip_tunnels.h | 7 ++++--- 2 files changed, 5 insertions(+), 5 deletions(-)