| 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
| ||
|
Message-ID: <2649494.1721337149@famine>
Date: Thu, 18 Jul 2024 14:12:29 -0700
From: Jay Vosburgh <jv@...sburgh.net>
To: Johannes Berg <johannes@...solutions.net>
cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
Johannes Berg <johannes.berg@...el.com>,
syzbot+2120b9a8f96b3fa90bad@...kaller.appspotmail.com
Subject: Re: [RFC PATCH 2/2] net: bonding: don't call ethtool methods under
RCU
Johannes Berg <johannes@...solutions.net> wrote:
>From: Johannes Berg <johannes.berg@...el.com>
>
>Currently, bond_miimon_inspect() is called under RCU, but it
>calls ethtool ops. Since my earlier commit facd15dfd691
>("net: core: synchronize link-watch when carrier is queried")
>this is no longer permitted in the general ethtool case, but
>it was already not permitted for many drivers such as USB in
>which it can sleep to do MDIO register accesses etc.
>
>Therefore, it's better to simply not do this. Change bonding
>to acquire the RTNL for the MII monitor work directly to call
>the bond_miimon_inspect() function and thus ethtool ops.
We can't do this, as it will hit RTNL every monitor interval,
which can be many times per second. The logic is structured to
specifically avoid acquiring RTNL during the inspection pass.
The issue that szybot is seeing only happens if bonding's
use_carrier option is set to 0, which is not the normal case.
use_carrier is a backwards compatibility option from years ago for
drivers that do not implement netif_carrier_on/off (and thus calling
netif_carrier_ok() would be unreliable).
This also came up in [0], and looking now I see there's a patch
that syzbot tested, although I haven't reviewed it.
Another option is to for the Powers That Be to declare that it's
safe to assume that network drivers implement netif_carrier_on/off to
advertise their link state, in which case the use_carrier logic in
bonding can be removed.
Or we can somehow isolate the "must acquire RTNL instead of RCU"
to the problematic use_carrier=0 path, but that's a nontrivial change.
-J
[0] https://lore.kernel.org/lkml/000000000000eb54bf061cfd666a@google.com/
>Reported-by: syzbot+2120b9a8f96b3fa90bad@...kaller.appspotmail.com
>Signed-off-by: Johannes Berg <johannes.berg@...el.com>
>---
> drivers/net/bonding/bond_main.c | 49 +++++++++++----------------------
> 1 file changed, 16 insertions(+), 33 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 2ed0da068490..6a635c23b00e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2576,7 +2576,6 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
>
> /*-------------------------------- Monitoring -------------------------------*/
>
>-/* called with rcu_read_lock() */
> static int bond_miimon_inspect(struct bonding *bond)
> {
> bool ignore_updelay = false;
>@@ -2585,17 +2584,17 @@ static int bond_miimon_inspect(struct bonding *bond)
> struct slave *slave;
>
> if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) {
>- ignore_updelay = !rcu_dereference(bond->curr_active_slave);
>+ ignore_updelay = !rcu_access_pointer(bond->curr_active_slave);
> } else {
> struct bond_up_slave *usable_slaves;
>
>- usable_slaves = rcu_dereference(bond->usable_slaves);
>+ usable_slaves = rtnl_dereference(bond->usable_slaves);
>
> if (usable_slaves && usable_slaves->count == 0)
> ignore_updelay = true;
> }
>
>- bond_for_each_slave_rcu(bond, slave, iter) {
>+ bond_for_each_slave(bond, slave, iter) {
> bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
>
> link_state = bond_check_dev_link(bond, slave->dev, 0);
>@@ -2807,8 +2806,7 @@ static void bond_mii_monitor(struct work_struct *work)
> {
> struct bonding *bond = container_of(work, struct bonding,
> mii_work.work);
>- bool should_notify_peers = false;
>- bool commit;
>+ bool should_notify_peers;
> unsigned long delay;
> struct slave *slave;
> struct list_head *iter;
>@@ -2818,45 +2816,30 @@ static void bond_mii_monitor(struct work_struct *work)
> if (!bond_has_slaves(bond))
> goto re_arm;
>
>- rcu_read_lock();
>- should_notify_peers = bond_should_notify_peers(bond);
>- commit = !!bond_miimon_inspect(bond);
>- if (bond->send_peer_notif) {
>- rcu_read_unlock();
>- if (rtnl_trylock()) {
>- bond->send_peer_notif--;
>- rtnl_unlock();
>- }
>- } else {
>- rcu_read_unlock();
>+ /* deadlock avoidance with bond_close cancel of workqueue */
>+ if (!rtnl_trylock()) {
>+ delay = 1;
>+ goto re_arm;
> }
>
>- if (commit) {
>- /* Race avoidance with bond_close cancel of workqueue */
>- if (!rtnl_trylock()) {
>- delay = 1;
>- should_notify_peers = false;
>- goto re_arm;
>- }
>+ should_notify_peers = bond_should_notify_peers(bond);
>
>+ if (bond_miimon_inspect(bond)) {
> bond_for_each_slave(bond, slave, iter) {
> bond_commit_link_state(slave, BOND_SLAVE_NOTIFY_LATER);
> }
> bond_miimon_commit(bond);
>-
>- rtnl_unlock(); /* might sleep, hold no other locks */
> }
>
>+ if (bond->send_peer_notif)
>+ bond->send_peer_notif--;
>+ if (should_notify_peers)
>+ call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>+ rtnl_unlock(); /* might sleep, hold no other locks */
>+
> re_arm:
> if (bond->params.miimon)
> queue_delayed_work(bond->wq, &bond->mii_work, delay);
>-
>- if (should_notify_peers) {
>- if (!rtnl_trylock())
>- return;
>- call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>- rtnl_unlock();
>- }
> }
>
> static int bond_upper_dev_walk(struct net_device *upper,
>--
>2.45.2
>
>
---
-Jay Vosburgh, jv@...sburgh.net
Powered by blists - more mailing lists