diff mbox series

[net] geneve: do not assume mac header is set in geneve_xmit_skb()

Message ID 20241203182122.2725517-1-edumazet@google.com (mailing list archive)
State Accepted
Commit 8588c99c7d47448fcae39e3227d6e2bb97aad86d
Delegated to: Netdev Maintainers
Headers show
Series [net] geneve: do not assume mac header is set in geneve_xmit_skb() | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: sd@queasysnail.net; 2 maintainers not CCed: andrew+netdev@lunn.ch sd@queasysnail.net
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 304 this patch: 304
netdev/checkpatch warning WARNING: Possible repeated word: 'Google'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-04--15-02 (tests: 760)

Commit Message

Eric Dumazet Dec. 3, 2024, 6:21 p.m. UTC 
We should not assume mac header is set in output path.

Use skb_eth_hdr() instead of eth_hdr() to fix the issue.

sysbot reported the following :

 WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 skb_mac_header include/linux/skbuff.h:3052 [inline]
 WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 eth_hdr include/linux/if_ether.h:24 [inline]
 WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit_skb drivers/net/geneve.c:898 [inline]
 WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
Modules linked in:
CPU: 0 UID: 0 PID: 11635 Comm: syz.4.1423 Not tainted 6.12.0-syzkaller-10296-gaaf20f870da0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
 RIP: 0010:skb_mac_header include/linux/skbuff.h:3052 [inline]
 RIP: 0010:eth_hdr include/linux/if_ether.h:24 [inline]
 RIP: 0010:geneve_xmit_skb drivers/net/geneve.c:898 [inline]
 RIP: 0010:geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
