diff mbox series

ip_tunnels: Set tunnel option flag when tunnel metadata is present

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

Checks

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

Commit Message

Yi-Hung Wei Nov. 11, 2020, 12:16 a.m. UTC
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(-)

Comments

Yi-Hung Wei Nov. 11, 2020, 12:25 a.m. UTC | #1
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
Jakub Kicinski Nov. 14, 2020, 12:13 a.m. UTC | #2
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 */
Daniel Borkmann Nov. 14, 2020, 12:46 a.m. UTC | #3
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
Jakub Kicinski Nov. 14, 2020, 1:01 a.m. UTC | #4
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 mbox series

Patch

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 */