Re: [PATCH] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure
On 02/27/2018 04:18 PM, Kuzeja, William wrote:
>
> Thanks for the quick reply and analysis Hannes! My apologies in advance, I'm
> stuck on
> Outlook here at work - I'll try to format this to be readable (hopefully it
> doesn't get
> mangled).
>
> On 02/27/2018 06:01 AM, Hannes Reinecke wrote:
>
>> Hmm. Isn't is rather the case that the labels and gotos are mixed up?
>> We have this order of labels:
>
>> probe_init_failed
>> probe_failed
>> probe_hw_failed
>> iospace_config_failed
>> disable_device
>
>> but this potential calling order:
>
>> disable_device
>> iospace_config_failed
>> probe_hw_failed
>> probe_hw_failed
>> probe_init_failed
>> probe_failed
>> probe_failed
>> probe_failed
>
>> So it looks to me as if the 'probe_init_failed' and 'probe_failed'
>> labels should be exchanged...
>> But yeah, we need to fix this up.
>> I've run into this issue myself recently :-(
>
> So, that is partly the problem. But.if we switch the order, we still have
> problems.
> Consider the code with order switched:
>
> probe_failed:
>
> qla2x00_free_device(base_vha);
> scsi_host_put(base_vha->host);
>
> probe_init_failed:
> qla2x00_free_req_que(ha, req);
> ha->req_q_map[0] = NULL; alloc_queues succeeds
> clear_bit(0, ha->req_qid_map);
> qla2x00_free_rsp_que(ha, rsp);
> ha->rsp_q_map[0] = NULL;
> clear_bit(0, ha->rsp_qid_map);
> ha->max_req_queues = ha->max_rsp_queues = 0;
>
> probe_hw_failed:
> qla2x00_mem_free(ha);
> qla2x00_free_req_que(ha, req);
> qla2x00_free_rsp_que(ha, rsp);
> qla2x00_clear_drv_active(ha);
>
> 1) We call probe_init_failed only if qla2x00_alloc_queues fails. But if this
> routine fails,
> ha->req_q_map = NULL (as well as rsp_q_map), so we crash doing
> ha->req_q_map[0] = NULL. So, I can remove these assignments. But the
> qla2x00_free_req_que calls are redundant (they are done below), so we're left
> with clearing out max_req_queues and max_rsp_queues for this label. Since
> we're freeing ha anyways, I'm not sure if this is worth having another label
> for.
>
> 2) We still have the problem where in probe_failed, qla2x00_free_device does
> frees which are repeated later on in probe_hw_failed.
>
> qla2x00_free_device -> qla2x00_mem_free
> qla2x00_free_device -> qla2x00_free_queues -> qla2x00_free_req_que
>
> 3) Also, I still need another label for when qla2x00_mem_alloc fails. We
> can't just
> jump to probe_hw_failed.
>
> I suppose we could do something like this:
>
> probe_failed:
> probe_failed = 1;
>
> qla2x00_free_device(base_vha);
> scsi_host_put(base_vha->host);
>
> probe_init_failed:
> ha->max_req_queues = ha->max_rsp_queues = 0;
>
> probe_hw_failed:
> if (!probe_failed) {
> qla2x00_mem_free(ha);
> qla2x00_free_req_que(ha, req);
> qla2x00_free_rsp_que(ha, rsp);
> }
Why not make all of these idempotent, so that they won't crash when
called twice? Then we can drop the 'probe_failed' marker, and the entire
code becomes more readable.
Plus the diff will be smaller, too...
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
RE: [PATCH] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure
Thanks for the quick reply and analysis Hannes! My apologies in advance, I'm
stuck on
Outlook here at work - I'll try to format this to be readable (hopefully it
doesn't get
mangled).
On 02/27/2018 06:01 AM, Hannes Reinecke wrote:
> Hmm. Isn't is rather the case that the labels and gotos are mixed up?
> We have this order of labels:
> probe_init_failed
> probe_failed
> probe_hw_failed
> iospace_config_failed
> disable_device
> but this potential calling order:
> disable_device
> iospace_config_failed
> probe_hw_failed
> probe_hw_failed
> probe_init_failed
> probe_failed
> probe_failed
> probe_failed
> So it looks to me as if the 'probe_init_failed' and 'probe_failed'
> labels should be exchanged...
> But yeah, we need to fix this up.
> I've run into this issue myself recently :-(
So, that is partly the problem. But.if we switch the order, we still have
problems.
Consider the code with order switched:
probe_failed:
qla2x00_free_device(base_vha);
scsi_host_put(base_vha->host);
probe_init_failed:
qla2x00_free_req_que(ha, req);
ha->req_q_map[0] = NULL; alloc_queues succeeds
clear_bit(0, ha->req_qid_map);
qla2x00_free_rsp_que(ha, rsp);
ha->rsp_q_map[0] = NULL;
clear_bit(0, ha->rsp_qid_map);
ha->max_req_queues = ha->max_rsp_queues = 0;
probe_hw_failed:
qla2x00_mem_free(ha);
qla2x00_free_req_que(ha, req);
qla2x00_free_rsp_que(ha, rsp);
qla2x00_clear_drv_active(ha);
1) We call probe_init_failed only if qla2x00_alloc_queues fails. But if this
routine fails,
ha->req_q_map = NULL (as well as rsp_q_map), so we crash doing
ha->req_q_map[0] = NULL. So, I can remove these assignments. But the
qla2x00_free_req_que calls are redundant (they are done below), so we're left
with clearing out max_req_queues and max_rsp_queues for this label. Since
we're freeing ha anyways, I'm not sure if this is worth having another label
for.
2) We still have the problem where in probe_failed, qla2x00_free_device does
frees which are repeated later on in probe_hw_failed.
qla2x00_free_device -> qla2x00_mem_free
qla2x00_free_device -> qla2x00_free_queues -> qla2x00_free_req_que
3) Also, I still need another label for when qla2x00_mem_alloc fails. We can't
just
jump to probe_hw_failed.
I suppose we could do something like this:
probe_failed:
probe_failed = 1;
qla2x00_free_device(base_vha);
scsi_host_put(base_vha->host);
probe_init_failed:
ha->max_req_queues = ha->max_rsp_queues = 0;
probe_hw_failed:
if (!probe_failed) {
qla2x00_mem_free(ha);
qla2x00_free_req_que(ha, req);
qla2x00_free_rsp_que(ha, rsp);
}
probe_hw_failed_no_mem:
qla2x00_clear_drv_active(ha);
This is essentially what I have, except my version does not have the
probe_init_failed
label. Thoughts??
Thanks again and regards.
---
Re: [PATCH] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure
On 02/26/2018 11:37 PM, Bill Kuzeja wrote:
> Because of the shifting around of code in qla2x00_probe_one recently,
> failures during adapter initialization can lead to problems, i.e. NULL
> pointer crashes and doubly freed data structures which cause eventual
> panics.
>
> To address these problems, I tested all early probe exit points (and
> debugged the resulting panics). The end result is a simple looking patch
> set of changes, with minimum reshuffling of code. In my testing with this
> patch, I have not encountered any NULL pointer crashes or double frees.
>
> Fixes: d64d6c5671db ("scsi: qla2xxx: Fix NULL pointer crash due to probe
> failure")
> Signed-off-by: Bill Kuzeja
>
> ---
>
> Some of these issues are due to:
>
> commit d64d6c5671db scsi: qla2xxx: Fix NULL pointer crash due to probe failure
>
> where some frees were moved around, as well as the error exit from
> a qla2x00_request_irqs failure.
>
> This was a fix for:
>
> commit d74595278f4a scsi: qla2xxx: Add multiple queue pair functionality.
>
> which caused problems of its own.
>
> To reproduce these issues, I run a test where I break the card early in init,
> (and also through kprobe fault injection). This way, I've been able to hit
> four different types of crashes.
>
> Here's the leadup to the first issue:
>
> [ 1044.133225] mpt3sas_cm4: diag reset: FAILED
> [ 1072.694857] qla2xxx [:b7:00.0]-d04c:12: MBX Command timeout for cmd 2,
> _status 0x hccr 0x
> [ 1072.734020] qla2xxx [:b7:00.0]-00cf:12: Setup chip FAILED.
> [ 1072.748506] qla2xxx [:b7:00.0]-00d6:12: Failed to initialize adapter -
> Adapter flags 42.
>
> and we crash
>
> [ 1072.787691] kernel BUG at mm/slub.c:3601!
>
> Here's the stack trace from the dump:
>
> PID: 20037 TASK: a1447ab24f10 CPU: 0 COMMAND: "kworker/0:14"
> #0 [a144c6ba7850] die at 8aa2e92b
> #1 [a144c6ba7880] do_trap at 8b10e1f0
> #2 [a144c6ba78d0] do_invalid_op at 8aa2b284
> #3 [a144c6ba7980] invalid_op at 8b11b4ee
> [exception RIP: kfree+313]
> RIP: 8abf3f79 RSP: a144c6ba7a30 RFLAGS: 00010246
> RAX: 001f RBX: a13cdbe24000 RCX: 00010040003c
> RDX: 001f RSI: fd615cf8a6c0 RDI: a13cdbe24000
> RBP: a144c6ba7a48 R8: a143be29b1c0 R9: 00010040003c
> R10: be29bc01 R11: fd61416f8900 R12: a14c763bf000
> R13: c08df36d R14: a13cdbe20740 R15: a13cdbe2
> ORIG_RAX: CS: 0010 SS:
> #4 [a144c6ba7a50] qla2x00_probe_one at c08df36d [qla2xxx]
> #5 [a144c6ba7dd0] local_pci_probe at 8ad92e45
> #6 [a144c6ba7e08] work_for_cpu_fn at 8aaae4c4
> #7 [a144c6ba7e20] process_one_work at 8aab170a
> #8 [a144c6ba7e68] worker_thread at 8aab2528
> #9 [a144c6ba7ec8] kthread at 8aab96af
>
> Here's we're trying to re-free ha->nvram in qla2x00_mem_free. When the
> failure in ha->isp_ops->initialize_adapter occurs, we jump to the
> probe_failed label. Then we end up calling qla2x00_mem_free twice because
> qla2x00_free_device calls qla2x00_mem_free as well. This is problem #1.
>
> Problem #2: If we fail even earlier - in qla2x00_mem_alloc - we jump to
> probe_hw_failed which now tries to free memory that wasn't allocated
> (note before commit d64d6c5671db, we only called qla2x00_clear_drv_active -
> this was correct). My server crashed on qla2x00_free_req_que.
>
> Next, notice that the only exit point that calls probe_init_failed is
> when qla2x00_alloc_queues fails. But looking inside qla2x00_alloc_queues,
> we only do the following if this routine is successful:
>
> ha->rsp_q_map[0] = rsp;
> ha->req_q_map[0] = req;
> set_bit(0, ha->rsp_qid_map);
> set_bit(0, ha->req_qid_map);
>
> Otherwise, we free up ha->base_qpair, ha->rsp_q_map, ha->req_q_map
> and exit with an error. Then we jump to probe_init_failed, which does
> two things that are bad.
>
> First, there's (problem #3):
>
>ha->req_q_map[0] = NULL;
>ha->rsp_q_map[0] = NULL;
>
> In the failure case of qla2x00_alloc_queues, ha->req_q_map and
> ha->rsp_q_map are set to NULL. Can't dereference a NULL pointer and
> that is what we crash on here.
>
> If the system didn't panic on this, it would have on problem #4.
> There are extra calls to qla2x00_free_req_que and qla2x00_free_rsp_que
> in probe_init_failed path because these calls are now duplicated in
> probe_hw_failed because of commit d64d6c5671db.
>
> Now for the changes...
>
> Problem #1 is easily solved. When we enter probe_failed, have a new
> variable that indicates this is the case so probe_hw_failed can check
> it and not call the same free routines again (qla2x00_free_device calls
> qla2x00_mem_free, qla2x00_free_req_que, and qla2x00_free_rsp_que).
>
> For problem 2, an extra label
[PATCH] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure
Because of the shifting around of code in qla2x00_probe_one recently,
failures during adapter initialization can lead to problems, i.e. NULL
pointer crashes and doubly freed data structures which cause eventual
panics.
To address these problems, I tested all early probe exit points (and
debugged the resulting panics). The end result is a simple looking patch
set of changes, with minimum reshuffling of code. In my testing with this
patch, I have not encountered any NULL pointer crashes or double frees.
Fixes: d64d6c5671db ("scsi: qla2xxx: Fix NULL pointer crash due to probe
failure")
Signed-off-by: Bill Kuzeja
---
Some of these issues are due to:
commit d64d6c5671db scsi: qla2xxx: Fix NULL pointer crash due to probe failure
where some frees were moved around, as well as the error exit from
a qla2x00_request_irqs failure.
This was a fix for:
commit d74595278f4a scsi: qla2xxx: Add multiple queue pair functionality.
which caused problems of its own.
To reproduce these issues, I run a test where I break the card early in init,
(and also through kprobe fault injection). This way, I've been able to hit
four different types of crashes.
Here's the leadup to the first issue:
[ 1044.133225] mpt3sas_cm4: diag reset: FAILED
[ 1072.694857] qla2xxx [:b7:00.0]-d04c:12: MBX Command timeout for cmd 2,
_status 0x hccr 0x
[ 1072.734020] qla2xxx [:b7:00.0]-00cf:12: Setup chip FAILED.
[ 1072.748506] qla2xxx [:b7:00.0]-00d6:12: Failed to initialize adapter -
Adapter flags 42.
and we crash
[ 1072.787691] kernel BUG at mm/slub.c:3601!
Here's the stack trace from the dump:
PID: 20037 TASK: a1447ab24f10 CPU: 0 COMMAND: "kworker/0:14"
#0 [a144c6ba7850] die at 8aa2e92b
#1 [a144c6ba7880] do_trap at 8b10e1f0
#2 [a144c6ba78d0] do_invalid_op at 8aa2b284
#3 [a144c6ba7980] invalid_op at 8b11b4ee
[exception RIP: kfree+313]
RIP: 8abf3f79 RSP: a144c6ba7a30 RFLAGS: 00010246
RAX: 001f RBX: a13cdbe24000 RCX: 00010040003c
RDX: 001f RSI: fd615cf8a6c0 RDI: a13cdbe24000
RBP: a144c6ba7a48 R8: a143be29b1c0 R9: 00010040003c
R10: be29bc01 R11: fd61416f8900 R12: a14c763bf000
R13: c08df36d R14: a13cdbe20740 R15: a13cdbe2
ORIG_RAX: CS: 0010 SS:
#4 [a144c6ba7a50] qla2x00_probe_one at c08df36d [qla2xxx]
#5 [a144c6ba7dd0] local_pci_probe at 8ad92e45
#6 [a144c6ba7e08] work_for_cpu_fn at 8aaae4c4
#7 [a144c6ba7e20] process_one_work at 8aab170a
#8 [a144c6ba7e68] worker_thread at 8aab2528
#9 [a144c6ba7ec8] kthread at 8aab96af
Here's we're trying to re-free ha->nvram in qla2x00_mem_free. When the
failure in ha->isp_ops->initialize_adapter occurs, we jump to the
probe_failed label. Then we end up calling qla2x00_mem_free twice because
qla2x00_free_device calls qla2x00_mem_free as well. This is problem #1.
Problem #2: If we fail even earlier - in qla2x00_mem_alloc - we jump to
probe_hw_failed which now tries to free memory that wasn't allocated
(note before commit d64d6c5671db, we only called qla2x00_clear_drv_active -
this was correct). My server crashed on qla2x00_free_req_que.
Next, notice that the only exit point that calls probe_init_failed is
when qla2x00_alloc_queues fails. But looking inside qla2x00_alloc_queues,
we only do the following if this routine is successful:
ha->rsp_q_map[0] = rsp;
ha->req_q_map[0] = req;
set_bit(0, ha->rsp_qid_map);
set_bit(0, ha->req_qid_map);
Otherwise, we free up ha->base_qpair, ha->rsp_q_map, ha->req_q_map
and exit with an error. Then we jump to probe_init_failed, which does
two things that are bad.
First, there's (problem #3):
ha->req_q_map[0] = NULL;
ha->rsp_q_map[0] = NULL;
In the failure case of qla2x00_alloc_queues, ha->req_q_map and
ha->rsp_q_map are set to NULL. Can't dereference a NULL pointer and
that is what we crash on here.
If the system didn't panic on this, it would have on problem #4.
There are extra calls to qla2x00_free_req_que and qla2x00_free_rsp_que
in probe_init_failed path because these calls are now duplicated in
probe_hw_failed because of commit d64d6c5671db.
Now for the changes...
Problem #1 is easily solved. When we enter probe_failed, have a new
variable that indicates this is the case so probe_hw_failed can check
it and not call the same free routines again (qla2x00_free_device calls
qla2x00_mem_free, qla2x00_free_req_que, and qla2x00_free_rsp_que).
For problem 2, an extra label (probe_hw_failed_no_mem) will be created
at the end of probe_hw_failed, so we don't try to free unallocated memory.
Problems 3 and 4 are also easy to remedy: if qla2x00_alloc_queues fails,
we should call probe_failed instead. And since nobody jumps to
probe_init_failed anymore,
