qla2xxx: Fix dpc_thread race on the module unload

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

Signed-off-by Vladislav Bolkhovitin <[email protected]>

Quoted 3 lines · Vladislav Bolkhovitin

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

Quoted 3 lines

This patch is against 2.6.26

Signed-off-by Vladislav Bolkhovitin <[email protected]>

Vladislav,

Could you provide some details on the failure you encountered which prompted this patch (backtrace/reproduction method)?

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

Quoted 4 lines · Vladislav Bolkhovitin

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

I'm afraid adding a lock is almost certainly the wrong way to handle this type of failure.

What should be done is to make sure the qla is correctly shut down (i.e. no tasks requiring the dpc_thread can be performed) *before* killing the thread ...

it sounds like shutdown is
slightly broken in the current driver ... could you post the oops details and we can try to work out what the problem is

James

Quoted 8 lines · James Bottomley

Quoted 4 lines · Vladislav Bolkhovitin

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

I'm afraid adding a lock is almost certainly the wrong way to handle this type of failure.

Why? It's simple and fully solves the problem. All the events, which left unhandled, because there is nobody to wake up by qla2xxx_wake_dpc(), are not relevant after the driver's shutdown.

Quoted 3 lines

What should be done is to make sure the qla is correctly shut down (i.e. no tasks requiring the dpc_thread can be performed) *before* killing the thread ...

Sure, in ideal it would be the best approach. But, certainly, it would be a lot more complicated and error-prone.

From other side, actually, it doesn't matter much for me how it will be
fixed, if it's fixed.

Quoted 3 lines

it sounds like shutdown is
slightly broken in the current driver ... could you post the oops details and we can try to work out what the problem is

Gal, can you send the details, please?

Quoted 4 lines

James

Quoted 9 lines · James Bottomley

Quoted 8 lines · Vladislav Bolkhovitin

Nope, taking only one that hunk from this patch isn't sufficient. Around
dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)
y = *x;

without any protection, if x can be set to NULL by another thread.

Quoted 4 lines

Quoted 2 lines

can happen exactly between "if" and "*x" and hence lead to a crash, correct?

No.

What "No"? The above unlocked "if (x != NULL) y = *x;" is always safe now? ;)

Nope, taking only one that hunk from this patch isn't sufficient. Around
dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)
y = *x;

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

Andrew,

Add checking of the flag online still does not answer the race that Vlad stated before,

unless you thought about another way without lock to protect this thread.

Quoted 8 lines

Nope, taking only one that hunk from this patch isn't sufficient. Around dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)
y = *x;

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

Gal.

Quoted 4 lines · Gal Rosen

Andrew,

Add checking of the flag online still does not answer the race that Vlad stated before,

Yes, that's true.

And, since ha->flags.online set to 0 not only on shutdown, I'm afraid you could introduce a new set of subtle bugs, if not for the moment, but in the future, because with your patch it gets impossible to wake up the DPC thread if HA is offline.

Quoted 8 lines · Vladislav Bolkhovitin

Quoted 4 lines · Gal Rosen

Andrew,

Add checking of the flag online still does not answer the race that Vlad stated before,

Yes, that's true.

And, since ha->flags.online set to 0 not only on shutdown, I'm afraid you could introduce a new set of subtle bugs, if not for the moment, but in the future, because with your patch it gets impossible to wake up the DPC thread if HA is offline.

You are both missing a subtle point, it's incidental if qla2xxx_wake_dpc() misses a 'wake-up' due the HBA being 'offline', as the qla2x00_timer(), woken up every second, will do the wake-up if necessary. The code changes offered, close the window as tightly as possible without introducing needlessly complex changes. All this infrastructure is legacy constucts from a time long-before work-queues and the like.

This area is one of several where we are moving to clean-up and modernize in our upstream offering:

* dropping the heavy DPC thread in favor of work-queues.
* using proper life-cycle and reference-handling of fcport objects (yes, the objects will be freed after use).
* refactoring the HA respresentations for physical and virtual ports, the current memcpy() physical-HA to vport-HA and slight-mods is error-prone and doesn't scale.
* drop the illogical single physical-HA maintains all fcports across N-vports.

The changes above are large (170k diffs so far), and at this point are being run-through our testing. The hope is to get the changes upstream during one of the next two merge windows.

Given the infrustructure mods and our focus on that front, if there's something small and contained you can offer above what I've proposed we'll be interested in reviewing any patches you'd push forward.

Regards,
Andrew Vasquez
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines

The changes above are large (170k diffs so far), and at this point are being run-through our testing. The hope is to get the changes upstream during one of the next two merge windows.

Given the infrustructure mods and our focus on that front, if there's something small and contained you can offer above what I've proposed we'll be interested in reviewing any patches you'd push forward.

Then, I believe, my patch should go in as a temporal measure. I don't think we should crash users for 2 more major releases.

The same is true for my
other patch "Proposed protection of fcports field of struct scsi_qla_host" as well, because without it there should be no big problems to crash the driver via sysfs.

