The first patch (hopefully) fixes a real issue while the second is an
unrelated cleanup. But it shares a context so sending as a series.
Daniel Vacek (2):
IB/ipoib: Fix mcast list locking
IB/ipoib: Clean up redundant netif_addr_lock
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
--
2.43.0
We need an additional protection against list removal between ipoib_mcast_join_task()
and ipoib_mcast_dev_flush() in case the &priv->lock needs to be dropped while
iterating the &priv->multicast_list in ipoib_mcast_join_task(). If the mcast
is removed while the lock was dropped, the for loop spins forever resulting
in a hard lockup (as was reported on RHEL 4.18.0-372.75.1.el8_6 kernel):
Task A (kworker/u72:2 below) | Task B (kworker/u72:0 below)
-----------------------------------+-----------------------------------
ipoib_mcast_join_task(work) | ipoib_ib_dev_flush_light(work)
spin_lock_irq(&priv->lock) | __ipoib_ib_dev_flush(priv, ...)
list_for_each_entry(mcast, | ipoib_mcast_dev_flush(dev = priv->dev)
&priv->multicast_list, list) | mutex_lock(&priv->mcast_mutex)
ipoib_mcast_join(dev, mcast) |
spin_unlock_irq(&priv->lock) |
| spin_lock_irqsave(&priv->lock, flags)
| list_for_each_entry_safe(mcast, tmcast,
| &priv->multicast_list, list)
| list_del(&mcast->list);
| list_add_tail(&mcast->list, &remove_list)
| spin_unlock_irqrestore(&priv->lock, flags)
spin_lock_irq(&priv->lock) |
| ipoib_mcast_remove_list(&remove_list)
(Here, mcast is no longer on the | list_for_each_entry_safe(mcast, tmcast,
&priv->multicast_list and we keep | remove_list, list)
spinning on the &remove_list of the \ >>> wait_for_completion(&mcast->done)
other thread which is blocked and the|
list is still valid on it's stack.) | mutex_unlock(&priv->mcast_mutex)
Fix this by adding mutex_lock(&priv->mcast_mutex) to ipoib_mcast_join_task().
Unfortunately we could not reproduce the lockup and confirm this fix but
based on the code review I think this fix should address such lockups.
crash> bc 31
PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
--
[exception RIP: ipoib_mcast_join_task+0x1b1]
RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
work (&priv->mcast_task{,.work})
RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
&mcast->list
RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
mcast
R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
dev priv (&priv->lock) &priv->multicast_list (aka head)
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
--- <NMI exception stack> ---
#5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
#6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
crash> rx ff646f199a8c7e68
ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
(empty)
crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
mcast_mutex.owner.counter = 0xff1c69998efec000
crash> b 8
PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
--
#3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
#4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
#5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
#6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
#7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
crash> rx ff646f1980153e68
ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
broadcast = 0x0,
The mcast(s) on the &remove_list (the remaining part of the ex &priv->multicast_list):
crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
ff1c6a192bd0c200 done.done = 0x0,
ff1c6a192d60ac00 done.done = 0x0,
Reported-by: Yuya Fujita-bishamonten <[email protected]>
Signed-off-by: Daniel Vacek <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 5b3154503bf4..8e4f2c8839be 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -580,6 +580,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
}
netif_addr_unlock_bh(dev);
+ mutex_lock(&priv->mcast_mutex);
spin_lock_irq(&priv->lock);
if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
goto out;
@@ -634,6 +635,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
/* Found the next unjoined group */
if (ipoib_mcast_join(dev, mcast)) {
spin_unlock_irq(&priv->lock);
+ mutex_unlock(&priv->mcast_mutex);
return;
}
} else if (!delay_until ||
@@ -655,6 +657,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
ipoib_mcast_join(dev, mcast);
spin_unlock_irq(&priv->lock);
+ mutex_unlock(&priv->mcast_mutex);
}
void ipoib_mcast_start_thread(struct net_device *dev)
--
2.43.0
A single memory load does not need to be protected by any lock.
The same priv->flags are fetched 15 lines ago without locking anyways.
Signed-off-by: Daniel Vacek <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 8e4f2c8839be..f54e0d212630 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -572,13 +572,9 @@ void ipoib_mcast_join_task(struct work_struct *work)
return;
}
priv->local_lid = port_attr.lid;
- netif_addr_lock_bh(dev);
- if (!test_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags)) {
- netif_addr_unlock_bh(dev);
+ if (!test_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags))
return;
- }
- netif_addr_unlock_bh(dev);
mutex_lock(&priv->mcast_mutex);
spin_lock_irq(&priv->lock);
--
2.43.0
On Mon, Dec 11, 2023 at 02:04:24PM +0100, Daniel Vacek wrote:
> We need an additional protection against list removal between ipoib_mcast_join_task()
> and ipoib_mcast_dev_flush() in case the &priv->lock needs to be dropped while
> iterating the &priv->multicast_list in ipoib_mcast_join_task(). If the mcast
> is removed while the lock was dropped, the for loop spins forever resulting
> in a hard lockup (as was reported on RHEL 4.18.0-372.75.1.el8_6 kernel):
>
> Task A (kworker/u72:2 below) | Task B (kworker/u72:0 below)
> -----------------------------------+-----------------------------------
> ipoib_mcast_join_task(work) | ipoib_ib_dev_flush_light(work)
> spin_lock_irq(&priv->lock) | __ipoib_ib_dev_flush(priv, ...)
> list_for_each_entry(mcast, | ipoib_mcast_dev_flush(dev = priv->dev)
> &priv->multicast_list, list) | mutex_lock(&priv->mcast_mutex)
> ipoib_mcast_join(dev, mcast) |
> spin_unlock_irq(&priv->lock) |
> | spin_lock_irqsave(&priv->lock, flags)
> | list_for_each_entry_safe(mcast, tmcast,
> | &priv->multicast_list, list)
> | list_del(&mcast->list);
> | list_add_tail(&mcast->list, &remove_list)
> | spin_unlock_irqrestore(&priv->lock, flags)
> spin_lock_irq(&priv->lock) |
> | ipoib_mcast_remove_list(&remove_list)
> (Here, mcast is no longer on the | list_for_each_entry_safe(mcast, tmcast,
> &priv->multicast_list and we keep | remove_list, list)
> spinning on the &remove_list of the \ >>> wait_for_completion(&mcast->done)
> other thread which is blocked and the|
> list is still valid on it's stack.) | mutex_unlock(&priv->mcast_mutex)
>
> Fix this by adding mutex_lock(&priv->mcast_mutex) to ipoib_mcast_join_task().
I don't entirely understand the issue and the proposed solution.
There is only one spin_unlock_irq() in the middle of list_for_each_entry(mcast, &priv->multicast_list, list)
and it is right before return statement which will break the loop. So
how will loop spin forever?
Thanks
> Unfortunately we could not reproduce the lockup and confirm this fix but
> based on the code review I think this fix should address such lockups.
>
> crash> bc 31
> PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
> --
> [exception RIP: ipoib_mcast_join_task+0x1b1]
> RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
> RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
> work (&priv->mcast_task{,.work})
> RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
> &mcast->list
> RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
> R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
> mcast
> R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
> dev priv (&priv->lock) &priv->multicast_list (aka head)
> ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> --- <NMI exception stack> ---
> #5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
> #6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
>
> crash> rx ff646f199a8c7e68
> ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
>
> crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
> (empty)
>
> crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
> mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
> mcast_mutex.owner.counter = 0xff1c69998efec000
>
> crash> b 8
> PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
> --
> #3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
> #4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
> #5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
> #6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
> #7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
>
> crash> rx ff646f1980153e68
> ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
>
> crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
> flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
> broadcast = 0x0,
>
> The mcast(s) on the &remove_list (the remaining part of the ex &priv->multicast_list):
>
> crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
> ff1c6a192bd0c200 done.done = 0x0,
> ff1c6a192d60ac00 done.done = 0x0,
>
> Reported-by: Yuya Fujita-bishamonten <[email protected]>
> Signed-off-by: Daniel Vacek <[email protected]>
> ---
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 5b3154503bf4..8e4f2c8839be 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -580,6 +580,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> }
> netif_addr_unlock_bh(dev);
>
> + mutex_lock(&priv->mcast_mutex);
> spin_lock_irq(&priv->lock);
> if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
> goto out;
> @@ -634,6 +635,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> /* Found the next unjoined group */
> if (ipoib_mcast_join(dev, mcast)) {
> spin_unlock_irq(&priv->lock);
> + mutex_unlock(&priv->mcast_mutex);
> return;
> }
> } else if (!delay_until ||
> @@ -655,6 +657,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> ipoib_mcast_join(dev, mcast);
>
> spin_unlock_irq(&priv->lock);
> + mutex_unlock(&priv->mcast_mutex);
> }
>
> void ipoib_mcast_start_thread(struct net_device *dev)
> --
> 2.43.0
>
On Mon, Dec 11, 2023 at 2:45 PM Leon Romanovsky <[email protected]> wrote:
>
> On Mon, Dec 11, 2023 at 02:04:24PM +0100, Daniel Vacek wrote:
> > We need an additional protection against list removal between ipoib_mcast_join_task()
> > and ipoib_mcast_dev_flush() in case the &priv->lock needs to be dropped while
> > iterating the &priv->multicast_list in ipoib_mcast_join_task(). If the mcast
> > is removed while the lock was dropped, the for loop spins forever resulting
> > in a hard lockup (as was reported on RHEL 4.18.0-372.75.1.el8_6 kernel):
> >
> > Task A (kworker/u72:2 below) | Task B (kworker/u72:0 below)
> > -----------------------------------+-----------------------------------
> > ipoib_mcast_join_task(work) | ipoib_ib_dev_flush_light(work)
> > spin_lock_irq(&priv->lock) | __ipoib_ib_dev_flush(priv, ...)
> > list_for_each_entry(mcast, | ipoib_mcast_dev_flush(dev = priv->dev)
> > &priv->multicast_list, list) | mutex_lock(&priv->mcast_mutex)
> > ipoib_mcast_join(dev, mcast) |
> > spin_unlock_irq(&priv->lock) |
> > | spin_lock_irqsave(&priv->lock, flags)
> > | list_for_each_entry_safe(mcast, tmcast,
> > | &priv->multicast_list, list)
> > | list_del(&mcast->list);
> > | list_add_tail(&mcast->list, &remove_list)
> > | spin_unlock_irqrestore(&priv->lock, flags)
> > spin_lock_irq(&priv->lock) |
> > | ipoib_mcast_remove_list(&remove_list)
> > (Here, mcast is no longer on the | list_for_each_entry_safe(mcast, tmcast,
> > &priv->multicast_list and we keep | remove_list, list)
> > spinning on the &remove_list of the \ >>> wait_for_completion(&mcast->done)
> > other thread which is blocked and the|
> > list is still valid on it's stack.) | mutex_unlock(&priv->mcast_mutex)
> >
> > Fix this by adding mutex_lock(&priv->mcast_mutex) to ipoib_mcast_join_task().
>
> I don't entirely understand the issue and the proposed solution.
> There is only one spin_unlock_irq() in the middle of list_for_each_entry(mcast, &priv->multicast_list, list)
> and it is right before return statement which will break the loop. So
> how will loop spin forever?
There's another unlock/lock pair around ib_sa_join_multicast() call in
ipoib_mcast_join() no matter the outcome of the condition. The
ib_sa_join_multicast() cannot be called with the lock being held due
to GFP_KERNEL allocation can possibly sleep. That's what's causing the
issue.
Actually if you check the code, only if the mentioned condition is
false (and the loop is not broken and returned from
ipoib_mcast_join_task()) the lock is released and re-acquired,
creating the window for
ipoib_ib_dev_flush_light()/ipoib_mcast_dev_flush() to break the list.
The vmcore data shows/confirms that clearly.
--nX
> Thanks
>
> > Unfortunately we could not reproduce the lockup and confirm this fix but
> > based on the code review I think this fix should address such lockups.
> >
> > crash> bc 31
> > PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
> > --
> > [exception RIP: ipoib_mcast_join_task+0x1b1]
> > RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
> > RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
> > work (&priv->mcast_task{,.work})
> > RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
> > &mcast->list
> > RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
> > R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
> > mcast
> > R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
> > dev priv (&priv->lock) &priv->multicast_list (aka head)
> > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > --- <NMI exception stack> ---
> > #5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
> > #6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
> >
> > crash> rx ff646f199a8c7e68
> > ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
> >
> > crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
> > (empty)
> >
> > crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
> > mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
> > mcast_mutex.owner.counter = 0xff1c69998efec000
> >
> > crash> b 8
> > PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
> > --
> > #3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
> > #4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
> > #5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
> > #6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
> > #7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
> >
> > crash> rx ff646f1980153e68
> > ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
> >
> > crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
> > flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
> > broadcast = 0x0,
> >
> > The mcast(s) on the &remove_list (the remaining part of the ex &priv->multicast_list):
> >
> > crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
> > ff1c6a192bd0c200 done.done = 0x0,
> > ff1c6a192d60ac00 done.done = 0x0,
> >
> > Reported-by: Yuya Fujita-bishamonten <[email protected]>
> > Signed-off-by: Daniel Vacek <[email protected]>
> > ---
> > drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > index 5b3154503bf4..8e4f2c8839be 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > @@ -580,6 +580,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > }
> > netif_addr_unlock_bh(dev);
> >
> > + mutex_lock(&priv->mcast_mutex);
> > spin_lock_irq(&priv->lock);
> > if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
> > goto out;
> > @@ -634,6 +635,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > /* Found the next unjoined group */
> > if (ipoib_mcast_join(dev, mcast)) {
> > spin_unlock_irq(&priv->lock);
> > + mutex_unlock(&priv->mcast_mutex);
> > return;
> > }
> > } else if (!delay_until ||
> > @@ -655,6 +657,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > ipoib_mcast_join(dev, mcast);
> >
> > spin_unlock_irq(&priv->lock);
> > + mutex_unlock(&priv->mcast_mutex);
> > }
> >
> > void ipoib_mcast_start_thread(struct net_device *dev)
> > --
> > 2.43.0
> >
>
On Mon, Dec 11, 2023 at 03:25:39PM +0100, Daniel Vacek wrote:
> On Mon, Dec 11, 2023 at 2:45 PM Leon Romanovsky <[email protected]> wrote:
> >
> > On Mon, Dec 11, 2023 at 02:04:24PM +0100, Daniel Vacek wrote:
> > > We need an additional protection against list removal between ipoib_mcast_join_task()
> > > and ipoib_mcast_dev_flush() in case the &priv->lock needs to be dropped while
> > > iterating the &priv->multicast_list in ipoib_mcast_join_task(). If the mcast
> > > is removed while the lock was dropped, the for loop spins forever resulting
> > > in a hard lockup (as was reported on RHEL 4.18.0-372.75.1.el8_6 kernel):
> > >
> > > Task A (kworker/u72:2 below) | Task B (kworker/u72:0 below)
> > > -----------------------------------+-----------------------------------
> > > ipoib_mcast_join_task(work) | ipoib_ib_dev_flush_light(work)
> > > spin_lock_irq(&priv->lock) | __ipoib_ib_dev_flush(priv, ...)
> > > list_for_each_entry(mcast, | ipoib_mcast_dev_flush(dev = priv->dev)
> > > &priv->multicast_list, list) | mutex_lock(&priv->mcast_mutex)
> > > ipoib_mcast_join(dev, mcast) |
> > > spin_unlock_irq(&priv->lock) |
> > > | spin_lock_irqsave(&priv->lock, flags)
> > > | list_for_each_entry_safe(mcast, tmcast,
> > > | &priv->multicast_list, list)
> > > | list_del(&mcast->list);
> > > | list_add_tail(&mcast->list, &remove_list)
> > > | spin_unlock_irqrestore(&priv->lock, flags)
> > > spin_lock_irq(&priv->lock) |
> > > | ipoib_mcast_remove_list(&remove_list)
> > > (Here, mcast is no longer on the | list_for_each_entry_safe(mcast, tmcast,
> > > &priv->multicast_list and we keep | remove_list, list)
> > > spinning on the &remove_list of the \ >>> wait_for_completion(&mcast->done)
> > > other thread which is blocked and the|
> > > list is still valid on it's stack.) | mutex_unlock(&priv->mcast_mutex)
> > >
> > > Fix this by adding mutex_lock(&priv->mcast_mutex) to ipoib_mcast_join_task().
> >
> > I don't entirely understand the issue and the proposed solution.
> > There is only one spin_unlock_irq() in the middle of list_for_each_entry(mcast, &priv->multicast_list, list)
> > and it is right before return statement which will break the loop. So
> > how will loop spin forever?
>
> There's another unlock/lock pair around ib_sa_join_multicast() call in
> ipoib_mcast_join() no matter the outcome of the condition. The
> ib_sa_join_multicast() cannot be called with the lock being held due
> to GFP_KERNEL allocation can possibly sleep. That's what's causing the
> issue.
>
> Actually if you check the code, only if the mentioned condition is
> false (and the loop is not broken and returned from
> ipoib_mcast_join_task()) the lock is released and re-acquired,
> creating the window for
> ipoib_ib_dev_flush_light()/ipoib_mcast_dev_flush() to break the list.
> The vmcore data shows/confirms that clearly.
Thanks, it is more clear now.
What about the following change instead of adding extra lock to already
too much complicated IPoIB?
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 5b3154503bf4..bca80fe07584 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -531,21 +531,17 @@ static int ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast)
if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
rec.join_state = SENDONLY_FULLMEMBER_JOIN;
}
- spin_unlock_irq(&priv->lock);
multicast = ib_sa_join_multicast(&ipoib_sa_client, priv->ca, priv->port,
- &rec, comp_mask, GFP_KERNEL,
+ &rec, comp_mask, GFP_ATOMIC,
ipoib_mcast_join_complete, mcast);
- spin_lock_irq(&priv->lock);
if (IS_ERR(multicast)) {
ret = PTR_ERR(multicast);
ipoib_warn(priv, "ib_sa_join_multicast failed, status %d\n", ret);
/* Requeue this join task with a backoff delay */
__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
- spin_unlock_irq(&priv->lock);
complete(&mcast->done);
- spin_lock_irq(&priv->lock);
}
return 0;
}
>
> --nX
>
>
> > Thanks
> >
> > > Unfortunately we could not reproduce the lockup and confirm this fix but
> > > based on the code review I think this fix should address such lockups.
> > >
> > > crash> bc 31
> > > PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
> > > --
> > > [exception RIP: ipoib_mcast_join_task+0x1b1]
> > > RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
> > > RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
> > > work (&priv->mcast_task{,.work})
> > > RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
> > > &mcast->list
> > > RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
> > > R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
> > > mcast
> > > R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
> > > dev priv (&priv->lock) &priv->multicast_list (aka head)
> > > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > > --- <NMI exception stack> ---
> > > #5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
> > > #6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
> > >
> > > crash> rx ff646f199a8c7e68
> > > ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
> > >
> > > crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
> > > (empty)
> > >
> > > crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
> > > mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
> > > mcast_mutex.owner.counter = 0xff1c69998efec000
> > >
> > > crash> b 8
> > > PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
> > > --
> > > #3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
> > > #4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
> > > #5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
> > > #6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
> > > #7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
> > >
> > > crash> rx ff646f1980153e68
> > > ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
> > >
> > > crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
> > > flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
> > > broadcast = 0x0,
> > >
> > > The mcast(s) on the &remove_list (the remaining part of the ex &priv->multicast_list):
> > >
> > > crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
> > > ff1c6a192bd0c200 done.done = 0x0,
> > > ff1c6a192d60ac00 done.done = 0x0,
> > >
> > > Reported-by: Yuya Fujita-bishamonten <[email protected]>
> > > Signed-off-by: Daniel Vacek <[email protected]>
> > > ---
> > > drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > index 5b3154503bf4..8e4f2c8839be 100644
> > > --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > @@ -580,6 +580,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > }
> > > netif_addr_unlock_bh(dev);
> > >
> > > + mutex_lock(&priv->mcast_mutex);
> > > spin_lock_irq(&priv->lock);
> > > if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
> > > goto out;
> > > @@ -634,6 +635,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > /* Found the next unjoined group */
> > > if (ipoib_mcast_join(dev, mcast)) {
> > > spin_unlock_irq(&priv->lock);
> > > + mutex_unlock(&priv->mcast_mutex);
> > > return;
> > > }
> > > } else if (!delay_until ||
> > > @@ -655,6 +657,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > ipoib_mcast_join(dev, mcast);
> > >
> > > spin_unlock_irq(&priv->lock);
> > > + mutex_unlock(&priv->mcast_mutex);
> > > }
> > >
> > > void ipoib_mcast_start_thread(struct net_device *dev)
> > > --
> > > 2.43.0
> > >
> >
>
On Mon, Dec 11, 2023 at 4:18 PM Leon Romanovsky <[email protected]> wrote:
>
> What about the following change instead of adding extra lock to already
> too much complicated IPoIB?
Yeah, that's the other option should also work I believe. And it
simplifies the code nicely.
The allocated mcast_member and mcast_group structures are small enough
so that slab (by default) should not need more then order 1 block to
eventually extend/refill the full kmalloc-256 cache. Some arches will
even use order 0 I believe.
And unless I'm missing something I do not see any other sleeps in that path.
That said, as long as you are fine with occasional failures under
memory pressure, it looks OK to me.
--nX
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 5b3154503bf4..bca80fe07584 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -531,21 +531,17 @@ static int ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast)
> if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
> rec.join_state = SENDONLY_FULLMEMBER_JOIN;
> }
> - spin_unlock_irq(&priv->lock);
>
> multicast = ib_sa_join_multicast(&ipoib_sa_client, priv->ca, priv->port,
> - &rec, comp_mask, GFP_KERNEL,
> + &rec, comp_mask, GFP_ATOMIC,
> ipoib_mcast_join_complete, mcast);
> - spin_lock_irq(&priv->lock);
> if (IS_ERR(multicast)) {
> ret = PTR_ERR(multicast);
> ipoib_warn(priv, "ib_sa_join_multicast failed, status %d\n", ret);
> /* Requeue this join task with a backoff delay */
> __ipoib_mcast_schedule_join_thread(priv, mcast, 1);
> clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
> - spin_unlock_irq(&priv->lock);
> complete(&mcast->done);
> - spin_lock_irq(&priv->lock);
> }
> return 0;
> }
>
>
> >
> > --nX
> >
> >
> > > Thanks
> > >
> > > > Unfortunately we could not reproduce the lockup and confirm this fix but
> > > > based on the code review I think this fix should address such lockups.
> > > >
> > > > crash> bc 31
> > > > PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
> > > > --
> > > > [exception RIP: ipoib_mcast_join_task+0x1b1]
> > > > RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
> > > > RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
> > > > work (&priv->mcast_task{,.work})
> > > > RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
> > > > &mcast->list
> > > > RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
> > > > R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
> > > > mcast
> > > > R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
> > > > dev priv (&priv->lock) &priv->multicast_list (aka head)
> > > > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > > > --- <NMI exception stack> ---
> > > > #5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
> > > > #6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
> > > >
> > > > crash> rx ff646f199a8c7e68
> > > > ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
> > > >
> > > > crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
> > > > (empty)
> > > >
> > > > crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
> > > > mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
> > > > mcast_mutex.owner.counter = 0xff1c69998efec000
> > > >
> > > > crash> b 8
> > > > PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
> > > > --
> > > > #3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
> > > > #4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
> > > > #5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
> > > > #6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
> > > > #7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
> > > >
> > > > crash> rx ff646f1980153e68
> > > > ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
> > > >
> > > > crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
> > > > flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
> > > > broadcast = 0x0,
> > > >
> > > > The mcast(s) on the &remove_list (the remaining part of the ex &priv->multicast_list):
> > > >
> > > > crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
> > > > ff1c6a192bd0c200 done.done = 0x0,
> > > > ff1c6a192d60ac00 done.done = 0x0,
> > > >
> > > > Reported-by: Yuya Fujita-bishamonten <[email protected]>
> > > > Signed-off-by: Daniel Vacek <[email protected]>
> > > > ---
> > > > drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > > index 5b3154503bf4..8e4f2c8839be 100644
> > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > > @@ -580,6 +580,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > > }
> > > > netif_addr_unlock_bh(dev);
> > > >
> > > > + mutex_lock(&priv->mcast_mutex);
> > > > spin_lock_irq(&priv->lock);
> > > > if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
> > > > goto out;
> > > > @@ -634,6 +635,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > > /* Found the next unjoined group */
> > > > if (ipoib_mcast_join(dev, mcast)) {
> > > > spin_unlock_irq(&priv->lock);
> > > > + mutex_unlock(&priv->mcast_mutex);
> > > > return;
> > > > }
> > > > } else if (!delay_until ||
> > > > @@ -655,6 +657,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > > ipoib_mcast_join(dev, mcast);
> > > >
> > > > spin_unlock_irq(&priv->lock);
> > > > + mutex_unlock(&priv->mcast_mutex);
> > > > }
> > > >
> > > > void ipoib_mcast_start_thread(struct net_device *dev)
> > > > --
> > > > 2.43.0
> > > >
> > >
> >
>
On Mon, Dec 11, 2023 at 05:00:11PM +0100, Daniel Vacek wrote:
> On Mon, Dec 11, 2023 at 4:18 PM Leon Romanovsky <[email protected]> wrote:
> >
> > What about the following change instead of adding extra lock to already
> > too much complicated IPoIB?
>
> Yeah, that's the other option should also work I believe. And it
> simplifies the code nicely.
>
> The allocated mcast_member and mcast_group structures are small enough
> so that slab (by default) should not need more then order 1 block to
> eventually extend/refill the full kmalloc-256 cache. Some arches will
> even use order 0 I believe.
> And unless I'm missing something I do not see any other sleeps in that path.
>
> That said, as long as you are fine with occasional failures under
> memory pressure, it looks OK to me.
Yes, IMHO change from GFP_KERNEL to be GFP_ATOMIC is safer than adding extra lock.
Thanks
>
> --nX
>
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > index 5b3154503bf4..bca80fe07584 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > @@ -531,21 +531,17 @@ static int ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast)
> > if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
> > rec.join_state = SENDONLY_FULLMEMBER_JOIN;
> > }
> > - spin_unlock_irq(&priv->lock);
> >
> > multicast = ib_sa_join_multicast(&ipoib_sa_client, priv->ca, priv->port,
> > - &rec, comp_mask, GFP_KERNEL,
> > + &rec, comp_mask, GFP_ATOMIC,
> > ipoib_mcast_join_complete, mcast);
> > - spin_lock_irq(&priv->lock);
> > if (IS_ERR(multicast)) {
> > ret = PTR_ERR(multicast);
> > ipoib_warn(priv, "ib_sa_join_multicast failed, status %d\n", ret);
> > /* Requeue this join task with a backoff delay */
> > __ipoib_mcast_schedule_join_thread(priv, mcast, 1);
> > clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
> > - spin_unlock_irq(&priv->lock);
> > complete(&mcast->done);
> > - spin_lock_irq(&priv->lock);
> > }
> > return 0;
> > }
> >
> >
> > >
> > > --nX
> > >
> > >
> > > > Thanks
> > > >
> > > > > Unfortunately we could not reproduce the lockup and confirm this fix but
> > > > > based on the code review I think this fix should address such lockups.
> > > > >
> > > > > crash> bc 31
> > > > > PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
> > > > > --
> > > > > [exception RIP: ipoib_mcast_join_task+0x1b1]
> > > > > RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
> > > > > RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
> > > > > work (&priv->mcast_task{,.work})
> > > > > RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
> > > > > &mcast->list
> > > > > RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
> > > > > R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
> > > > > mcast
> > > > > R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
> > > > > dev priv (&priv->lock) &priv->multicast_list (aka head)
> > > > > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > > > > --- <NMI exception stack> ---
> > > > > #5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
> > > > > #6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
> > > > >
> > > > > crash> rx ff646f199a8c7e68
> > > > > ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
> > > > >
> > > > > crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
> > > > > (empty)
> > > > >
> > > > > crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
> > > > > mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
> > > > > mcast_mutex.owner.counter = 0xff1c69998efec000
> > > > >
> > > > > crash> b 8
> > > > > PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
> > > > > --
> > > > > #3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
> > > > > #4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
> > > > > #5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
> > > > > #6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
> > > > > #7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
> > > > >
> > > > > crash> rx ff646f1980153e68
> > > > > ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
> > > > >
> > > > > crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
> > > > > flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
> > > > > broadcast = 0x0,
> > > > >
> > > > > The mcast(s) on the &remove_list (the remaining part of the ex &priv->multicast_list):
> > > > >
> > > > > crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
> > > > > ff1c6a192bd0c200 done.done = 0x0,
> > > > > ff1c6a192d60ac00 done.done = 0x0,
> > > > >
> > > > > Reported-by: Yuya Fujita-bishamonten <[email protected]>
> > > > > Signed-off-by: Daniel Vacek <[email protected]>
> > > > > ---
> > > > > drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > > > index 5b3154503bf4..8e4f2c8839be 100644
> > > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> > > > > @@ -580,6 +580,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > > > }
> > > > > netif_addr_unlock_bh(dev);
> > > > >
> > > > > + mutex_lock(&priv->mcast_mutex);
> > > > > spin_lock_irq(&priv->lock);
> > > > > if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
> > > > > goto out;
> > > > > @@ -634,6 +635,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > > > /* Found the next unjoined group */
> > > > > if (ipoib_mcast_join(dev, mcast)) {
> > > > > spin_unlock_irq(&priv->lock);
> > > > > + mutex_unlock(&priv->mcast_mutex);
> > > > > return;
> > > > > }
> > > > > } else if (!delay_until ||
> > > > > @@ -655,6 +657,7 @@ void ipoib_mcast_join_task(struct work_struct *work)
> > > > > ipoib_mcast_join(dev, mcast);
> > > > >
> > > > > spin_unlock_irq(&priv->lock);
> > > > > + mutex_unlock(&priv->mcast_mutex);
> > > > > }
> > > > >
> > > > > void ipoib_mcast_start_thread(struct net_device *dev)
> > > > > --
> > > > > 2.43.0
> > > > >
> > > >
> > >
> >
>
Releasing the `priv->lock` while iterating the `priv->multicast_list` in
`ipoib_mcast_join_task()` opens a window for `ipoib_mcast_dev_flush()` to
remove the items while in the middle of iteration. If the mcast is removed
while the lock was dropped, the for loop spins forever resulting in a hard
lockup (as was reported on RHEL 4.18.0-372.75.1.el8_6 kernel):
Task A (kworker/u72:2 below) | Task B (kworker/u72:0 below)
-----------------------------------+-----------------------------------
ipoib_mcast_join_task(work) | ipoib_ib_dev_flush_light(work)
spin_lock_irq(&priv->lock) | __ipoib_ib_dev_flush(priv, ...)
list_for_each_entry(mcast, | ipoib_mcast_dev_flush(dev = priv->dev)
&priv->multicast_list, list) |
ipoib_mcast_join(dev, mcast) |
spin_unlock_irq(&priv->lock) |
| spin_lock_irqsave(&priv->lock, flags)
| list_for_each_entry_safe(mcast, tmcast,
| &priv->multicast_list, list)
| list_del(&mcast->list);
| list_add_tail(&mcast->list, &remove_list)
| spin_unlock_irqrestore(&priv->lock, flags)
spin_lock_irq(&priv->lock) |
| ipoib_mcast_remove_list(&remove_list)
(Here, `mcast` is no longer on the | list_for_each_entry_safe(mcast, tmcast,
`priv->multicast_list` and we keep | remove_list, list)
spinning on the `remove_list` of | >>> wait_for_completion(&mcast->done)
the other thread which is blocked |
and the list is still valid on |
it's stack.)
Fix this by keeping the lock held and changing to GFP_ATOMIC to prevent
eventual sleeps.
Unfortunately we could not reproduce the lockup and confirm this fix but
based on the code review I think this fix should address such lockups.
crash> bc 31
PID: 747 TASK: ff1c6a1a007e8000 CPU: 31 COMMAND: "kworker/u72:2"
--
[exception RIP: ipoib_mcast_join_task+0x1b1]
RIP: ffffffffc0944ac1 RSP: ff646f199a8c7e00 RFLAGS: 00000002
RAX: 0000000000000000 RBX: ff1c6a1a04dc82f8 RCX: 0000000000000000
work (&priv->mcast_task{,.work})
RDX: ff1c6a192d60ac68 RSI: 0000000000000286 RDI: ff1c6a1a04dc8000
&mcast->list
RBP: ff646f199a8c7e90 R8: ff1c699980019420 R9: ff1c6a1920c9a000
R10: ff646f199a8c7e00 R11: ff1c6a191a7d9800 R12: ff1c6a192d60ac00
mcast
R13: ff1c6a1d82200000 R14: ff1c6a1a04dc8000 R15: ff1c6a1a04dc82d8
dev priv (&priv->lock) &priv->multicast_list (aka head)
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
--- <NMI exception stack> ---
#5 [ff646f199a8c7e00] ipoib_mcast_join_task+0x1b1 at ffffffffc0944ac1 [ib_ipoib]
#6 [ff646f199a8c7e98] process_one_work+0x1a7 at ffffffff9bf10967
crash> rx ff646f199a8c7e68
ff646f199a8c7e68: ff1c6a1a04dc82f8 <<< work = &priv->mcast_task.work
crash> list -hO ipoib_dev_priv.multicast_list ff1c6a1a04dc8000
(empty)
crash> ipoib_dev_priv.mcast_task.work.func,mcast_mutex.owner.counter ff1c6a1a04dc8000
mcast_task.work.func = 0xffffffffc0944910 <ipoib_mcast_join_task>,
mcast_mutex.owner.counter = 0xff1c69998efec000
crash> b 8
PID: 8 TASK: ff1c69998efec000 CPU: 33 COMMAND: "kworker/u72:0"
--
#3 [ff646f1980153d50] wait_for_completion+0x96 at ffffffff9c7d7646
#4 [ff646f1980153d90] ipoib_mcast_remove_list+0x56 at ffffffffc0944dc6 [ib_ipoib]
#5 [ff646f1980153de8] ipoib_mcast_dev_flush+0x1a7 at ffffffffc09455a7 [ib_ipoib]
#6 [ff646f1980153e58] __ipoib_ib_dev_flush+0x1a4 at ffffffffc09431a4 [ib_ipoib]
#7 [ff646f1980153e98] process_one_work+0x1a7 at ffffffff9bf10967
crash> rx ff646f1980153e68
ff646f1980153e68: ff1c6a1a04dc83f0 <<< work = &priv->flush_light
crash> ipoib_dev_priv.flush_light.func,broadcast ff1c6a1a04dc8000
flush_light.func = 0xffffffffc0943820 <ipoib_ib_dev_flush_light>,
broadcast = 0x0,
The mcast(s) on the `remove_list` (the remaining part of the ex `priv->multicast_list`):
crash> list -s ipoib_mcast.done.done ipoib_mcast.list -H ff646f1980153e10 | paste - -
ff1c6a192bd0c200 done.done = 0x0,
ff1c6a192d60ac00 done.done = 0x0,
Reported-by: Yuya Fujita-bishamonten <[email protected]>
Signed-off-by: Daniel Vacek <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 5b3154503bf4..bca80fe07584 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -531,21 +531,17 @@ static int ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast)
if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
rec.join_state = SENDONLY_FULLMEMBER_JOIN;
}
- spin_unlock_irq(&priv->lock);
multicast = ib_sa_join_multicast(&ipoib_sa_client, priv->ca, priv->port,
- &rec, comp_mask, GFP_KERNEL,
+ &rec, comp_mask, GFP_ATOMIC,
ipoib_mcast_join_complete, mcast);
- spin_lock_irq(&priv->lock);
if (IS_ERR(multicast)) {
ret = PTR_ERR(multicast);
ipoib_warn(priv, "ib_sa_join_multicast failed, status %d\n", ret);
/* Requeue this join task with a backoff delay */
__ipoib_mcast_schedule_join_thread(priv, mcast, 1);
clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
- spin_unlock_irq(&priv->lock);
complete(&mcast->done);
- spin_lock_irq(&priv->lock);
}
return 0;
}
--
2.43.0
On Mon, 11 Dec 2023 14:04:23 +0100, Daniel Vacek wrote:
> The first patch (hopefully) fixes a real issue while the second is an
> unrelated cleanup. But it shares a context so sending as a series.
>
> Daniel Vacek (2):
> IB/ipoib: Fix mcast list locking
> IB/ipoib: Clean up redundant netif_addr_lock
>
> [...]
Applied, thanks!
[1/1] IB/ipoib: Fix mcast list locking
https://git.kernel.org/rdma/rdma/c/4f973e211b3b1c
Best regards,
--
Leon Romanovsky <[email protected]>
On Tue, Dec 12, 2023 at 9:29 AM Leon Romanovsky <[email protected]> wrote:
>
>
> On Mon, 11 Dec 2023 14:04:23 +0100, Daniel Vacek wrote:
> > The first patch (hopefully) fixes a real issue while the second is an
> > unrelated cleanup. But it shares a context so sending as a series.
> >
> > Daniel Vacek (2):
> > IB/ipoib: Fix mcast list locking
> > IB/ipoib: Clean up redundant netif_addr_lock
> >
> > [...]
>
> Applied, thanks!
Thank you.
One small detail - I was asked by Yuya to change the "Reported-by" as follows:
---
Reported-by: Yuya Fujita <[email protected]>
---
Would that be possible? And if yes, could you amend the commit
yourself or do you want me to send a v3?
--nX
> [1/1] IB/ipoib: Fix mcast list locking
> https://git.kernel.org/rdma/rdma/c/4f973e211b3b1c
>
> Best regards,
> --
> Leon Romanovsky <[email protected]>
>
On Wed, Dec 13, 2023 at 01:18:26PM +0100, Daniel Vacek wrote:
> On Tue, Dec 12, 2023 at 9:29 AM Leon Romanovsky <[email protected]> wrote:
> >
> >
> > On Mon, 11 Dec 2023 14:04:23 +0100, Daniel Vacek wrote:
> > > The first patch (hopefully) fixes a real issue while the second is an
> > > unrelated cleanup. But it shares a context so sending as a series.
> > >
> > > Daniel Vacek (2):
> > > IB/ipoib: Fix mcast list locking
> > > IB/ipoib: Clean up redundant netif_addr_lock
> > >
> > > [...]
> >
> > Applied, thanks!
>
> Thank you.
>
> One small detail - I was asked by Yuya to change the "Reported-by" as follows:
>
> ---
> Reported-by: Yuya Fujita <[email protected]>
> ---
>
> Would that be possible? And if yes, could you amend the commit
> yourself or do you want me to send a v3?
Unfortunately, it is already too late as we promoted my wip branch to be
official rdma/for-next.
Thanks
>
> --nX
>
>
> > [1/1] IB/ipoib: Fix mcast list locking
> > https://git.kernel.org/rdma/rdma/c/4f973e211b3b1c
> >
> > Best regards,
> > --
> > Leon Romanovsky <[email protected]>
> >
>