[PATCHv3 net] bonding: fix missed rcu protection

[PATCHv3 net] bonding: fix missed rcu protection

[Hangbin Liu]

When removing the rcu_read_lock in bond_ethtool_get_ts_info() as
discussed [1], I didn't notice it could be called via setsockopt,
which doesn't hold rcu lock, as syzbot pointed:

  stack backtrace:
  CPU: 0 PID: 3599 Comm: syz-executor317 Not tainted 5.18.0-rc5-syzkaller-01392-g01f4685797a5 #0
  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
  Call Trace:
   <TASK>
   __dump_stack lib/dump_stack.c:88 [inline]
   dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
   bond_option_active_slave_get_rcu include/net/bonding.h:353 [inline]
   bond_ethtool_get_ts_info+0x32c/0x3a0 drivers/net/bonding/bond_main.c:5595
   __ethtool_get_ts_info+0x173/0x240 net/ethtool/common.c:554
   ethtool_get_phc_vclocks+0x99/0x110 net/ethtool/common.c:568
   sock_timestamping_bind_phc net/core/sock.c:869 [inline]
   sock_set_timestamping+0x3a3/0x7e0 net/core/sock.c:916
   sock_setsockopt+0x543/0x2ec0 net/core/sock.c:1221
   __sys_setsockopt+0x55e/0x6a0 net/socket.c:2223
   __do_sys_setsockopt net/socket.c:2238 [inline]
   __se_sys_setsockopt net/socket.c:2235 [inline]
   __x64_sys_setsockopt+0xba/0x150 net/socket.c:2235
   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
   do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
   entry_SYSCALL_64_after_hwframe+0x44/0xae
  RIP: 0033:0x7f8902c8eb39

Fix it by adding rcu_read_lock and take a ref on the real_dev.
Since dev_hold() and dev_put() can take NULL these days, we can
skip checking if real_dev exist.

[1] https://lore.kernel.org/netdev/27565.1642742439@famine/

Reported-by: syzbot+92beb3d46aab498710fa@syzkaller.appspotmail.com
Fixes: aa6034678e87 ("bonding: use rcu_dereference_rtnl when get bonding active slave")
Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v3: skip checking if real_dev exist since dev_hold/put could take NULL.
v2: add ref on the real_dev as Jakub and Paolo suggested.
---
 drivers/net/bonding/bond_main.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 38e152548126..b5c5196e03ee 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5591,16 +5591,23 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 	const struct ethtool_ops *ops;
 	struct net_device *real_dev;
 	struct phy_device *phydev;
+	int ret = 0;
 
+	rcu_read_lock();
 	real_dev = bond_option_active_slave_get_rcu(bond);
+	dev_hold(real_dev);
+	rcu_read_unlock();
+
 	if (real_dev) {
 		ops = real_dev->ethtool_ops;
 		phydev = real_dev->phydev;
 
 		if (phy_has_tsinfo(phydev)) {
-			return phy_ts_info(phydev, info);
+			ret = phy_ts_info(phydev, info);
+			goto out;
 		} else if (ops->get_ts_info) {
-			return ops->get_ts_info(real_dev, info);
+			ret = ops->get_ts_info(real_dev, info);
+			goto out;
 		}
 	}
 
@@ -5608,7 +5615,9 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 				SOF_TIMESTAMPING_SOFTWARE;
 	info->phc_index = -1;
 
-	return 0;
+out:
+	dev_put(real_dev);
+	return ret;
 }
 
 static const struct ethtool_ops bond_ethtool_ops = {
-- 
2.35.1

Re: [PATCHv3 net] bonding: fix missed rcu protection

[Vladimir Oltean]

On Thu, May 19, 2022 at 10:01:48AM +0800, Hangbin Liu wrote:
> When removing the rcu_read_lock in bond_ethtool_get_ts_info() as
> discussed [1], I didn't notice it could be called via setsockopt,
> which doesn't hold rcu lock, as syzbot pointed:
> 
>   stack backtrace:
>   CPU: 0 PID: 3599 Comm: syz-executor317 Not tainted 5.18.0-rc5-syzkaller-01392-g01f4685797a5 #0
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>   Call Trace:
>    <TASK>
>    __dump_stack lib/dump_stack.c:88 [inline]
>    dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>    bond_option_active_slave_get_rcu include/net/bonding.h:353 [inline]
>    bond_ethtool_get_ts_info+0x32c/0x3a0 drivers/net/bonding/bond_main.c:5595
>    __ethtool_get_ts_info+0x173/0x240 net/ethtool/common.c:554
>    ethtool_get_phc_vclocks+0x99/0x110 net/ethtool/common.c:568
>    sock_timestamping_bind_phc net/core/sock.c:869 [inline]
>    sock_set_timestamping+0x3a3/0x7e0 net/core/sock.c:916
>    sock_setsockopt+0x543/0x2ec0 net/core/sock.c:1221
>    __sys_setsockopt+0x55e/0x6a0 net/socket.c:2223
>    __do_sys_setsockopt net/socket.c:2238 [inline]
>    __se_sys_setsockopt net/socket.c:2235 [inline]
>    __x64_sys_setsockopt+0xba/0x150 net/socket.c:2235
>    do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>    do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
>   RIP: 0033:0x7f8902c8eb39
> 
> Fix it by adding rcu_read_lock and take a ref on the real_dev.
> Since dev_hold() and dev_put() can take NULL these days, we can
> skip checking if real_dev exist.
> 
> [1] https://lore.kernel.org/netdev/27565.1642742439@famine/
> 
> Reported-by: syzbot+92beb3d46aab498710fa@syzkaller.appspotmail.com
> Fixes: aa6034678e87 ("bonding: use rcu_dereference_rtnl when get bonding active slave")
> Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCHv3 net] bonding: fix missed rcu protection

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 19 May 2022 10:01:48 +0800 you wrote:
> When removing the rcu_read_lock in bond_ethtool_get_ts_info() as
> discussed [1], I didn't notice it could be called via setsockopt,
> which doesn't hold rcu lock, as syzbot pointed:
> 
>   stack backtrace:
>   CPU: 0 PID: 3599 Comm: syz-executor317 Not tainted 5.18.0-rc5-syzkaller-01392-g01f4685797a5 #0
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>   Call Trace:
>    <TASK>
>    __dump_stack lib/dump_stack.c:88 [inline]
>    dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>    bond_option_active_slave_get_rcu include/net/bonding.h:353 [inline]
>    bond_ethtool_get_ts_info+0x32c/0x3a0 drivers/net/bonding/bond_main.c:5595
>    __ethtool_get_ts_info+0x173/0x240 net/ethtool/common.c:554
>    ethtool_get_phc_vclocks+0x99/0x110 net/ethtool/common.c:568
>    sock_timestamping_bind_phc net/core/sock.c:869 [inline]
>    sock_set_timestamping+0x3a3/0x7e0 net/core/sock.c:916
>    sock_setsockopt+0x543/0x2ec0 net/core/sock.c:1221
>    __sys_setsockopt+0x55e/0x6a0 net/socket.c:2223
>    __do_sys_setsockopt net/socket.c:2238 [inline]
>    __se_sys_setsockopt net/socket.c:2235 [inline]
>    __x64_sys_setsockopt+0xba/0x150 net/socket.c:2235
>    do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>    do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
>   RIP: 0033:0x7f8902c8eb39
> 
> [...]

Here is the summary with links:
  - [PATCHv3,net] bonding: fix missed rcu protection
    https://git.kernel.org/netdev/net/c/9b80ccda233f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html