Quoted 7 lines · Vladislav Bolkhovitin

Quoted 7 lines

The changes above are large (170k diffs so far), and at this point are being run-through our testing. The hope is to get the changes upstream during one of the next two merge windows.

Given the infrustructure mods and our focus on that front, if there's something small and contained you can offer above what I've proposed we'll be interested in reviewing any patches you'd push forward.

Quoted 1 line

Then, I believe, my patch should go in as a temporal measure. I don't think we should crash users for 2 more major releases.

So the 'online' check concerns you? So, add an 'unloading' flag, set it on remove_one(), save a copy of dpc_thread at qla2xxx_wake_dpc(), then wake_up() saved value if not 'unloading'. Of course the direct wake_up() in request_acqusition should be modded to call qla2xxx_wake_dpc(). We're not talking about some huge window here.

Quoted 1 line

The same is true for my other patch "Proposed protection of fcports field of struct scsi_qla_host" as well, because without it there should be no big problems to crash the driver via sysfs.

We're still looking through your patches... So how exactly would it be 'be no big problems to crash the driver via sysfs.'???

Quoted 3 lines · Vladislav Bolkhovitin

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

Quoted 1 line

This patch is against 2.6.26

I'm afraid adding a lock is almost certainly the wrong way to handle this type of failure. What should be done is to make sure the qla is correctly shut down (i.e. no tasks requiring the dpc_thread can be performed) *before* killing the thread ... it sounds like shutdown is slightly broken in the current driver ... could you post the oops details and we can try to work out what the problem is

Quoted 11 lines · James Bottomley

Quoted 4 lines · Vladislav Bolkhovitin

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

I'm afraid adding a lock is almost certainly the wrong way to handle this type of failure. What should be done is to make sure the qla is correctly shut down (i.e. no tasks requiring the dpc_thread can be performed) *before* killing the thread ... it sounds like shutdown is slightly broken in the current driver ... could you post the oops details and we can try to work out what the problem is

I'm not even sure it's the current driver as he mentions that he using some out of tree target driver which might be the problem.

Can you reproduce it with a stock 2.6.27-rc tree?

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

Signed-off-by Vladislav Bolkhovitin <[email protected]>

 qla_def.h |    1 +
 qla_mbx.c |    2 +-
 qla_os.c  |   36 ++++++++++++++++++++++++++----------
 3 files changed, 28 insertions(+), 11 deletions(-)

diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h

Quoted 16 lines · Vladislav Bolkhovitin

This patch fixes race on dpc_thread field of struct scsi_qla_host, which can lead to crash on the module unload.

This patch is against 2.6.26

Signed-off-by Vladislav Bolkhovitin <[email protected]>

 qla_def.h |    1 +
 qla_mbx.c |    2 +-
 qla_os.c  |   36 ++++++++++++++++++++++++++----------
 3 files changed, 28 insertions(+), 11 deletions(-)

diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h

