netdev - Re: [syzbot] WARNING: suspicious RCU usage in bond_ethtool_get_ts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220513084819.zrg4ssnw667rhndt@skbuf>
Date:   Fri, 13 May 2022 08:48:20 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Hangbin Liu <liuhangbin@...il.com>
CC:     Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "syzkaller-bugs@...glegroups.com" <syzkaller-bugs@...glegroups.com>,
        syzbot <syzbot+92beb3d46aab498710fa@...kaller.appspotmail.com>
Subject: Re: [syzbot] WARNING: suspicious RCU usage in
 bond_ethtool_get_ts_info

On Fri, May 13, 2022 at 09:51:34AM +0800, Hangbin Liu wrote:
> Remove bpf guys from cc list.
> 
> On Thu, May 12, 2022 at 12:35:28PM -0700, syzbot wrote:
> > 
> > The issue was bisected to:
> > 
> > commit aa6034678e873db8bd5c5a4b73f8b88c469374d6
> > Author: Hangbin Liu <liuhangbin@...il.com>
> > Date:   Fri Jan 21 08:25:18 2022 +0000
> > 
> >     bonding: use rcu_dereference_rtnl when get bonding active slave
> > 
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=16fce349f00000
> > final oops:     https://syzkaller.appspot.com/x/report.txt?x=15fce349f00000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=11fce349f00000
> > 
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+92beb3d46aab498710fa@...kaller.appspotmail.com
> > Fixes: aa6034678e87 ("bonding: use rcu_dereference_rtnl when get bonding active slave")
> > 
> > =============================
> > WARNING: suspicious RCU usage
> > 5.18.0-rc5-syzkaller-01392-g01f4685797a5 #0 Not tainted
> > -----------------------------
> > include/net/bonding.h:353 suspicious rcu_dereference_check() usage!
> > 
> > other info that might help us debug this:
> > 
> > 
> > rcu_scheduler_active = 2, debug_locks = 1
> > 1 lock held by syz-executor317/3599:
> >  #0: ffff88801de78130 (sk_lock-AF_INET){+.+.}-{0:0}, at: lock_sock include/net/sock.h:1680 [inline]
> >  #0: ffff88801de78130 (sk_lock-AF_INET){+.+.}-{0:0}, at: sock_setsockopt+0x1e3/0x2ec0 net/core/sock.c:1066
> > 
> > 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
> 
> Oh, I forgot to check setsockopt path...
> 
> Hi Vladimir, Jay,
> 
> Do you think if I should revert my previous commit, or just add
> rcu_read_lock()/rcu_read_unlock() back to bond_ethtool_get_ts_info()?
> 
> Thanks
> Hangbin

Hi Hangbin,

sock_timestamping_bind_phc() appears to run unlocked, with the exception
of the rcu_read_lock() in dev_get_by_index() in which there is a dev_hold().
I'm thinking that this dev_hold ensures that the bonding interface does
not go away, but it still does not ensure that the active slave does not
go away.

I only looked superficially at this because I am AFK today, but I think
I would put rcu_read_lock() in bond_ethtool_get_ts_info(), but not the
way in which it was before, but rather for the entire time during which
real_dev, ops and phydev are being dereferenced.