RDMA/mlx5: Release locks during notifier unregister

From: Leon Romanovsky <[email protected]>

The below kernel panic was observed when created bond mode LACP with GRE tunnel on top. The reason to it was not released spinlock during mlx5 notify unregsiter sequence.

[  234.562007] BUG: scheduling while atomic: sh/10900/0x00000002
[  234.563005] Preemption disabled at:
[  234.566864] ------------[ cut here ]------------
[  234.567120] DEBUG_LOCKS_WARN_ON(val > preempt_count())
[  234.567139] WARNING: CPU: 16 PID: 10900 at kernel/sched/core.c:3203
preempt_count_sub+0xca/0x170
[  234.569550] CPU: 16 PID: 10900 Comm: sh Tainted: G        W 5.2.0-
rc1-for-linust-dbg-2019-05-25_04-57-33-60 #1
[  234.569886] Hardware name: Dell Inc. PowerEdge R720/0X3D66, BIOS
2.6.1 02/12/2018
[  234.570183] RIP: 0010:preempt_count_sub+0xca/0x170
[  234.570404] Code: 03 38

d0 7c 08 84 d2 0f 85 b0 00 00 00 8b 15 dd 02 03 04 85 d2 75 ba 48 c7 c6
00 e1 88 83 48 c7 c7 40 e1 88 83 e8 76 11 f7 ff <0f> 0b 5b c3 65 8b 05 d3 1f d8 7e 84 c0 75 82 e8 62 c3 c3 00 85 c0

[  234.570911] RSP: 0018:ffff888b94477b08 EFLAGS: 00010286
[  234.571133] RAX: 0000000000000000 RBX: 0000000000000001 RCX:
0000000000000000
[  234.571391] RDX: 0000000000000000 RSI: 0000000000000004 RDI:
0000000000000246
[  234.571648] RBP: ffff888ba5560000 R08: fffffbfff08962d5 R09:
fffffbfff08962d5
[  234.571902] R10: 0000000000000001 R11: fffffbfff08962d4 R12:
ffff888bac6e9548
[  234.572157] R13: ffff888babfaf728 R14: ffff888bac6e9568 R15:
ffff888babfaf750
[  234.572412] FS: 00007fcafa59b740(0000) GS:ffff888bed200000(0000)
knlGS:0000000000000000
[  234.572686] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  234.572914] CR2: 00007f984f16b140 CR3: 0000000b2bf0a001 CR4:
00000000001606e0
[  234.573172] Call Trace:
[  234.573336] _raw_spin_unlock+0x2e/0x50
[  234.573542] mlx5_ib_unbind_slave_port+0x1bc/0x690 [mlx5_ib]
[  234.573793] mlx5_ib_cleanup_multiport_master+0x1d3/0x660 [mlx5_ib]
[  234.574039] mlx5_ib_stage_init_cleanup+0x4c/0x360 [mlx5_ib]
[  234.574271]  ? kfree+0xf5/0x2f0
[  234.574465] __mlx5_ib_remove+0x61/0xd0 [mlx5_ib]
[  234.574688]  ? __mlx5_ib_remove+0xd0/0xd0 [mlx5_ib]
[  234.574951] mlx5_remove_device+0x234/0x300 [mlx5_core]
[  234.575224] mlx5_unregister_device+0x4d/0x1e0 [mlx5_core]
[  234.575493] remove_one+0x4f/0x160 [mlx5_core]
[  234.575704] pci_device_remove+0xef/0x2a0
[  234.581407]  ? pcibios_free_irq+0x10/0x10
[  234.587143]  ? up_read+0xc1/0x260
[  234.592785] device_release_driver_internal+0x1ab/0x430
[  234.598442] unbind_store+0x152/0x200
[  234.604064]  ? sysfs_kf_write+0x3b/0x180
[  234.609441]  ? sysfs_file_ops+0x160/0x160
[  234.615021] kernfs_fop_write+0x277/0x440
[  234.620288]  ? __sb_start_write+0x1ef/0x2c0
[  234.625512] vfs_write+0x15e/0x460
[  234.630786] ksys_write+0x156/0x1e0
[  234.635988]  ? __ia32_sys_read+0xb0/0xb0
[  234.641120]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[  234.646163] do_syscall_64+0x95/0x470
[  234.651106] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  234.656004] RIP: 0033:0x7fcaf9c9cfd0
[  234.660686] Code: 73 01

