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

[Stefano Brivio <sbrivio@redhat.com>]

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