Code: 21 c6 02 e9 35 d4 ff ff e8 a5 48 4c fb 90 0f 0b 90 e9 fd f5 ff ff e8 97 48 4c fb 90 0f 0b 90 e9 d8 f5 ff ff e8 89 48 4c fb 90 <0f> 0b 90 e9 41 e4 ff ff e8 7b 48 4c fb 90 0f 0b 90 e9 cd e7 ff ff
RSP: 0018:ffffc90003b2f870 EFLAGS: 00010283
RAX: 000000000000037a RBX: 000000000000ffff RCX: ffffc9000dc3d000
RDX: 0000000000080000 RSI: ffffffff86428417 RDI: 0000000000000003
RBP: ffffc90003b2f9f0 R08: 0000000000000003 R09: 000000000000ffff
R10: 000000000000ffff R11: 0000000000000002 R12: ffff88806603c000
R13: 0000000000000000 R14: ffff8880685b2780 R15: 0000000000000e23
FS:  00007fdc2deed6c0(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b30a1dff8 CR3: 0000000056b8c000 CR4: 00000000003526f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
  __netdev_start_xmit include/linux/netdevice.h:5002 [inline]
  netdev_start_xmit include/linux/netdevice.h:5011 [inline]
  __dev_direct_xmit+0x58a/0x720 net/core/dev.c:4490
  dev_direct_xmit include/linux/netdevice.h:3181 [inline]
  packet_xmit+0x1e4/0x360 net/packet/af_packet.c:285
  packet_snd net/packet/af_packet.c:3146 [inline]
  packet_sendmsg+0x2700/0x5660 net/packet/af_packet.c:3178
  sock_sendmsg_nosec net/socket.c:711 [inline]
  __sock_sendmsg net/socket.c:726 [inline]
  __sys_sendto+0x488/0x4f0 net/socket.c:2197
  __do_sys_sendto net/socket.c:2204 [inline]
  __se_sys_sendto net/socket.c:2200 [inline]
  __x64_sys_sendto+0xe0/0x1c0 net/socket.c:2200
  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
  do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
Reported-by: syzbot+3ec5271486d7cb2d242a@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/674f4b72.050a0220.17bd51.004a.GAE@google.com/T/#u
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Stefano Brivio <sbrivio@redhat.com>
---
 drivers/net/geneve.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefano Brivio Dec. 3, 2024, 11:42 p.m. UTC | #1 
Hi,

On Tue,  3 Dec 2024 18:21:21 +0000
Eric Dumazet <edumazet@google.com> wrote:

> We should not assume mac header is set in output path.
> 
> Use skb_eth_hdr() instead of eth_hdr() to fix the issue.
> 
> sysbot reported the following :
> 
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 skb_mac_header include/linux/skbuff.h:3052 [inline]
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 eth_hdr include/linux/if_ether.h:24 [inline]
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit_skb drivers/net/geneve.c:898 [inline]
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
> Modules linked in:
> CPU: 0 UID: 0 PID: 11635 Comm: syz.4.1423 Not tainted 6.12.0-syzkaller-10296-gaaf20f870da0 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
>  RIP: 0010:skb_mac_header include/linux/skbuff.h:3052 [inline]
>  RIP: 0010:eth_hdr include/linux/if_ether.h:24 [inline]
>  RIP: 0010:geneve_xmit_skb drivers/net/geneve.c:898 [inline]
>  RIP: 0010:geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
> Code: 21 c6 02 e9 35 d4 ff ff e8 a5 48 4c fb 90 0f 0b 90 e9 fd f5 ff ff e8 97 48 4c fb 90 0f 0b 90 e9 d8 f5 ff ff e8 89 48 4c fb 90 <0f> 0b 90 e9 41 e4 ff ff e8 7b 48 4c fb 90 0f 0b 90 e9 cd e7 ff ff
> RSP: 0018:ffffc90003b2f870 EFLAGS: 00010283
> RAX: 000000000000037a RBX: 000000000000ffff RCX: ffffc9000dc3d000
> RDX: 0000000000080000 RSI: ffffffff86428417 RDI: 0000000000000003
> RBP: ffffc90003b2f9f0 R08: 0000000000000003 R09: 000000000000ffff
> R10: 000000000000ffff R11: 0000000000000002 R12: ffff88806603c000
> R13: 0000000000000000 R14: ffff8880685b2780 R15: 0000000000000e23
> FS:  00007fdc2deed6c0(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b30a1dff8 CR3: 0000000056b8c000 CR4: 00000000003526f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>   __netdev_start_xmit include/linux/netdevice.h:5002 [inline]
>   netdev_start_xmit include/linux/netdevice.h:5011 [inline]
>   __dev_direct_xmit+0x58a/0x720 net/core/dev.c:4490
>   dev_direct_xmit include/linux/netdevice.h:3181 [inline]
>   packet_xmit+0x1e4/0x360 net/packet/af_packet.c:285
>   packet_snd net/packet/af_packet.c:3146 [inline]
>   packet_sendmsg+0x2700/0x5660 net/packet/af_packet.c:3178
>   sock_sendmsg_nosec net/socket.c:711 [inline]
>   __sock_sendmsg net/socket.c:726 [inline]
>   __sys_sendto+0x488/0x4f0 net/socket.c:2197
>   __do_sys_sendto net/socket.c:2204 [inline]
>   __se_sys_sendto net/socket.c:2200 [inline]
>   __x64_sys_sendto+0xe0/0x1c0 net/socket.c:2200
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f

Oops. Thanks for looking into this.

> Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
> Reported-by: syzbot+3ec5271486d7cb2d242a@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/674f4b72.050a0220.17bd51.004a.GAE@google.com/T/#u
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stefano Brivio <sbrivio@redhat.com>
> ---
>  drivers/net/geneve.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 2f29b1386b1c81640562e6ce91d6e8d88f0ffe1c..bc658bc6088546d5d1f116988b93d4dda915a799 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -895,7 +895,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
>  		if (geneve->cfg.df == GENEVE_DF_SET) {
>  			df = htons(IP_DF);
>  		} else if (geneve->cfg.df == GENEVE_DF_INHERIT) {
> -			struct ethhdr *eth = eth_hdr(skb);
> +			struct ethhdr *eth = skb_eth_hdr(skb);

Now, while your patch clearly looks better than the alternative, I
wonder: if skb->mac_header is not set...

>  			if (ntohs(eth->h_proto) == ETH_P_IPV6) {
>  				df = htons(IP_DF);

does eth->h_proto contain anything meaningful at this point? Is
there a more robust way to check for the IP version of the
encapsulated packet (assuming it's IP at all)?

Or should we rather *not* touch 'df' at all if
!skb_mac_header_was_set(skb)?

Unless you have the answers, give me some time to check that.
Stefano Brivio Dec. 4, 2024, 12:12 p.m. UTC | #2 
On Wed, 4 Dec 2024 00:42:28 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> Hi,
> 
> On Tue,  3 Dec 2024 18:21:21 +0000
> Eric Dumazet <edumazet@google.com> wrote:
> 
> > We should not assume mac header is set in output path.
> > 
> > Use skb_eth_hdr() instead of eth_hdr() to fix the issue.
> > 
> > sysbot reported the following :
> > 
> >  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 skb_mac_header include/linux/skbuff.h:3052 [inline]
> >  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 eth_hdr include/linux/if_ether.h:24 [inline]
> >  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit_skb drivers/net/geneve.c:898 [inline]
> >  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
> > Modules linked in:
> > CPU: 0 UID: 0 PID: 11635 Comm: syz.4.1423 Not tainted 6.12.0-syzkaller-10296-gaaf20f870da0 #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
> >  RIP: 0010:skb_mac_header include/linux/skbuff.h:3052 [inline]
> >  RIP: 0010:eth_hdr include/linux/if_ether.h:24 [inline]
> >  RIP: 0010:geneve_xmit_skb drivers/net/geneve.c:898 [inline]
> >  RIP: 0010:geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
> > Code: 21 c6 02 e9 35 d4 ff ff e8 a5 48 4c fb 90 0f 0b 90 e9 fd f5 ff ff e8 97 48 4c fb 90 0f 0b 90 e9 d8 f5 ff ff e8 89 48 4c fb 90 <0f> 0b 90 e9 41 e4 ff ff e8 7b 48 4c fb 90 0f 0b 90 e9 cd e7 ff ff
> > RSP: 0018:ffffc90003b2f870 EFLAGS: 00010283
> > RAX: 000000000000037a RBX: 000000000000ffff RCX: ffffc9000dc3d000
> > RDX: 0000000000080000 RSI: ffffffff86428417 RDI: 0000000000000003
> > RBP: ffffc90003b2f9f0 R08: 0000000000000003 R09: 000000000000ffff
> > R10: 000000000000ffff R11: 0000000000000002 R12: ffff88806603c000
> > R13: 0000000000000000 R14: ffff8880685b2780 R15: 0000000000000e23
> > FS:  00007fdc2deed6c0(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000001b30a1dff8 CR3: 0000000056b8c000 CR4: 00000000003526f0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> >  <TASK>
> >   __netdev_start_xmit include/linux/netdevice.h:5002 [inline]
> >   netdev_start_xmit include/linux/netdevice.h:5011 [inline]
> >   __dev_direct_xmit+0x58a/0x720 net/core/dev.c:4490
> >   dev_direct_xmit include/linux/netdevice.h:3181 [inline]
> >   packet_xmit+0x1e4/0x360 net/packet/af_packet.c:285
> >   packet_snd net/packet/af_packet.c:3146 [inline]
> >   packet_sendmsg+0x2700/0x5660 net/packet/af_packet.c:3178
> >   sock_sendmsg_nosec net/socket.c:711 [inline]
> >   __sock_sendmsg net/socket.c:726 [inline]
> >   __sys_sendto+0x488/0x4f0 net/socket.c:2197
> >   __do_sys_sendto net/socket.c:2204 [inline]
> >   __se_sys_sendto net/socket.c:2200 [inline]
> >   __x64_sys_sendto+0xe0/0x1c0 net/socket.c:2200
> >   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> >   do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
> >  entry_SYSCALL_64_after_hwframe+0x77/0x7f  
> 
> Oops. Thanks for looking into this.
> 
> > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
> > Reported-by: syzbot+3ec5271486d7cb2d242a@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/netdev/674f4b72.050a0220.17bd51.004a.GAE@google.com/T/#u
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  drivers/net/geneve.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> > index 2f29b1386b1c81640562e6ce91d6e8d88f0ffe1c..bc658bc6088546d5d1f116988b93d4dda915a799 100644
> > --- a/drivers/net/geneve.c
> > +++ b/drivers/net/geneve.c
> > @@ -895,7 +895,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
> >  		if (geneve->cfg.df == GENEVE_DF_SET) {
> >  			df = htons(IP_DF);
> >  		} else if (geneve->cfg.df == GENEVE_DF_INHERIT) {
> > -			struct ethhdr *eth = eth_hdr(skb);
> > +			struct ethhdr *eth = skb_eth_hdr(skb);  
> 
> Now, while your patch clearly looks better than the alternative, I
> wonder: if skb->mac_header is not set...
> 
> >  			if (ntohs(eth->h_proto) == ETH_P_IPV6) {
> >  				df = htons(IP_DF);  
> 
> does eth->h_proto contain anything meaningful at this point?

At a second look: yes, it should always have the Ethertype, because you
can't encapsulate anything that doesn't over GENEVE. At this point, if
it's IPv4 or IPv6, we'll set DF, and otherwise we leave it unaffected.

Thanks for fixing this.

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
patchwork-bot+netdevbpf@kernel.org Dec. 5, 2024, 3:30 a.m. UTC | #3 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  3 Dec 2024 18:21:21 +0000 you wrote:
> We should not assume mac header is set in output path.
> 
> Use skb_eth_hdr() instead of eth_hdr() to fix the issue.
> 
> sysbot reported the following :
> 
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 skb_mac_header include/linux/skbuff.h:3052 [inline]
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 eth_hdr include/linux/if_ether.h:24 [inline]
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit_skb drivers/net/geneve.c:898 [inline]
>  WARNING: CPU: 0 PID: 11635 at include/linux/skbuff.h:3052 geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
> Modules linked in:
> CPU: 0 UID: 0 PID: 11635 Comm: syz.4.1423 Not tainted 6.12.0-syzkaller-10296-gaaf20f870da0 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/13/2024
>  RIP: 0010:skb_mac_header include/linux/skbuff.h:3052 [inline]
>  RIP: 0010:eth_hdr include/linux/if_ether.h:24 [inline]
>  RIP: 0010:geneve_xmit_skb drivers/net/geneve.c:898 [inline]
>  RIP: 0010:geneve_xmit+0x4c38/0x5730 drivers/net/geneve.c:1039
> Code: 21 c6 02 e9 35 d4 ff ff e8 a5 48 4c fb 90 0f 0b 90 e9 fd f5 ff ff e8 97 48 4c fb 90 0f 0b 90 e9 d8 f5 ff ff e8 89 48 4c fb 90 <0f> 0b 90 e9 41 e4 ff ff e8 7b 48 4c fb 90 0f 0b 90 e9 cd e7 ff ff
> RSP: 0018:ffffc90003b2f870 EFLAGS: 00010283
> RAX: 000000000000037a RBX: 000000000000ffff RCX: ffffc9000dc3d000
> RDX: 0000000000080000 RSI: ffffffff86428417 RDI: 0000000000000003
> RBP: ffffc90003b2f9f0 R08: 0000000000000003 R09: 000000000000ffff
> R10: 000000000000ffff R11: 0000000000000002 R12: ffff88806603c000
> R13: 0000000000000000 R14: ffff8880685b2780 R15: 0000000000000e23
> FS:  00007fdc2deed6c0(0000) GS:ffff8880b8600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b30a1dff8 CR3: 0000000056b8c000 CR4: 00000000003526f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>   __netdev_start_xmit include/linux/netdevice.h:5002 [inline]
>   netdev_start_xmit include/linux/netdevice.h:5011 [inline]
>   __dev_direct_xmit+0x58a/0x720 net/core/dev.c:4490
>   dev_direct_xmit include/linux/netdevice.h:3181 [inline]
>   packet_xmit+0x1e4/0x360 net/packet/af_packet.c:285
>   packet_snd net/packet/af_packet.c:3146 [inline]
>   packet_sendmsg+0x2700/0x5660 net/packet/af_packet.c:3178
>   sock_sendmsg_nosec net/socket.c:711 [inline]
>   __sock_sendmsg net/socket.c:726 [inline]
>   __sys_sendto+0x488/0x4f0 net/socket.c:2197
>   __do_sys_sendto net/socket.c:2204 [inline]
>   __se_sys_sendto net/socket.c:2200 [inline]
>   __x64_sys_sendto+0xe0/0x1c0 net/socket.c:2200
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> [...]

Here is the summary with links:
  - [net] geneve: do not assume mac header is set in geneve_xmit_skb()
    https://git.kernel.org/netdev/net/c/8588c99c7d47

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 2f29b1386b1c81640562e6ce91d6e8d88f0ffe1c..bc658bc6088546d5d1f116988b93d4dda915a799 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -895,7 +895,7 @@  static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		if (geneve->cfg.df == GENEVE_DF_SET) {
 			df = htons(IP_DF);
 		} else if (geneve->cfg.df == GENEVE_DF_INHERIT) {
-			struct ethhdr *eth = eth_hdr(skb);
+			struct ethhdr *eth = skb_eth_hdr(skb);
 
 			if (ntohs(eth->h_proto) == ETH_P_IPV6) {
 				df = htons(IP_DF);