c3 48 8b 0d c0 6e 2d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00
83 3d cd cf 2d 00 00 75 10 b8 01 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ee cb 01 00 48 89 04 24

[  234.670128] RSP: 002b:00007ffd3b01ddd8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[  234.674811] RAX: ffffffffffffffda RBX: 000000000000000d RCX:
00007fcaf9c9cfd0
[  234.679387] RDX: 000000000000000d RSI: 00007fcafa5c1000 RDI:
0000000000000001
[  234.683848] RBP: 00007fcafa5c1000 R08: 000000000000000a R09:
00007fcafa59b740
[  234.688167] R10: 00007ffd3b01d8e0 R11: 0000000000000246 R12:
00007fcaf9f75400
[  234.692386] R13: 000000000000000d R14: 0000000000000001 R15:
0000000000000000
[  234.696495] irq event stamp: 153067
[  234.700525] hardirqs last enabled at (153067): [<ffffffff83258c39>]
_raw_spin_unlock_irqrestore+0x59/0x70
[  234.704665] hardirqs last disabled at (153066):
[<ffffffff83259382>] _raw_spin_lock_irqsave+0x22/0x90
[  234.708722] softirqs last enabled at (153058): [<ffffffff836006c5>]
__do_softirq+0x6c5/0xb4e
[  234.712673] softirqs last disabled at (153051):
[<ffffffff81227c1d>] irq_exit+0x17d/0x1d0
[  234.716601] ---[ end trace 5dbf096843ee9ce6 ]---

Fixes df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier API")

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/hw/mlx5/main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/infiniband/hw/mlx5/main.c +0 -0

index c2a5780cb394..e12a4404096b 100644
@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c +0 -0

 	mlx5_remove_netdev_notifier(ibdev, port_num);
 	spin_lock(&port->mp.mpi_lock);

Quoted 15 lines

drivers/infiniband/hw/mlx5/main.c +1 -4

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 
-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct mlx5_ib_dev *ibdev, · line 5810

Quoted 4 lines

drivers/infiniband/hw/mlx5/main.c +3 -0

 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 15 lines · Doug Ledford

Quoted 15 lines

drivers/infiniband/hw/mlx5/main.c +1 -4

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 
-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 

Quoted 11 lines

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct mlx5_ib_dev *ibdev, · line 5810

Quoted 4 lines

drivers/infiniband/hw/mlx5/main.c +3 -0

 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

Jason

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Doug Ledford

Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct · line 5811

Quoted 10 lines

drivers/infiniband/hw/mlx5/main.c +3 -4

-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 2 lines

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

What do you propose to delete?

Quoted 29 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Doug Ledford

Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct · line 5811

Quoted 10 lines

drivers/infiniband/hw/mlx5/main.c +3 -4

-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 4 lines

Quoted 2 lines

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

What do you propose to delete?

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just dead code, delete it.

Jason

Quoted 44 lines · Jason Gunthorpe

Quoted 37 lines · Leon Romanovsky

Quoted 36 lines · Jason Gunthorpe

Quoted 34 lines · Doug Ledford

Quoted 21 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void

mlx5_ib_unbind_slave_port(struct
mlx5_ib_dev *ibdev,

drivers/infiniband/hw/mlx5/main.c +0 -2

 		return;
 	}
 
-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi-

Quoted 1 line

mdev_events);

