| Message ID | 20231013041448.8229-1-cpaasch@apple.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 503930f8e113edc86f92b767efb4ea57bdffffb2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] netlink: Correct offload_xstats size | expand |
Christoph Paasch <cpaasch@apple.com> writes: > rtnl_offload_xstats_get_size_hw_s_info_one() conditionalizes the > size-computation for IFLA_OFFLOAD_XSTATS_HW_S_INFO_USED based on whether > or not the device has offload_xstats enabled. > > However, rtnl_offload_xstats_fill_hw_s_info_one() is adding the u8 for > that field uncondtionally. > Which didn't happen prior to commit bf9f1baa279f ("net: add dedicated > kmem_cache for typical/small skb->head") as the skb always was large > enough. > > Cc: Petr Machata <petrm@nvidia.com> > Cc: Eric Dumazet <edumazet@google.com> > Fixes: 0e7788fd7622 ("net: rtnetlink: Add UAPI for obtaining L3 offload xstats") > Signed-off-by: Christoph Paasch <cpaasch@apple.com> > --- > > Notes: > Another fix would be to make rtnl_offload_xstats_fill_hw_s_info_one() > check whether the device has offload_xstats enabled. Let me know if that > is a preferred route. I think I decided that it's going to be useful to get the info always, but then neglected to update the size computation. So this fix looks good to me. Also, it maintains the same behavior as before, minus the size computation bug. Reviewed-by: Petr Machata <petrm@nvidia.com> > > net/core/rtnetlink.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 4a2ec33bfb51..53c377d054f0 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -5503,13 +5503,11 @@ static unsigned int > rtnl_offload_xstats_get_size_hw_s_info_one(const struct net_device *dev, > enum netdev_offload_xstats_type type) > { > - bool enabled = netdev_offload_xstats_enabled(dev, type); > - > return nla_total_size(0) + > /* IFLA_OFFLOAD_XSTATS_HW_S_INFO_REQUEST */ > nla_total_size(sizeof(u8)) + > /* IFLA_OFFLOAD_XSTATS_HW_S_INFO_USED */ > - (enabled ? nla_total_size(sizeof(u8)) : 0) + > + nla_total_size(sizeof(u8)) + > 0; > }
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 12 Oct 2023 21:14:48 -0700 you wrote: > rtnl_offload_xstats_get_size_hw_s_info_one() conditionalizes the > size-computation for IFLA_OFFLOAD_XSTATS_HW_S_INFO_USED based on whether > or not the device has offload_xstats enabled. > > However, rtnl_offload_xstats_fill_hw_s_info_one() is adding the u8 for > that field uncondtionally. > > [...] Here is the summary with links: - [net] netlink: Correct offload_xstats size https://git.kernel.org/netdev/net/c/503930f8e113 You are awesome, thank you!
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 4a2ec33bfb51..53c377d054f0 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -5503,13 +5503,11 @@ static unsigned int rtnl_offload_xstats_get_size_hw_s_info_one(const struct net_device *dev, enum netdev_offload_xstats_type type) { - bool enabled = netdev_offload_xstats_enabled(dev, type); - return nla_total_size(0) + /* IFLA_OFFLOAD_XSTATS_HW_S_INFO_REQUEST */ nla_total_size(sizeof(u8)) + /* IFLA_OFFLOAD_XSTATS_HW_S_INFO_USED */ - (enabled ? nla_total_size(sizeof(u8)) : 0) + + nla_total_size(sizeof(u8)) + 0; }
rtnl_offload_xstats_get_size_hw_s_info_one() conditionalizes the size-computation for IFLA_OFFLOAD_XSTATS_HW_S_INFO_USED based on whether or not the device has offload_xstats enabled. However, rtnl_offload_xstats_fill_hw_s_info_one() is adding the u8 for that field uncondtionally. syzkaller triggered a WARNING in rtnl_stats_get due to this: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 754 at net/core/rtnetlink.c:5982 rtnl_stats_get+0x2f4/0x300 Modules linked in: CPU: 0 PID: 754 Comm: syz-executor148 Not tainted 6.6.0-rc2-g331b78eb12af #45 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 RIP: 0010:rtnl_stats_get+0x2f4/0x300 net/core/rtnetlink.c:5982 Code: ff ff 89 ee e8 7d 72 50 ff 83 fd a6 74 17 e8 33 6e 50 ff 4c 89 ef be 02 00 00 00 e8 86 00 fa ff e9 7b fe ff ff e8 1c 6e 50 ff <0f> 0b eb e5 e8 73 79 7b 00 0f 1f 00 90 90 90 90 90 90 90 90 90 90 RSP: 0018:ffffc900006837c0 EFLAGS: 00010293 RAX: ffffffff81cf7f24 RBX: ffff8881015d9000 RCX: ffff888101815a00 RDX: 0000000000000000 RSI: 00000000ffffffa6 RDI: 00000000ffffffa6 RBP: 00000000ffffffa6 R08: ffffffff81cf7f03 R09: 0000000000000001 R10: ffff888101ba47b9 R11: ffff888101815a00 R12: ffff8881017dae00 R13: ffff8881017dad00 R14: ffffc90000683ab8 R15: ffffffff83c1f740 FS: 00007fbc22dbc740(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000046 CR3: 000000010264e003 CR4: 0000000000170ef0 Call Trace: <TASK> rtnetlink_rcv_msg+0x677/0x710 net/core/rtnetlink.c:6480 netlink_rcv_skb+0xea/0x1c0 net/netlink/af_netlink.c:2545 netlink_unicast+0x430/0x500 net/netlink/af_netlink.c:1342 netlink_sendmsg+0x4fc/0x620 net/netlink/af_netlink.c:1910 sock_sendmsg+0xa8/0xd0 net/socket.c:730 ____sys_sendmsg+0x22a/0x320 net/socket.c:2541 ___sys_sendmsg+0x143/0x190 net/socket.c:2595 __x64_sys_sendmsg+0xd8/0x150 net/socket.c:2624 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x47/0xa0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x6e/0xd8 RIP: 0033:0x7fbc22e8d6a9 Code: 5c c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4f 37 0d 00 f7 d8 64 89 01 48 RSP: 002b:00007ffc4320e778 EFLAGS: 00000246 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 00000000004007d0 RCX: 00007fbc22e8d6a9 RDX: 0000000000000000 RSI: 0000000020000000 RDI: 0000000000000003 RBP: 0000000000000001 R08: 0000000000000000 R09: 00000000004007d0 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffc4320e898 R13: 00007ffc4320e8a8 R14: 00000000004004a0 R15: 00007fbc22fa5a80 </TASK> ---[ end trace 0000000000000000 ]--- Which didn't happen prior to commit bf9f1baa279f ("net: add dedicated kmem_cache for typical/small skb->head") as the skb always was large enough. Cc: Petr Machata <petrm@nvidia.com> Cc: Eric Dumazet <edumazet@google.com> Fixes: 0e7788fd7622 ("net: rtnetlink: Add UAPI for obtaining L3 offload xstats") Signed-off-by: Christoph Paasch <cpaasch@apple.com> --- Notes: Another fix would be to make rtnl_offload_xstats_fill_hw_s_info_one() check whether the device has offload_xstats enabled. Let me know if that is a preferred route. net/core/rtnetlink.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)