drivers/scsi/qla2xxx/qla_def.h typedef struct scsi_qla_host { · line 2439

Quoted 4 lines

drivers/scsi/qla2xxx/qla_def.h +1 -0

@@ -2425,6 +2436,7 @@ typedef struct scsi_qla_host {
 	void			*sfp_data;
 	dma_addr_t		sfp_data_dma;
 
+	spinlock_t		dpc_lock;

As James mentioned in another email, there's something else going on with qla2xxx shutdown process which should be addressed...

drivers/scsi/qla2xxx/qla_mbx.c qla24xx_report_id_acquisition(scsi_qla_h · line 2690

Quoted 12 lines

drivers/scsi/qla2xxx/qla_def.h +0 -0

 	struct task_struct	*dpc_thread;
 	uint8_t dpc_active;                  /* DPC routine is active */
 

diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_mbx.c linux-2.6.26/drivers/scsi/qla2xxx/qla_mbx.c

drivers/scsi/qla2xxx/qla_mbx.c +1 -1

@@ -2683,7 +2687,7 @@ qla24xx_report_id_acquisition(scsi_qla_h
 		set_bit(VP_IDX_ACQUIRED, &vha->vp_flags);
 		set_bit(VP_DPC_NEEDED, &ha->dpc_flags);
 
-		wake_up_process(ha->dpc_thread);
+		qla2xxx_wake_dpc(ha);

Ok, your change looks correct here... This is defintely the correct way the qla24xx_report_id_acquisition() routine should be triggering the DPC thread.

I'm guessing the backtrace in "Gal Rosen's" report pointed to qla24xx_report_id_acquisition()? If, so, I'm not entirely sure we need anything more to 'protect' tear-down...

Nope, taking only one that hunk from this patch isn't sufficient. Around
dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)

        y = *x;

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

Quoted 8 lines · Vladislav Bolkhovitin

Nope, taking only one that hunk from this patch isn't sufficient. Around dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)

        y = *x;

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

No.

Today we go to reasonable lengths to avoid additional locking. Spinlocks are nasty in most boxes because of the potential for introducing hot points and cacheline bouncing.

The first question you ask is how hot is the variable ... as in how constantly changing is it? This one is set at start of day and cleared when the thread is killed in shutdown. Basically there's a single not NULL -> NULL transition. Once NULL, it never goes back to not NULL.

The point is that in this particular circumstance, the lock is overkill. You can either use other indicators to show that the driver is being removed (and the thread is dying) or you can simply use some type of refcounting to ensure the thread isn't killed until it's no longer needed.

Even if it were a constantly changing variable, and therefore a more ideal candidate for locking, we might still look into atomics and bitops before adding a lock.

Quoted 8 lines · James Bottomley

Quoted 7 lines · Vladislav Bolkhovitin

Nope, taking only one that hunk from this patch isn't sufficient. Around
dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)

        y = *x;

Quoted 5 lines

Quoted 3 lines

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

No.

What "No"? The above unlocked "if (x != NULL) y = *x;" is always safe now? ;)

Quoted 15 lines

Today we go to reasonable lengths to avoid additional locking. Spinlocks are nasty in most boxes because of the potential for introducing hot points and cacheline bouncing.

The first question you ask is how hot is the variable ... as in how constantly changing is it? This one is set at start of day and cleared when the thread is killed in shutdown. Basically there's a single not NULL -> NULL transition. Once NULL, it never goes back to not NULL.

The point is that in this particular circumstance, the lock is overkill. You can either use other indicators to show that the driver is being removed (and the thread is dying) or you can simply use some type of refcounting to ensure the thread isn't killed until it's no longer needed.

Quoted 3 lines

Even if it were a constantly changing variable, and therefore a more ideal candidate for locking, we might still look into atomics and bitops before adding a lock.

Sure, all you wrote above is correct, but in this *particular* case what you propose is an overkill, not use of the spinlock. Waking up DPC thread isn't on the hot path by any mean, it's quite rare event. So, all those lockfree techniques comparing to a simple single lock would introduce only additional complicated code without any measurable gain.

But, as I already wrote, I don't care much how this problem will be fixed. I care only that it should be fixed. So, my point was only that use from my patch only one hunk

drivers/scsi/qla2xxx/qla_mbx.c +1 -1

-		wake_up_process(ha->dpc_thread);
+		qla2xxx_wake_dpc(ha);

is insufficient to fix the problem.

Quoted 5 lines · Vladislav Bolkhovitin

Quoted 4 lines · James Bottomley

Quoted 3 lines · Vladislav Bolkhovitin

Nope, taking only one that hunk from this patch isn't sufficient. Around dpc_thread there is pretty simple and classical race. You can't do

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

if (x != NULL)

        y = *x;

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

No.

What "No"? The above unlocked "if (x != NULL) y = *x;" is always safe now? ;)

No ... no as in your analysis based on the example is not correct to conclude protection is required. We have quite a number of examples of this within the linux kernel (the SCSI error thread would be one).

Quoted 15 lines

Quoted 15 lines

Today we go to reasonable lengths to avoid additional locking. Spinlocks are nasty in most boxes because of the potential for introducing hot points and cacheline bouncing.

The first question you ask is how hot is the variable ... as in how constantly changing is it? This one is set at start of day and cleared when the thread is killed in shutdown. Basically there's a single not NULL -> NULL transition. Once NULL, it never goes back to not NULL.

The point is that in this particular circumstance, the lock is overkill. You can either use other indicators to show that the driver is being removed (and the thread is dying) or you can simply use some type of refcounting to ensure the thread isn't killed until it's no longer needed.

Quoted 4 lines

Quoted 3 lines

Even if it were a constantly changing variable, and therefore a more ideal candidate for locking, we might still look into atomics and bitops before adding a lock.

Sure, all you wrote above is correct, but in this *particular* case what you propose is an overkill, not use of the spinlock. Waking up DPC thread isn't on the hot path by any mean, it's quite rare event. So, all those lockfree techniques comparing to a simple single lock would introduce only additional complicated code without any measurable gain.

It's important to think about this kind of thing even in cases where it doesn't necessarily matter that much just so it becomes standard practice in cases where it does.

Quoted 6 lines

But, as I already wrote, I don't care much how this problem will be fixed. I care only that it should be fixed. So, my point was only that use from my patch only one hunk

drivers/scsi/qla2xxx/qla_mbx.c +1 -1

-		wake_up_process(ha->dpc_thread);
+		qla2xxx_wake_dpc(ha);

is insufficient to fix the problem.

Yes, I agree with that ... I just want to see a better fix.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines

Ok, your change looks correct here... This is defintely the correct way the qla24xx_report_id_acquisition() routine should be triggering the DPC thread.

I'm guessing the backtrace in "Gal Rosen's" report pointed to qla24xx_report_id_acquisition()? If, so, I'm not entirely sure we need anything more to 'protect' tear-down...

Nope, taking only one that hunk from this patch isn't sufficient. Around dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)

	y = *x;

without any protection, if x can be set to NULL by another thread. It can happen exactly between "if" and "*x" and hence lead to a crash, correct?

What this patch does is adding such protection.