drivers/infiniband/hw/mlx5/main.c +2 -2

-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi-

Quoted 1 line

mdev_events);

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct mlx5_ib_dev *ibdev, · line 5810

Quoted 1 line

drivers/infiniband/hw/mlx5/main.c +1 -0

+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the
appearance of creating a new race. Doing a check/unregister/set-null
series outside of any locks is a red flag to someone investigating the
code. You should at least make note of the fact that calling unregister
more than once is safe. If you're fine with it, I can add a comment and
take the patch, or you can resubmit.

Quoted 5 lines

Quoted 3 lines

Mucking about notifier_call like that is gross anyhow, maybe better to
delete it entirely.

What do you propose to delete?

Quoted 8 lines

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just
dead code, delete it.

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check
is needed to ensure single call to mlx5_notifier_unregister, because calls to mlx5_ib_unbind_slave_port() will be serialized.

Quoted 54 lines · Leon Romanovsky

Quoted 44 lines · Jason Gunthorpe

Quoted 37 lines · Leon Romanovsky

Quoted 36 lines · Jason Gunthorpe

Quoted 34 lines · Doug Ledford

Quoted 21 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void

mlx5_ib_unbind_slave_port(struct
mlx5_ib_dev *ibdev,

drivers/infiniband/hw/mlx5/main.c +0 -2

 		return;
 	}
 
-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi-

Quoted 1 line

mdev_events);

drivers/infiniband/hw/mlx5/main.c +2 -2

-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi-

Quoted 1 line

mdev_events);

drivers/infiniband/hw/mlx5/main.c

Quoted 1 line

drivers/infiniband/hw/mlx5/main.c +1 -0

+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the
appearance of creating a new race. Doing a check/unregister/set-null
series outside of any locks is a red flag to someone investigating the
code. You should at least make note of the fact that calling unregister
more than once is safe. If you're fine with it, I can add a comment and
take the patch, or you can resubmit.

Quoted 5 lines

Quoted 3 lines

Mucking about notifier_call like that is gross anyhow, maybe better to
delete it entirely.

What do you propose to delete?

Quoted 8 lines

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just
dead code, delete it.

Quoted 5 lines

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check
is needed to ensure single call to mlx5_notifier_unregister, because calls to mlx5_ib_unbind_slave_port() will be serialized.

But looking at the code, it doesn't appear mlx5_notifier_unregister requires there to only be a single call. It's safe to call it multiple times for the same notifier.

--
Doug Ledford <[email protected]>

    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Quoted 40 lines · Jason Gunthorpe

Quoted 29 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Doug Ledford

Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct · line 5811

Quoted 10 lines

drivers/infiniband/hw/mlx5/main.c +3 -4

-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 4 lines

Quoted 2 lines

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

What do you propose to delete?

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

Quoted 2 lines

If you need it, then we can't drop the lock, if you don't, it is just dead code, delete it.

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check is needed to ensure single call to mlx5_notifier_unregister, because calls to mlx5_ib_unbind_slave_port() will be serialized.

Quoted 44 lines · Leon Romanovsky

Quoted 42 lines · Jason Gunthorpe

Quoted 29 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Doug Ledford

Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct · line 5811

Quoted 10 lines

drivers/infiniband/hw/mlx5/main.c +3 -4

-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 4 lines

Quoted 2 lines

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

What do you propose to delete?

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just dead code, delete it.

Quoted 4 lines

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check is needed to ensure single call to mlx5_notifier_unregister, because calls to mlx5_ib_unbind_slave_port() will be serialized.

If this routine is now relying on locking that is not obvious in the function itself then add a lockdep too.

Jason

Quoted 50 lines · Jason Gunthorpe

Quoted 48 lines · Leon Romanovsky

Quoted 35 lines · Jason Gunthorpe

Quoted 29 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Doug Ledford

Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct · line 5811

Quoted 10 lines

drivers/infiniband/hw/mlx5/main.c +3 -4

