[PATCH RESEND net] bonding: fix missed rcu protection

[PATCH RESEND net] bonding: fix missed rcu protection

[Hangbin Liu]

When removing the rcu_read_lock in bond_ethtool_get_ts_info(), 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 during the whole slave dev get_ts_info period.

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>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---

Sorry, I hit send too quick in last patch and forgot to add net prefix.
So resend the patch in this mail.

---
 drivers/net/bonding/bond_main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 38e152548126..3a6f879ad7a9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5591,16 +5591,20 @@ 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);
 	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 +5612,9 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 				SOF_TIMESTAMPING_SOFTWARE;
 	info->phc_index = -1;
 
-	return 0;
+out:
+	rcu_read_unlock();
+	return ret;
 }
 
 static const struct ethtool_ops bond_ethtool_ops = {
-- 
2.35.1

Re: [PATCH RESEND net] bonding: fix missed rcu protection

[Jonathan Toppins]

On 5/13/22 06:33, Hangbin Liu wrote:
> When removing the rcu_read_lock in bond_ethtool_get_ts_info(), 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 during the whole slave dev get_ts_info period.
> 
> 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>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
Acked-by: Jonathan Toppins <jtoppins@redhat.com>

Re: [PATCH RESEND net] bonding: fix missed rcu protection

[Vladimir Oltean]

On Fri, May 13, 2022 at 06:33:50PM +0800, Hangbin Liu wrote:
> When removing the rcu_read_lock in bond_ethtool_get_ts_info(), 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 during the whole slave dev get_ts_info period.
> 
> 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>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

General feedback, the whole stack trace kind of clutters the commit
message and you could trim most of it. Otherwise the patch looks good to me.

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

Re: [PATCH RESEND net] bonding: fix missed rcu protection

[Jakub Kicinski]

On Fri, 13 May 2022 18:33:50 +0800 Hangbin Liu wrote:
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 38e152548126..3a6f879ad7a9 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5591,16 +5591,20 @@ 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);
>  	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;
>  		}
>  	}

Can't ->get_ts_info sleep now? It'd be a little sad to force it 
to be atomic just because of one upper dev trying to be fancy.
Maybe all we need to do is to take a ref on the real_dev?

Also please add a Link: to the previous discussion, it'd have been
useful to get the context in which Vladimir suggested this.

Re: [PATCH RESEND net] bonding: fix missed rcu protection

[Hangbin Liu]

On Mon, May 16, 2022 at 06:10:28PM -0700, Jakub Kicinski wrote:
> Can't ->get_ts_info sleep now? It'd be a little sad to force it 
> to be atomic just because of one upper dev trying to be fancy.
> Maybe all we need to do is to take a ref on the real_dev?

Do you mean

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 38e152548126..b60450211579 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5591,16 +5591,20 @@ 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;
 
 	real_dev = bond_option_active_slave_get_rcu(bond);
 	if (real_dev) {
+		dev_hold(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 +5612,10 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 				SOF_TIMESTAMPING_SOFTWARE;
 	info->phc_index = -1;
 
-	return 0;
+out:
+	if (real_dev)
+		dev_put(real_dev);
+	return ret;
 }


This look OK to me.

Vladimir, Jay, WDYT?

> 
> Also please add a Link: to the previous discussion, it'd have been
> useful to get the context in which Vladimir suggested this.

OK, I will.

Thanks
Hangbin

Re: [PATCH RESEND net] bonding: fix missed rcu protection

[Paolo Abeni]

On Tue, 2022-05-17 at 11:42 +0800, Hangbin Liu wrote:
> On Mon, May 16, 2022 at 06:10:28PM -0700, Jakub Kicinski wrote:
> > Can't ->get_ts_info sleep now? It'd be a little sad to force it 
> > to be atomic just because of one upper dev trying to be fancy.
> > Maybe all we need to do is to take a ref on the real_dev?
> 
> Do you mean
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 38e152548126..b60450211579 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5591,16 +5591,20 @@ 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;
>  

You additionally need something alike the following:

	rcu_read_lock();
>  	real_dev = bond_option_active_slave_get_rcu(bond);
>  	if (real_dev) {
> +		dev_hold(real_dev)
		rcu_read_unlock();

>  		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;
>  		}
	} else {
		rcu_read_unlock();
>  	}

... or you will hit the initial RCU splat. Overall this will not put
atomicy constraint on get_ts_info.

Cheers,

Paol

>  
> @@ -5608,7 +5612,10 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>  				SOF_TIMESTAMPING_SOFTWARE;
>  	info->phc_index = -1;
>  
> -	return 0;
> +out:
> +	if (real_dev)
> +		dev_put(real_dev);
> +	return ret;
>  }
> 
> 
> This look OK to me.
> 
> Vladimir, Jay, WDYT?
> 
> > 
> > Also please add a Link: to the previous discussion, it'd have been
> > useful to get the context in which Vladimir suggested this.
> 
> OK, I will.
> 
> Thanks
> Hangbin
> 

Re: [PATCH RESEND net] bonding: fix missed rcu protection

[Hangbin Liu]

On Tue, May 17, 2022 at 09:24:00AM +0200, Paolo Abeni wrote:
> On Tue, 2022-05-17 at 11:42 +0800, Hangbin Liu wrote:
> > On Mon, May 16, 2022 at 06:10:28PM -0700, Jakub Kicinski wrote:
> > > Can't ->get_ts_info sleep now? It'd be a little sad to force it 
> > > to be atomic just because of one upper dev trying to be fancy.
> > > Maybe all we need to do is to take a ref on the real_dev?
> > 
> > Do you mean
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 38e152548126..b60450211579 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5591,16 +5591,20 @@ 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;
> >  
> 
> You additionally need something alike the following:
> 
> 	rcu_read_lock();

Thanks Paolo, I only thought the real_dev ref and forgot the initial problem
is the rcu warning...

Hangbin