* [PATCH net] bonding: hold ops lock around get_link
@ 2025-04-08 17:14 Stanislav Fomichev
2025-04-09 1:54 ` Hangbin Liu
2025-04-10 0:44 ` Jakub Kicinski
0 siblings, 2 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2025-04-08 17:14 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, jv, andrew+netdev, sdf,
linux-kernel, syzbot+48c14f61594bdfadb086
syzbot reports a case of ethtool_ops->get_link being called without
ops lock:
ethtool_op_get_link+0x15/0x60 net/ethtool/ioctl.c:63
bond_check_dev_link+0x1fb/0x4b0 drivers/net/bonding/bond_main.c:864
bond_miimon_inspect drivers/net/bonding/bond_main.c:2734 [inline]
bond_mii_monitor+0x49d/0x3170 drivers/net/bonding/bond_main.c:2956
process_one_work kernel/workqueue.c:3238 [inline]
process_scheduled_works+0xac3/0x18e0 kernel/workqueue.c:3319
worker_thread+0x870/0xd50 kernel/workqueue.c:3400
kthread+0x7b7/0x940 kernel/kthread.c:464
ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:153
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
Commit 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
changed to lockless __linkwatch_sync_dev in ethtool_op_get_link.
All paths except bonding are coming via locked ioctl. Add necessary
locking to bonding.
Reported-by: syzbot+48c14f61594bdfadb086@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=48c14f61594bdfadb086
Fixes: 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
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 950d8e4d86f8..d1ec5ec6f7e5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -850,8 +850,9 @@ static int bond_check_dev_link(struct bonding *bond,
struct net_device *slave_dev, int reporting)
{
const struct net_device_ops *slave_ops = slave_dev->netdev_ops;
- struct ifreq ifr;
struct mii_ioctl_data *mii;
+ struct ifreq ifr;
+ int ret;
if (!reporting && !netif_running(slave_dev))
return 0;
@@ -860,9 +861,14 @@ static int bond_check_dev_link(struct bonding *bond,
return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
/* Try to get link status using Ethtool first. */
- if (slave_dev->ethtool_ops->get_link)
- return slave_dev->ethtool_ops->get_link(slave_dev) ?
+ if (slave_dev->ethtool_ops->get_link) {
+ netdev_lock_ops(slave_dev);
+ ret = slave_dev->ethtool_ops->get_link(slave_dev) ?
BMSR_LSTATUS : 0;
+ netdev_unlock_ops(slave_dev);
+
+ return ret;
+ }
/* Ethtool can't be used, fallback to MII ioctls. */
if (slave_ops->ndo_eth_ioctl) {
--
2.49.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] bonding: hold ops lock around get_link
2025-04-08 17:14 [PATCH net] bonding: hold ops lock around get_link Stanislav Fomichev
@ 2025-04-09 1:54 ` Hangbin Liu
2025-04-10 0:44 ` Jakub Kicinski
1 sibling, 0 replies; 4+ messages in thread
From: Hangbin Liu @ 2025-04-09 1:54 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, davem, edumazet, kuba, pabeni, jv, andrew+netdev,
linux-kernel, syzbot+48c14f61594bdfadb086
On Tue, Apr 08, 2025 at 10:14:51AM -0700, Stanislav Fomichev wrote:
> syzbot reports a case of ethtool_ops->get_link being called without
> ops lock:
>
> ethtool_op_get_link+0x15/0x60 net/ethtool/ioctl.c:63
> bond_check_dev_link+0x1fb/0x4b0 drivers/net/bonding/bond_main.c:864
> bond_miimon_inspect drivers/net/bonding/bond_main.c:2734 [inline]
> bond_mii_monitor+0x49d/0x3170 drivers/net/bonding/bond_main.c:2956
> process_one_work kernel/workqueue.c:3238 [inline]
> process_scheduled_works+0xac3/0x18e0 kernel/workqueue.c:3319
> worker_thread+0x870/0xd50 kernel/workqueue.c:3400
> kthread+0x7b7/0x940 kernel/kthread.c:464
> ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:153
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>
> Commit 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
> changed to lockless __linkwatch_sync_dev in ethtool_op_get_link.
> All paths except bonding are coming via locked ioctl. Add necessary
> locking to bonding.
>
> Reported-by: syzbot+48c14f61594bdfadb086@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=48c14f61594bdfadb086
> Fixes: 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
> 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 950d8e4d86f8..d1ec5ec6f7e5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -850,8 +850,9 @@ static int bond_check_dev_link(struct bonding *bond,
> struct net_device *slave_dev, int reporting)
> {
> const struct net_device_ops *slave_ops = slave_dev->netdev_ops;
> - struct ifreq ifr;
> struct mii_ioctl_data *mii;
> + struct ifreq ifr;
> + int ret;
>
> if (!reporting && !netif_running(slave_dev))
> return 0;
> @@ -860,9 +861,14 @@ static int bond_check_dev_link(struct bonding *bond,
> return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
>
> /* Try to get link status using Ethtool first. */
> - if (slave_dev->ethtool_ops->get_link)
> - return slave_dev->ethtool_ops->get_link(slave_dev) ?
> + if (slave_dev->ethtool_ops->get_link) {
> + netdev_lock_ops(slave_dev);
> + ret = slave_dev->ethtool_ops->get_link(slave_dev) ?
> BMSR_LSTATUS : 0;
> + netdev_unlock_ops(slave_dev);
> +
> + return ret;
> + }
>
> /* Ethtool can't be used, fallback to MII ioctls. */
> if (slave_ops->ndo_eth_ioctl) {
> --
> 2.49.0
>
Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] bonding: hold ops lock around get_link
2025-04-08 17:14 [PATCH net] bonding: hold ops lock around get_link Stanislav Fomichev
2025-04-09 1:54 ` Hangbin Liu
@ 2025-04-10 0:44 ` Jakub Kicinski
2025-04-10 15:46 ` Stanislav Fomichev
1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2025-04-10 0:44 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, davem, edumazet, pabeni, jv, andrew+netdev, linux-kernel,
syzbot+48c14f61594bdfadb086
On Tue, 8 Apr 2025 10:14:51 -0700 Stanislav Fomichev wrote:
> + netdev_lock_ops(slave_dev);
> + ret = slave_dev->ethtool_ops->get_link(slave_dev) ?
> BMSR_LSTATUS : 0;
> + netdev_unlock_ops(slave_dev);
> +
> + return ret;
Is it okay to nit pick? Since you have a temp now it's cleaner to move
the ternary operator later, avoid the line break:
netdev_lock_ops(slave_dev);
ret = slave_dev->ethtool_ops->get_link(slave_dev);
netdev_unlock_ops(slave_dev);
return ret ? BMSR_LSTATUS : 0;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] bonding: hold ops lock around get_link
2025-04-10 0:44 ` Jakub Kicinski
@ 2025-04-10 15:46 ` Stanislav Fomichev
0 siblings, 0 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2025-04-10 15:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, davem, edumazet, pabeni, jv,
andrew+netdev, linux-kernel, syzbot+48c14f61594bdfadb086
On 04/09, Jakub Kicinski wrote:
> On Tue, 8 Apr 2025 10:14:51 -0700 Stanislav Fomichev wrote:
> > + netdev_lock_ops(slave_dev);
> > + ret = slave_dev->ethtool_ops->get_link(slave_dev) ?
> > BMSR_LSTATUS : 0;
> > + netdev_unlock_ops(slave_dev);
> > +
> > + return ret;
>
> Is it okay to nit pick? Since you have a temp now it's cleaner to move
> the ternary operator later, avoid the line break:
>
> netdev_lock_ops(slave_dev);
> ret = slave_dev->ethtool_ops->get_link(slave_dev);
> netdev_unlock_ops(slave_dev);
>
> return ret ? BMSR_LSTATUS : 0;
Nits are always welcome :-) Will repost shortly..
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-04-10 15:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-08 17:14 [PATCH net] bonding: hold ops lock around get_link Stanislav Fomichev
2025-04-09 1:54 ` Hangbin Liu
2025-04-10 0:44 ` Jakub Kicinski
2025-04-10 15:46 ` Stanislav Fomichev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).