-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 4 lines

Quoted 2 lines

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

What do you propose to delete?

Quoted 7 lines

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just dead code, delete it.

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check is needed to ensure single call to mlx5_notifier_unregister, because calls to mlx5_ib_unbind_slave_port() will be serialized.

Quoted 2 lines

If this routine is now relying on locking that is not obvious in the function itself then add a lockdep too.

It was "before" without lockdep and we are protecting "mpi->mdev_events.notifier_call = NULL;"

Quoted 54 lines · Leon Romanovsky

Quoted 52 lines · Jason Gunthorpe

Quoted 41 lines · Leon Romanovsky

Quoted 40 lines · Jason Gunthorpe

Quoted 29 lines · Leon Romanovsky

Quoted 28 lines · Jason Gunthorpe

Quoted 26 lines · Doug Ledford

Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void mlx5_ib_unbind_slave_port(struct
 mlx5_ib_dev *ibdev,
 		return;
 	}
 

drivers/infiniband/hw/mlx5/main.c static void mlx5_ib_unbind_slave_port(struct · line 5811

Quoted 10 lines

drivers/infiniband/hw/mlx5/main.c +3 -4

-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev, &mpi->mdev_events);
+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the appearance of creating a new race. Doing a check/unregister/set-null series outside of any locks is a red flag to someone investigating the code. You should at least make note of the fact that calling unregister more than once is safe. If you're fine with it, I can add a comment and take the patch, or you can resubmit.

Quoted 4 lines

Quoted 2 lines

Mucking about notifier_call like that is gross anyhow, maybe better to delete it entirely.

What do you propose to delete?

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

Quoted 7 lines

Quoted 2 lines

If you need it, then we can't drop the lock, if you don't, it is just dead code, delete it.

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check is needed to ensure single call to mlx5_notifier_unregister, because calls to mlx5_ib_unbind_slave_port() will be serialized.

If this routine is now relying on locking that is not obvious in the function itself then add a lockdep too.

Quoted 2 lines

It was "before" without lockdep and we are protecting "mpi->mdev_events.notifier_call = NULL;"

Before the locking was relying on mpi_lock inside this function now this patch changes it to relies on mlx5_ib_multiport_mutex, so it needs a lockdep

Jason

Quoted 75 lines · Jason Gunthorpe

Quoted 73 lines · Leon Romanovsky

Quoted 66 lines · Jason Gunthorpe

Quoted 58 lines · Leon Romanovsky

Quoted 47 lines · Jason Gunthorpe

Quoted 39 lines · Leon Romanovsky

Quoted 37 lines · Jason Gunthorpe

Quoted 34 lines · Doug Ledford

Quoted 21 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void

mlx5_ib_unbind_slave_port(struct
mlx5_ib_dev *ibdev,

drivers/infiniband/hw/mlx5/main.c +0 -2

 		return;
 	}
 
-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev,

&mpi->mdev_events);

drivers/infiniband/hw/mlx5/main.c +2 -2

-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev,

&mpi->mdev_events);

drivers/infiniband/hw/mlx5/main.c

Quoted 1 line

drivers/infiniband/hw/mlx5/main.c +1 -0

+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the
appearance of creating a new race. Doing a check/unregister/set-null
series outside of any locks is a red flag to someone investigating the
code. You should at least make note of the fact that calling unregister
more than once is safe. If you're fine with it, I can add a comment and
take the patch, or you can resubmit.

Quoted 5 lines

Quoted 3 lines

Mucking about notifier_call like that is gross anyhow, maybe better to
delete it entirely.

What do you propose to delete?

Quoted 8 lines

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just
dead code, delete it.

Quoted 6 lines

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check
is needed to ensure single call to mlx5_notifier_unregister, because
calls to mlx5_ib_unbind_slave_port() will be serialized.

Quoted 3 lines

