[PATCH net] netlink: Correct offload_xstats size

[PATCH net] netlink: Correct offload_xstats size

[Christoph Paasch]

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(-)

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;
 }
 
-- 
2.32.3 (Apple Git-135)

Re: [PATCH net] netlink: Correct offload_xstats size

[Petr Machata]

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;
>  }

Re: [PATCH net] netlink: Correct offload_xstats size

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html