* [PATCH net] bonding: do not assume skb mac_header is set
@ 2023-06-22 15:23 Eric Dumazet
2023-06-22 17:48 ` Jay Vosburgh
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Eric Dumazet @ 2023-06-22 15:23 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet, Eric Dumazet, syzbot, Jarod Wilson,
Moshe Tal, Jussi Maki, Jay Vosburgh, Andy Gospodarek,
Vladimir Oltean
Drivers must not assume in their ndo_start_xmit() that
skbs have their mac_header set. skb->data is all what is needed.
bonding seems to be one of the last offender as caught by syzbot:
WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
Modules linked in:
CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Moshe Tal <moshet@nvidia.com>
Cc: Jussi Maki <joamaki@gmail.com>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/bonding/bond_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
return skb->hash;
return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
- skb_mac_offset(skb), skb_network_offset(skb),
+ 0, skb_network_offset(skb),
skb_headlen(skb));
}
--
2.41.0.178.g377b9f9a00-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 15:23 [PATCH net] bonding: do not assume skb mac_header is set Eric Dumazet
@ 2023-06-22 17:48 ` Jay Vosburgh
2023-06-22 18:32 ` Eric Dumazet
2023-06-23 11:07 ` Jiri Pirko
2023-06-24 2:10 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 11+ messages in thread
From: Jay Vosburgh @ 2023-06-22 17:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Andy Gospodarek, Vladimir Oltean
Eric Dumazet <edumazet@google.com> wrote:
>Drivers must not assume in their ndo_start_xmit() that
>skbs have their mac_header set. skb->data is all what is needed.
>
>bonding seems to be one of the last offender as caught by syzbot:
>
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>Modules linked in:
>CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
>RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
>RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
>RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
>RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
>RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
>RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
>RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
>R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
>R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
>FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>Call Trace:
><TASK>
>[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
>[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
>[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
>[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
>[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
>[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
>[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
>[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
>[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
>[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
>[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
>[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
>[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
>[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
>Reported-by: syzbot <syzkaller@googlegroups.com>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Jarod Wilson <jarod@redhat.com>
>Cc: Moshe Tal <moshet@nvidia.com>
>Cc: Jussi Maki <joamaki@gmail.com>
>Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
>---
> drivers/net/bonding/bond_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> return skb->hash;
>
> return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>- skb_mac_offset(skb), skb_network_offset(skb),
>+ 0, skb_network_offset(skb),
> skb_headlen(skb));
> }
Is the MAC header guaranteed to be at skb->data, then? If not,
then isn't replacing skb_mac_offset() with 0 going to break the hash (as
it might or might not be looking at the actual MAC header)?
Also, assuming for the moment that this change is ok, this makes
all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
and a complete fix should therefore remove the unused parameter and its
various references.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 17:48 ` Jay Vosburgh
@ 2023-06-22 18:32 ` Eric Dumazet
2023-06-22 18:55 ` Jay Vosburgh
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2023-06-22 18:32 UTC (permalink / raw)
To: Jay Vosburgh
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Andy Gospodarek, Vladimir Oltean
On Thu, Jun 22, 2023 at 7:48 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> Eric Dumazet <edumazet@google.com> wrote:
>
> >Drivers must not assume in their ndo_start_xmit() that
> >skbs have their mac_header set. skb->data is all what is needed.
> >
> >bonding seems to be one of the last offender as caught by syzbot:
> >
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
> >Modules linked in:
> >CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
> >Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
> >RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
> >RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
> >RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
> >RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
> >RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
> >RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
> >RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
> >Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
> >RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
> >RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
> >RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
> >RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
> >R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
> >R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
> >FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
> >CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
> >DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >Call Trace:
> ><TASK>
> >[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
> >[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
> >[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
> >[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
> >[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
> >[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
> >[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
> >[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
> >[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
> >[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
> >[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
> >[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
> >[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
> >[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
> >Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
> >Reported-by: syzbot <syzkaller@googlegroups.com>
> >Signed-off-by: Eric Dumazet <edumazet@google.com>
> >Cc: Jarod Wilson <jarod@redhat.com>
> >Cc: Moshe Tal <moshet@nvidia.com>
> >Cc: Jussi Maki <joamaki@gmail.com>
> >Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> >Cc: Andy Gospodarek <andy@greyhouse.net>
> >Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> >---
> > drivers/net/bonding/bond_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> > return skb->hash;
> >
> > return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
> >- skb_mac_offset(skb), skb_network_offset(skb),
> >+ 0, skb_network_offset(skb),
> > skb_headlen(skb));
> > }
>
> Is the MAC header guaranteed to be at skb->data, then? If not,
> then isn't replacing skb_mac_offset() with 0 going to break the hash (as
> it might or might not be looking at the actual MAC header)?
>
In ndo_start_xmit(), skb->data points to MAC header by definition.
> Also, assuming for the moment that this change is ok, this makes
> all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
> and a complete fix should therefore remove the unused parameter and its
> various references.
Not really: bond_xmit_hash_xdp() calls __bond_xmit_hash() with
sizeof(struct ethhdr)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 18:32 ` Eric Dumazet
@ 2023-06-22 18:55 ` Jay Vosburgh
2023-06-22 19:00 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Jay Vosburgh @ 2023-06-22 18:55 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Andy Gospodarek, Vladimir Oltean
Eric Dumazet <edumazet@google.com> wrote:
>On Thu, Jun 22, 2023 at 7:48 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>>
>> Eric Dumazet <edumazet@google.com> wrote:
[...]
>> > drivers/net/bonding/bond_main.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>> > return skb->hash;
>> >
>> > return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>> >- skb_mac_offset(skb), skb_network_offset(skb),
>> >+ 0, skb_network_offset(skb),
>> > skb_headlen(skb));
>> > }
>>
>> Is the MAC header guaranteed to be at skb->data, then? If not,
>> then isn't replacing skb_mac_offset() with 0 going to break the hash (as
>> it might or might not be looking at the actual MAC header)?
>>
>
>In ndo_start_xmit(), skb->data points to MAC header by definition.
Ok.
>> Also, assuming for the moment that this change is ok, this makes
>> all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
>> and a complete fix should therefore remove the unused parameter and its
>> various references.
>
>Not really: bond_xmit_hash_xdp() calls __bond_xmit_hash() with
>sizeof(struct ethhdr)
I don't think so:
static u32 __bond_xmit_hash(struct bonding *bond, struct sk_buff *skb, const void *data,
__be16 l2_proto, int mhoff, int nhoff, int hlen)
{
"mhoff", currently supplied as skb_mac_offset(skb) in
bond_xmit_hash(), is the fifth parameter.
static u32 bond_xmit_hash_xdp(struct bonding *bond, struct xdp_buff *xdp)
{
[...]
return __bond_xmit_hash(bond, NULL, xdp->data, eth->h_proto, 0,
sizeof(struct ethhdr), xdp->data_end - xdp->data);
}
The fifth argument here is 0.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 18:55 ` Jay Vosburgh
@ 2023-06-22 19:00 ` Eric Dumazet
2023-06-22 19:01 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2023-06-22 19:00 UTC (permalink / raw)
To: Jay Vosburgh
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Andy Gospodarek, Vladimir Oltean
On Thu, Jun 22, 2023 at 8:55 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> Eric Dumazet <edumazet@google.com> wrote:
>
> >On Thu, Jun 22, 2023 at 7:48 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> >>
> >> Eric Dumazet <edumazet@google.com> wrote:
>
> [...]
>
> >> > drivers/net/bonding/bond_main.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
> >> >--- a/drivers/net/bonding/bond_main.c
> >> >+++ b/drivers/net/bonding/bond_main.c
> >> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> >> > return skb->hash;
> >> >
> >> > return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
> >> >- skb_mac_offset(skb), skb_network_offset(skb),
> >> >+ 0, skb_network_offset(skb),
> >> > skb_headlen(skb));
> >> > }
> >>
> >> Is the MAC header guaranteed to be at skb->data, then? If not,
> >> then isn't replacing skb_mac_offset() with 0 going to break the hash (as
> >> it might or might not be looking at the actual MAC header)?
> >>
> >
> >In ndo_start_xmit(), skb->data points to MAC header by definition.
>
> Ok.
>
> >> Also, assuming for the moment that this change is ok, this makes
> >> all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
> >> and a complete fix should therefore remove the unused parameter and its
> >> various references.
> >
> >Not really: bond_xmit_hash_xdp() calls __bond_xmit_hash() with
> >sizeof(struct ethhdr)
>
> I don't think so:
>
> static u32 __bond_xmit_hash(struct bonding *bond, struct sk_buff *skb, const void *data,
> __be16 l2_proto, int mhoff, int nhoff, int hlen)
> {
>
> "mhoff", currently supplied as skb_mac_offset(skb) in
> bond_xmit_hash(), is the fifth parameter.
>
> static u32 bond_xmit_hash_xdp(struct bonding *bond, struct xdp_buff *xdp)
> {
> [...]
> return __bond_xmit_hash(bond, NULL, xdp->data, eth->h_proto, 0,
> sizeof(struct ethhdr), xdp->data_end - xdp->data);
> }
>
> The fifth argument here is 0.
>
Ah right, I will send another patch to remove it then.
I think it makes sense to keep the first patch small for backports.
History of relevant patches :
from 5.17
429e3d123d9a50cc9882402e40e0ac912d88cfcf bonding: Fix extraction of
ports from the packet headers
from 5.15
a815bde56b15ce626caaacc952ab12501671e45d net, bonding: Refactor
bond_xmit_hash for use with xdp_buff
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 19:00 ` Eric Dumazet
@ 2023-06-22 19:01 ` Eric Dumazet
2023-06-22 19:31 ` Jay Vosburgh
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2023-06-22 19:01 UTC (permalink / raw)
To: Jay Vosburgh
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Andy Gospodarek, Vladimir Oltean
On Thu, Jun 22, 2023 at 9:00 PM Eric Dumazet <edumazet@google.com> wrote:
>
> Ah right, I will send another patch to remove it then.
>
> I think it makes sense to keep the first patch small for backports.
>
> History of relevant patches :
>
> from 5.17
>
> 429e3d123d9a50cc9882402e40e0ac912d88cfcf bonding: Fix extraction of
> ports from the packet headers
>
> from 5.15
>
> a815bde56b15ce626caaacc952ab12501671e45d net, bonding: Refactor
> bond_xmit_hash for use with xdp_buff
If this is ok for you, I will cook this cleanup patch for net-next.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 19:01 ` Eric Dumazet
@ 2023-06-22 19:31 ` Jay Vosburgh
0 siblings, 0 replies; 11+ messages in thread
From: Jay Vosburgh @ 2023-06-22 19:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Andy Gospodarek, Vladimir Oltean
Eric Dumazet <edumazet@google.com> wrote:
>On Thu, Jun 22, 2023 at 9:00 PM Eric Dumazet <edumazet@google.com> wrote:
>>
>
>> Ah right, I will send another patch to remove it then.
>>
>> I think it makes sense to keep the first patch small for backports.
>>
>> History of relevant patches :
>>
>> from 5.17
>>
>> 429e3d123d9a50cc9882402e40e0ac912d88cfcf bonding: Fix extraction of
>> ports from the packet headers
>>
>> from 5.15
>>
>> a815bde56b15ce626caaacc952ab12501671e45d net, bonding: Refactor
>> bond_xmit_hash for use with xdp_buff
>
>If this is ok for you, I will cook this cleanup patch for net-next.
Yes, this sounds fine to me.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 15:23 [PATCH net] bonding: do not assume skb mac_header is set Eric Dumazet
2023-06-22 17:48 ` Jay Vosburgh
@ 2023-06-23 11:07 ` Jiri Pirko
2023-06-23 14:28 ` Eric Dumazet
2023-06-24 2:10 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2023-06-23 11:07 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Jay Vosburgh, Andy Gospodarek, Vladimir Oltean
Thu, Jun 22, 2023 at 05:23:04PM CEST, edumazet@google.com wrote:
>Drivers must not assume in their ndo_start_xmit() that
>skbs have their mac_header set. skb->data is all what is needed.
>
>bonding seems to be one of the last offender as caught by syzbot:
>
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>Modules linked in:
>CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
>RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
>RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
>RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
>RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
>RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
>RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
>RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
>R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
>R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
>FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
>CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>Call Trace:
><TASK>
>[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
>[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
>[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
>[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
>[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
>[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
>[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
>[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
>[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
>[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
>[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
>[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
>[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
>[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
>Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
>Reported-by: syzbot <syzkaller@googlegroups.com>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Jarod Wilson <jarod@redhat.com>
>Cc: Moshe Tal <moshet@nvidia.com>
>Cc: Jussi Maki <joamaki@gmail.com>
>Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
>---
> drivers/net/bonding/bond_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> return skb->hash;
>
> return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>- skb_mac_offset(skb), skb_network_offset(skb),
>+ 0, skb_network_offset(skb),
After this change, both callers of __bond_xmit_hash() pass 0 as mhoff.
Wouldn't it make sense to remove this arg entirely here and in
bond_vlan_srcmac_hash() and bond_eth_hash()?
> skb_headlen(skb));
> }
>
>--
>2.41.0.178.g377b9f9a00-goog
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-23 11:07 ` Jiri Pirko
@ 2023-06-23 14:28 ` Eric Dumazet
2023-06-23 15:04 ` Jiri Pirko
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2023-06-23 14:28 UTC (permalink / raw)
To: Jiri Pirko
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Jay Vosburgh, Andy Gospodarek, Vladimir Oltean
On Fri, Jun 23, 2023 at 1:07 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Jun 22, 2023 at 05:23:04PM CEST, edumazet@google.com wrote:
> >Drivers must not assume in their ndo_start_xmit() that
> >skbs have their mac_header set. skb->data is all what is needed.
> >
> >bonding seems to be one of the last offender as caught by syzbot:
> >
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
> >Modules linked in:
> >CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
> >Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
> >RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
> >RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
> >RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
> >RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
> >RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
> >RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
> >RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
> >Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
> >RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
> >RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
> >RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
> >RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
> >R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
> >R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
> >FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
> >CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
> >DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >Call Trace:
> ><TASK>
> >[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
> >[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
> >[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
> >[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
> >[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
> >[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
> >[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
> >[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
> >[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
> >[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
> >[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
> >[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
> >[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
> >[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >
> >Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
> >Reported-by: syzbot <syzkaller@googlegroups.com>
> >Signed-off-by: Eric Dumazet <edumazet@google.com>
> >Cc: Jarod Wilson <jarod@redhat.com>
> >Cc: Moshe Tal <moshet@nvidia.com>
> >Cc: Jussi Maki <joamaki@gmail.com>
> >Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> >Cc: Andy Gospodarek <andy@greyhouse.net>
> >Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> >---
> > drivers/net/bonding/bond_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> > return skb->hash;
> >
> > return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
> >- skb_mac_offset(skb), skb_network_offset(skb),
> >+ 0, skb_network_offset(skb),
>
> After this change, both callers of __bond_xmit_hash() pass 0 as mhoff.
> Wouldn't it make sense to remove this arg entirely here and in
> bond_vlan_srcmac_hash() and bond_eth_hash()?
>
>
> > skb_headlen(skb));
> > }
Yes, this was mentioned by Jay yesterday, and we agreed to remove the
parameter on net-next.
The reason for that is that stable teams might have a hard time,
because of the various commits
in different linux versions that heavily changed this code.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-23 14:28 ` Eric Dumazet
@ 2023-06-23 15:04 ` Jiri Pirko
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2023-06-23 15:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jarod Wilson, Moshe Tal, Jussi Maki,
Jay Vosburgh, Andy Gospodarek, Vladimir Oltean
Fri, Jun 23, 2023 at 04:28:16PM CEST, edumazet@google.com wrote:
>On Fri, Jun 23, 2023 at 1:07 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Jun 22, 2023 at 05:23:04PM CEST, edumazet@google.com wrote:
>> >Drivers must not assume in their ndo_start_xmit() that
>> >skbs have their mac_header set. skb->data is all what is needed.
>> >
>> >bonding seems to be one of the last offender as caught by syzbot:
>> >
>> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
>> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>> >WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>> >Modules linked in:
>> >CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
>> >Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
>> >RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
>> >RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
>> >RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
>> >RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
>> >RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
>> >RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
>> >RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
>> >Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
>> >RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
>> >RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
>> >RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
>> >RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
>> >R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
>> >R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
>> >FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
>> >CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> >CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
>> >DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> >DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> >Call Trace:
>> ><TASK>
>> >[<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
>> >[<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
>> >[<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
>> >[<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
>> >[<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
>> >[<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
>> >[<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
>> >[<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
>> >[<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
>> >[<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
>> >[<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
>> >[<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
>> >[<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> >[<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
>> >[<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> >
>> >Fixes: 7b8fc0103bb5 ("bonding: add a vlan+srcmac tx hashing option")
>> >Reported-by: syzbot <syzkaller@googlegroups.com>
>> >Signed-off-by: Eric Dumazet <edumazet@google.com>
>> >Cc: Jarod Wilson <jarod@redhat.com>
>> >Cc: Moshe Tal <moshet@nvidia.com>
>> >Cc: Jussi Maki <joamaki@gmail.com>
>> >Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>> >Cc: Andy Gospodarek <andy@greyhouse.net>
>> >Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
>> >---
>> > drivers/net/bonding/bond_main.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>> > return skb->hash;
>> >
>> > return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>> >- skb_mac_offset(skb), skb_network_offset(skb),
>> >+ 0, skb_network_offset(skb),
>>
>> After this change, both callers of __bond_xmit_hash() pass 0 as mhoff.
>> Wouldn't it make sense to remove this arg entirely here and in
>> bond_vlan_srcmac_hash() and bond_eth_hash()?
>>
>>
>> > skb_headlen(skb));
>> > }
>
>Yes, this was mentioned by Jay yesterday, and we agreed to remove the
>parameter on net-next.
Yeah, I noticed that after I hit send button. :/
>
>The reason for that is that stable teams might have a hard time,
>because of the various commits
>in different linux versions that heavily changed this code.
Sounds good. Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net] bonding: do not assume skb mac_header is set
2023-06-22 15:23 [PATCH net] bonding: do not assume skb mac_header is set Eric Dumazet
2023-06-22 17:48 ` Jay Vosburgh
2023-06-23 11:07 ` Jiri Pirko
@ 2023-06-24 2:10 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-06-24 2:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, kuba, pabeni, netdev, eric.dumazet, syzkaller, jarod,
moshet, joamaki, j.vosburgh, andy, vladimir.oltean
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 22 Jun 2023 15:23:04 +0000 you wrote:
> Drivers must not assume in their ndo_start_xmit() that
> skbs have their mac_header set. skb->data is all what is needed.
>
> bonding seems to be one of the last offender as caught by syzbot:
>
> WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 skb_mac_offset include/linux/skbuff.h:2913 [inline]
> WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
> WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
> WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
> WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 __bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
> WARNING: CPU: 1 PID: 12155 at include/linux/skbuff.h:2907 bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
> Modules linked in:
> CPU: 1 PID: 12155 Comm: syz-executor.3 Not tainted 6.1.30-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
> RIP: 0010:skb_mac_header include/linux/skbuff.h:2907 [inline]
> RIP: 0010:skb_mac_offset include/linux/skbuff.h:2913 [inline]
> RIP: 0010:bond_xmit_hash drivers/net/bonding/bond_main.c:4170 [inline]
> RIP: 0010:bond_xmit_3ad_xor_slave_get drivers/net/bonding/bond_main.c:5149 [inline]
> RIP: 0010:bond_3ad_xor_xmit drivers/net/bonding/bond_main.c:5186 [inline]
> RIP: 0010:__bond_start_xmit drivers/net/bonding/bond_main.c:5442 [inline]
> RIP: 0010:bond_start_xmit+0x14ab/0x19d0 drivers/net/bonding/bond_main.c:5470
> Code: 8b 7c 24 30 e8 76 dd 1a 01 48 85 c0 74 0d 48 89 c3 e8 29 67 2e fe e9 15 ef ff ff e8 1f 67 2e fe e9 10 ef ff ff e8 15 67 2e fe <0f> 0b e9 45 f8 ff ff e8 09 67 2e fe e9 dc fa ff ff e8 ff 66 2e fe
> RSP: 0018:ffffc90002fff6e0 EFLAGS: 00010283
> RAX: ffffffff835874db RBX: 000000000000ffff RCX: 0000000000040000
> RDX: ffffc90004dcf000 RSI: 00000000000000b5 RDI: 00000000000000b6
> RBP: ffffc90002fff8b8 R08: ffffffff83586d16 R09: ffffffff83586584
> R10: 0000000000000007 R11: ffff8881599fc780 R12: ffff88811b6a7b7e
> R13: 1ffff110236d4f6f R14: ffff88811b6a7ac0 R15: 1ffff110236d4f76
> FS: 00007f2e9eb47700(0000) GS:ffff8881f6b00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b2e421000 CR3: 000000010e6d4000 CR4: 00000000003526e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> [<ffffffff8471a49f>] netdev_start_xmit include/linux/netdevice.h:4925 [inline]
> [<ffffffff8471a49f>] __dev_direct_xmit+0x4ef/0x850 net/core/dev.c:4380
> [<ffffffff851d845b>] dev_direct_xmit include/linux/netdevice.h:3043 [inline]
> [<ffffffff851d845b>] packet_direct_xmit+0x18b/0x300 net/packet/af_packet.c:284
> [<ffffffff851c7472>] packet_snd net/packet/af_packet.c:3112 [inline]
> [<ffffffff851c7472>] packet_sendmsg+0x4a22/0x64d0 net/packet/af_packet.c:3143
> [<ffffffff8467a4b2>] sock_sendmsg_nosec net/socket.c:716 [inline]
> [<ffffffff8467a4b2>] sock_sendmsg net/socket.c:736 [inline]
> [<ffffffff8467a4b2>] __sys_sendto+0x472/0x5f0 net/socket.c:2139
> [<ffffffff8467a715>] __do_sys_sendto net/socket.c:2151 [inline]
> [<ffffffff8467a715>] __se_sys_sendto net/socket.c:2147 [inline]
> [<ffffffff8467a715>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147
> [<ffffffff8553071f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> [<ffffffff8553071f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80
> [<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> [...]
Here is the summary with links:
- [net] bonding: do not assume skb mac_header is set
https://git.kernel.org/netdev/net/c/6a940abdef31
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-06-24 2:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-22 15:23 [PATCH net] bonding: do not assume skb mac_header is set Eric Dumazet
2023-06-22 17:48 ` Jay Vosburgh
2023-06-22 18:32 ` Eric Dumazet
2023-06-22 18:55 ` Jay Vosburgh
2023-06-22 19:00 ` Eric Dumazet
2023-06-22 19:01 ` Eric Dumazet
2023-06-22 19:31 ` Jay Vosburgh
2023-06-23 11:07 ` Jiri Pirko
2023-06-23 14:28 ` Eric Dumazet
2023-06-23 15:04 ` Jiri Pirko
2023-06-24 2:10 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.