If this routine is now relying on locking that is not obvious in the
function itself then add a lockdep too.

It was "before" without lockdep and we are protecting "mpi->mdev_events.notifier_call = NULL;"

Quoted 3 lines

Before the locking was relying on mpi_lock inside this function now this patch changes it to relies on mlx5_ib_multiport_mutex, so it needs a lockdep

It didn't rely, but was moved by mistake. I'll add lockdep and resend.

Thanks

Quoted 76 lines · Leon Romanovsky

Quoted 75 lines · Jason Gunthorpe

Quoted 73 lines · Leon Romanovsky

Quoted 66 lines · Jason Gunthorpe

Quoted 58 lines · Leon Romanovsky

Quoted 47 lines · Jason Gunthorpe

Quoted 39 lines · Leon Romanovsky

Quoted 37 lines · Jason Gunthorpe

Quoted 34 lines · Doug Ledford

Quoted 21 lines

drivers/infiniband/hw/mlx5/main.c +1 -0

index c2a5780cb394..e12a4404096b 100644
+++ b/drivers/infiniband/hw/mlx5/main.c

@@ -5802,13 +5802,12 @@ static void

mlx5_ib_unbind_slave_port(struct
mlx5_ib_dev *ibdev,

drivers/infiniband/hw/mlx5/main.c +0 -2

 		return;
 	}
 
-	if (mpi->mdev_events.notifier_call)
-		mlx5_notifier_unregister(mpi->mdev,

&mpi->mdev_events);

drivers/infiniband/hw/mlx5/main.c +2 -2

-	mpi->mdev_events.notifier_call = NULL;
-
 	mpi->ibdev = NULL;
 
 	spin_unlock(&port->mp.mpi_lock);
+	if (mpi->mdev_events.notifier_call)
+		mlx5_notifier_unregister(mpi->mdev,

&mpi->mdev_events);

drivers/infiniband/hw/mlx5/main.c

Quoted 1 line

drivers/infiniband/hw/mlx5/main.c +1 -0

+	mpi->mdev_events.notifier_call = NULL;

I can see where this fixes the problem at hand, but this gives the
appearance of creating a new race. Doing a check/unregister/set-null
series outside of any locks is a red flag to someone investigating the
code. You should at least make note of the fact that calling unregister
more than once is safe. If you're fine with it, I can add a comment and
take the patch, or you can resubmit.

Quoted 5 lines

Quoted 3 lines

Mucking about notifier_call like that is gross anyhow, maybe better to
delete it entirely.

What do you propose to delete?

Quoted 8 lines

The 'mpi->mdev_events.notifier_call = NULL;' and 'if (mpi->mdev_events.notifier_call)'

Once it leaves the lock it stops doing anything useful.

If you need it, then we can't drop the lock, if you don't, it is just
dead code, delete it.

Quoted 6 lines

This specific notifier_call is protected outside
of mlx5_ib_unbind_slave_port() by mlx5_ib_multiport_mutex and NULL check
is needed to ensure single call to mlx5_notifier_unregister, because
calls to mlx5_ib_unbind_slave_port() will be serialized.

Quoted 3 lines

If this routine is now relying on locking that is not obvious in the
function itself then add a lockdep too.

It was "before" without lockdep and we are protecting "mpi->mdev_events.notifier_call = NULL;"

Quoted 7 lines

Quoted 3 lines

Before the locking was relying on mpi_lock inside this function now this patch changes it to relies on mlx5_ib_multiport_mutex, so it needs a lockdep

It didn't rely, but was moved by mistake. I'll add lockdep and resend.

Thanks

There's no need for a lockdep. The removal of the notifier callback entry is re-entrant safe. The core removal routines have their own spinlock they use to protect the actual notifier list. If you call it more than once, the second and subsequent calls merely scan the list, find no matching entry, and return ENOENT. The only reason this might need a lock and a lockdep entry is if you are protecting against a race with the *add* notifier code in the mlx5 driver specifically (the core add code won't have an issue, but since you only have a single place to store the notifier callback pointer, if it would be possible for you to add two callbacks and write over the first callback pointer with the second without removing the first, then you would leak a callback notifier in the core notifier list).

--
Doug Ledford <[email protected]>

    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Quoted 11 lines

There's no need for a lockdep. The removal of the notifier callback entry is re-entrant safe. The core removal routines have their own spinlock they use to protect the actual notifier list. If you call it
more than once, the second and subsequent calls merely scan the list,
find no matching entry, and return ENOENT. The only reason this might
need a lock and a lockdep entry is if you are protecting against a race
with the *add* notifier code in the mlx5 driver specifically (the

Quoted 8 lines

core
add code won't have an issue, but since you only have a single place to
store the notifier callback pointer, if it would be possible for you to
add two callbacks and write over the first callback pointer with the second without removing the first, then you would leak a callback notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do for every port unbind.

Actually, I'm completely lost here, we are all agree that the patch fixes issue correctly, and it returns the code to be exactly as it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier
API"). Can we simply merge it and fix the kernel panic?

Quoted 24 lines · Leon Romanovsky

Quoted 11 lines

There's no need for a lockdep. The removal of the notifier callback entry is re-entrant safe. The core removal routines have their own spinlock they use to protect the actual notifier list. If you call it
more than once, the second and subsequent calls merely scan the list,
find no matching entry, and return ENOENT. The only reason this might
need a lock and a lockdep entry is if you are protecting against a race
with the *add* notifier code in the mlx5 driver specifically (the

Quoted 8 lines

core
add code won't have an issue, but since you only have a single place to
store the notifier callback pointer, if it would be possible for you to
add two callbacks and write over the first callback pointer with the second without removing the first, then you would leak a callback notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do for every port unbind.

Quoted 5 lines

Actually, I'm completely lost here, we are all agree that the patch fixes issue correctly, and it returns the code to be exactly as it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier
API"). Can we simply merge it and fix the kernel panic?

As long as you are OK with me adding a comment to the patch so people coming back later won't scratch their head about how can it possible be right to do that sequence without a lock held, I'm fine merging the fix.

Something like:

/*
* The check/unregister/set-NULL sequence below does not need to be
* locked for correctness as it's only an optimization, and can't
* be under a lock or will throw a scheduling while atomic error. */

Quoted 50 lines · Doug Ledford

Quoted 33 lines · Leon Romanovsky

Quoted 20 lines

There's no need for a lockdep. The removal of the notifier callback
entry is re-entrant safe. The core removal routines have their own
spinlock they use to protect the actual notifier list. If you call
it
more than once, the second and subsequent calls merely scan the list,
find no matching entry, and return ENOENT. The only reason this might
need a lock and a lockdep entry is if you are protecting against a
race
with the *add* notifier code in the mlx5 driver specifically (the
core
add code won't have an issue, but since you only have a single place
to

Quoted 8 lines

store the notifier callback pointer, if it would be possible for you
to
add two callbacks and write over the first callback pointer with the
second without removing the first, then you would leak a callback
notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do for every port unbind.

Quoted 6 lines

Actually, I'm completely lost here, we are all agree that the patch
fixes issue correctly, and it returns the code to be exactly as it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier
API"). Can we simply merge it and fix the kernel panic?

As long as you are OK with me adding a comment to the patch so people
coming back later won't scratch their head about how can it possible be
right to do that sequence without a lock held, I'm fine merging the fix.

Something like:

Quoted 5 lines

/*
* The check/unregister/set-NULL sequence below does not need to be
* locked for correctness as it's only an optimization, and can't
* be under a lock or will throw a scheduling while atomic error. */

It does have a lock though, the caller holds it, hence the request for the lockdep.

Quoted 2 lines

It does have a lock though, the caller holds it, hence the request for the lockdep.

You're right, although I think the lockdep annotation can be a separate patch as it's neeeded on more than just the function this patch touches.

Quoted 4 lines · Doug Ledford

Quoted 3 lines

It does have a lock though, the caller holds it, hence the request for
the lockdep.

Quoted 4 lines

You're right, although I think the lockdep annotation can be a separate
patch as it's neeeded on more than just the function this patch touches.

Why? This relies on that lock, so it should have the lockdep_assert_held assert.

If there are more functions with implicit locking theyt they can be fixed separately...

Quoted 11 lines · Jason Gunthorpe

Quoted 8 lines · Doug Ledford

Quoted 3 lines

It does have a lock though, the caller holds it, hence the request for
the lockdep.

You're right, although I think the lockdep annotation can be a separate
patch as it's neeeded on more than just the function this patch touches.

Quoted 2 lines

Why? This relies on that lock, so it should have the lockdep_assert_held assert.

It does, but this patch is about the scheduling while atomic, adding a lockdep assertion fix is doubling up on fixes in the patch. A separate patch that addes the lockdep assert to both the bind and unbind calls makes more sense and just feels cleaner to me.

Quoted 3 lines

If there are more functions with implicit locking theyt they can be fixed separately...

--
Doug Ledford <[email protected]>

    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Quoted 24 lines · Leon Romanovsky

Quoted 11 lines

There's no need for a lockdep. The removal of the notifier callback entry is re-entrant safe. The core removal routines have their own spinlock they use to protect the actual notifier list. If you call it
more than once, the second and subsequent calls merely scan the list,
find no matching entry, and return ENOENT. The only reason this might
need a lock and a lockdep entry is if you are protecting against a race
with the *add* notifier code in the mlx5 driver specifically (the

Quoted 8 lines

core
add code won't have an issue, but since you only have a single place to
store the notifier callback pointer, if it would be possible for you to
add two callbacks and write over the first callback pointer with the second without removing the first, then you would leak a callback notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do for every port unbind.

Quoted 5 lines

Actually, I'm completely lost here, we are all agree that the patch fixes issue correctly, and it returns the code to be exactly as it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier
API"). Can we simply merge it and fix the kernel panic?

As long as you are OK with me adding a comment to the patch so people coming back later won't scratch their head about how can it possible be right to do that sequence without a lock held, I'm fine merging the fix.

Something like:

/*
* The check/unregister/set-NULL sequence below does not need to be
* locked for correctness as it's only an optimization, and can't
* be under a lock or will throw a scheduling while atomic error. */

--
Doug Ledford <[email protected]>

    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Quoted 48 lines · Doug Ledford

Quoted 33 lines · Leon Romanovsky

Quoted 20 lines

There's no need for a lockdep. The removal of the notifier callback
entry is re-entrant safe. The core removal routines have their own
spinlock they use to protect the actual notifier list. If you call
it
more than once, the second and subsequent calls merely scan the list,
find no matching entry, and return ENOENT. The only reason this might
need a lock and a lockdep entry is if you are protecting against a
race
with the *add* notifier code in the mlx5 driver specifically (the
core
add code won't have an issue, but since you only have a single place
to

Quoted 8 lines

store the notifier callback pointer, if it would be possible for you
to
add two callbacks and write over the first callback pointer with the
second without removing the first, then you would leak a callback
notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do for every port unbind.

Quoted 6 lines

Actually, I'm completely lost here, we are all agree that the patch
fixes issue correctly, and it returns the code to be exactly as it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier
API"). Can we simply merge it and fix the kernel panic?

As long as you are OK with me adding a comment to the patch so people
coming back later won't scratch their head about how can it possible be
right to do that sequence without a lock held, I'm fine merging the fix.

Quoted 7 lines

Something like:

/*
* The check/unregister/set-NULL sequence below does not need to be
* locked for correctness as it's only an optimization, and can't
* be under a lock or will throw a scheduling while atomic error. */

I think that the best place will be in commit message for this explanation,
but I'm fine with the comment inside code as well.

Thanks a lot, I appreciate it.

Quoted 57 lines · Leon Romanovsky

Quoted 55 lines · Doug Ledford

Quoted 33 lines · Leon Romanovsky

Quoted 20 lines

There's no need for a lockdep. The removal of the notifier callback
entry is re-entrant safe. The core removal routines have their own
spinlock they use to protect the actual notifier list. If you call
it
more than once, the second and subsequent calls merely scan the list,
find no matching entry, and return ENOENT. The only reason this might
need a lock and a lockdep entry is if you are protecting against a
race
with the *add* notifier code in the mlx5 driver specifically (the
core
add code won't have an issue, but since you only have a single place
to

Quoted 8 lines

store the notifier callback pointer, if it would be possible for you
to
add two callbacks and write over the first callback pointer with the
second without removing the first, then you would leak a callback
notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do for every port unbind.

Quoted 6 lines

Actually, I'm completely lost here, we are all agree that the patch
fixes issue correctly, and it returns the code to be exactly as it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core notifier
API"). Can we simply merge it and fix the kernel panic?

As long as you are OK with me adding a comment to the patch so people
coming back later won't scratch their head about how can it possible be
right to do that sequence without a lock held, I'm fine merging the fix.

Something like:

/*
* The check/unregister/set-NULL sequence below does not need to be
* locked for correctness as it's only an optimization, and can't
* be under a lock or will throw a scheduling while atomic error. */

Quoted 5 lines

I think that the best place will be in commit message for this explanation,
but I'm fine with the comment inside code as well.

Thanks a lot, I appreciate it.

Patch (unmodified) is applied to for-rc, thanks.

--
Doug Ledford <[email protected]>

    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Quoted 78 lines · Doug Ledford

Quoted 77 lines · Leon Romanovsky

Quoted 63 lines · Doug Ledford

Quoted 42 lines · Leon Romanovsky

Quoted 28 lines

There's no need for a lockdep. The removal of the notifier callback
entry is re-entrant safe. The core removal routines have their
own
spinlock they use to protect the actual notifier list. If you
call
it
more than once, the second and subsequent calls merely scan the
list,
find no matching entry, and return ENOENT. The only reason this
might
need a lock and a lockdep entry is if you are protecting against
a
race
with the *add* notifier code in the mlx5 driver specifically (the
core
add code won't have an issue, but since you only have a single
place
to
store the notifier callback pointer, if it would be possible for

Quoted 8 lines

you
to
add two callbacks and write over the first callback pointer with
the
second without removing the first, then you would leak a callback
notifier in the core notifier list).

atomic_notifier_chain_unregister() unconditionally calls to syncronize_rcu() and I'm not so sure that it is best thing to do
for every port unbind.

Quoted 8 lines

Actually, I'm completely lost here, we are all agree that the patch
fixes issue correctly, and it returns the code to be exactly as
it was before commit df097a278c75 ("IB/mlx5: Use the new mlx5 core
notifier
API"). Can we simply merge it and fix the kernel panic?

As long as you are OK with me adding a comment to the patch so people
coming back later won't scratch their head about how can it possible
be
right to do that sequence without a lock held, I'm fine merging the
fix.

Something like:

Quoted 8 lines

/*
* The check/unregister/set-NULL sequence below does not need to
be
* locked for correctness as it's only an optimization, and
can't
* be under a lock or will throw a scheduling while atomic
error.
*/

I think that the best place will be in commit message for this explanation,
but I'm fine with the comment inside code as well.

Quoted 3 lines

Quoted 1 line

Thanks a lot, I appreciate it.

Patch (unmodified) is applied to for-rc, thanks.

Thanks Doug, I'll prepare patch with lockdep for Jason and will submit it to -next later on after passing verification.