* [RFC] tcmu: wrong input from userspace can confuse tcmu
@ 2020-04-30 15:10 Bodo Stroesser
2020-04-30 15:59 ` Mike Christie
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Bodo Stroesser @ 2020-04-30 15:10 UTC (permalink / raw)
To: target-devel
Hi,
When tcmu queues a new command - no matter whether in command
ring or in qfull_queue - a cmd_id from IDR udev->commands is
assigned to the command.
If userspaces sends a wrong command completion containing the
cmd_id of a command on the qfull_queue, tcmu_handle_completions()
finds the command in the IDR and calls tcmu_handle_completion()
for it. This might do some nasty things, because commands in
qfull_queue do not have a valid dbi list.
Of course this is easy to fix. E.g.:
--- a/drivers/target/target_core_user.c 2020-04-30 14:15:36.177184668 +0200
+++ b/drivers/target/target_core_user.c 2020-04-30 14:29:43.331882066 +0200
@@ -1242,13 +1242,14 @@ static unsigned int tcmu_handle_completi
}
WARN_ON(tcmu_hdr_get_op(entry->hdr.len_op) != TCMU_OP_CMD);
- cmd = idr_remove(&udev->commands, entry->hdr.cmd_id);
- if (!cmd) {
+ cmd = idr_find(&udev->commands, entry->hdr.cmd_id);
+ if (!cmd || !test_bit(TCMU_CMD_BIT_INFLIGHT, &cmd->flags)) {
pr_err("cmd_id %u not found, ring is broken\n",
entry->hdr.cmd_id);
set_bit(TCMU_DEV_BIT_BROKEN, &udev->flags);
break;
}
+ idr_remove(&udev->commands, entry->hdr.cmd_id);
tcmu_handle_completion(cmd, entry);
To avoid the second idr_*() call in main data path, it would also
be possible to not replace the idr_remove() by idr_find(), but
in case cmd was found without TCMU_CMD_BIT_INFLIGHT being set,
during error handling re-add the cmd to the idr with the same id
as before by calling idr_alloc(...,cmd_id, crmdd_id+1, ...).
OTOH, I'm wondering whether the better solution would be to
remove idr_alloc() from tcmu_setup_cmd_timer() and add it
to queue_cmd_ring() immediately before or after it calls
tcmu_setup_cmd_timer().
Doing so would mean that commands in qfull_queue no longer have
a cmd_id (which is not necessary AFAICS) and therefore the problem
cannot happen, because tcmu_handle_completions() cannot find them
in the IDR.
Of course, this change would cause a number of further small
changes in target_core_user.c, but the effort is not too high,
it seems.
What do you think is the best solution?
Thank you,
Bodo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] tcmu: wrong input from userspace can confuse tcmu
2020-04-30 15:10 [RFC] tcmu: wrong input from userspace can confuse tcmu Bodo Stroesser
@ 2020-04-30 15:59 ` Mike Christie
2020-04-30 16:40 ` Bodo Stroesser
2020-04-30 17:29 ` Mike Christie
2 siblings, 0 replies; 4+ messages in thread
From: Mike Christie @ 2020-04-30 15:59 UTC (permalink / raw)
To: target-devel
On 4/30/20 10:10 AM, Bodo Stroesser wrote:
> Hi,
>
> When tcmu queues a new command - no matter whether in command
> ring or in qfull_queue - a cmd_id from IDR udev->commands is
> assigned to the command.
>
> If userspaces sends a wrong command completion containing the
> cmd_id of a command on the qfull_queue, tcmu_handle_completions()
> finds the command in the IDR and calls tcmu_handle_completion()
> for it. This might do some nasty things, because commands in
> qfull_queue do not have a valid dbi list.
>
> Of course this is easy to fix. E.g.:
>
>
> --- a/drivers/target/target_core_user.c 2020-04-30 14:15:36.177184668 +0200
> +++ b/drivers/target/target_core_user.c 2020-04-30 14:29:43.331882066 +0200
> @@ -1242,13 +1242,14 @@ static unsigned int tcmu_handle_completi
> }
> WARN_ON(tcmu_hdr_get_op(entry->hdr.len_op) != TCMU_OP_CMD);
>
> - cmd = idr_remove(&udev->commands, entry->hdr.cmd_id);
> - if (!cmd) {
> + cmd = idr_find(&udev->commands, entry->hdr.cmd_id);
> + if (!cmd || !test_bit(TCMU_CMD_BIT_INFLIGHT, &cmd->flags)) {
> pr_err("cmd_id %u not found, ring is broken\n",
> entry->hdr.cmd_id);
> set_bit(TCMU_DEV_BIT_BROKEN, &udev->flags);
> break;
> }
> + idr_remove(&udev->commands, entry->hdr.cmd_id);
>
> tcmu_handle_completion(cmd, entry);
>
>
> To avoid the second idr_*() call in main data path, it would also
> be possible to not replace the idr_remove() by idr_find(), but
> in case cmd was found without TCMU_CMD_BIT_INFLIGHT being set,
> during error handling re-add the cmd to the idr with the same id
> as before by calling idr_alloc(...,cmd_id, crmdd_id+1, ...).
>
>
> OTOH, I'm wondering whether the better solution would be to
> remove idr_alloc() from tcmu_setup_cmd_timer() and add it
> to queue_cmd_ring() immediately before or after it calls
> tcmu_setup_cmd_timer().
> Doing so would mean that commands in qfull_queue no longer have
> a cmd_id (which is not necessary AFAICS) and therefore the problem
It's done for logging, but its only debug logging so we could just print
the pointer value.
> cannot happen, because tcmu_handle_completions() cannot find them
> in the IDR.
>
This is probably best if it's not a lot of trouble.
If you go this route then in tcmu_reset_ring I think I made a mistake
and we just continue the loop if TCMU_CMD_BIT_INFLIGHT is not set. I
think we want to do:
for cmd in qfull_list
target_complete_cmd()
for cmd in idr
target_complete_cmd()
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] tcmu: wrong input from userspace can confuse tcmu
2020-04-30 15:10 [RFC] tcmu: wrong input from userspace can confuse tcmu Bodo Stroesser
2020-04-30 15:59 ` Mike Christie
@ 2020-04-30 16:40 ` Bodo Stroesser
2020-04-30 17:29 ` Mike Christie
2 siblings, 0 replies; 4+ messages in thread
From: Bodo Stroesser @ 2020-04-30 16:40 UTC (permalink / raw)
To: target-devel
On 04/30/20 17:59, Mike Christie wrote:
> On 4/30/20 10:10 AM, Bodo Stroesser wrote:
>> Hi,
>>
>> When tcmu queues a new command - no matter whether in command
>> ring or in qfull_queue - a cmd_id from IDR udev->commands is
>> assigned to the command.
>>
>> If userspaces sends a wrong command completion containing the
>> cmd_id of a command on the qfull_queue, tcmu_handle_completions()
>> finds the command in the IDR and calls tcmu_handle_completion()
>> for it. This might do some nasty things, because commands in
>> qfull_queue do not have a valid dbi list.
>>
>> Of course this is easy to fix. E.g.:
>>
>>
>> --- a/drivers/target/target_core_user.c 2020-04-30 14:15:36.177184668 +0200
>> +++ b/drivers/target/target_core_user.c 2020-04-30 14:29:43.331882066 +0200
>> @@ -1242,13 +1242,14 @@ static unsigned int tcmu_handle_completi
>> }
>> WARN_ON(tcmu_hdr_get_op(entry->hdr.len_op) != TCMU_OP_CMD);
>>
>> - cmd = idr_remove(&udev->commands, entry->hdr.cmd_id);
>> - if (!cmd) {
>> + cmd = idr_find(&udev->commands, entry->hdr.cmd_id);
>> + if (!cmd || !test_bit(TCMU_CMD_BIT_INFLIGHT, &cmd->flags)) {
>> pr_err("cmd_id %u not found, ring is broken\n",
>> entry->hdr.cmd_id);
>> set_bit(TCMU_DEV_BIT_BROKEN, &udev->flags);
>> break;
>> }
>> + idr_remove(&udev->commands, entry->hdr.cmd_id);
>>
>> tcmu_handle_completion(cmd, entry);
>>
>>
>> To avoid the second idr_*() call in main data path, it would also
>> be possible to not replace the idr_remove() by idr_find(), but
>> in case cmd was found without TCMU_CMD_BIT_INFLIGHT being set,
>> during error handling re-add the cmd to the idr with the same id
>> as before by calling idr_alloc(...,cmd_id, crmdd_id+1, ...).
>>
>>
>> OTOH, I'm wondering whether the better solution would be to
>> remove idr_alloc() from tcmu_setup_cmd_timer() and add it
>> to queue_cmd_ring() immediately before or after it calls
>> tcmu_setup_cmd_timer().
>> Doing so would mean that commands in qfull_queue no longer have
>> a cmd_id (which is not necessary AFAICS) and therefore the problem
>
> It's done for logging, but its only debug logging so we could just print
> the pointer value.
>
Yes, and then add the pointer value to the pr_debug that prints the
assigned cmd_id after idr_alloc()
>> cannot happen, because tcmu_handle_completions() cannot find them
>> in the IDR.
>>
>
> This is probably best if it's not a lot of trouble.
I already spent some time to check the necessary changes. I think
there is no trouble, just some small changes.
So, yes, the bigger fix is probably better.
I'll wait till next week for further comments and then send a patch.
>
> If you go this route then in tcmu_reset_ring I think I made a mistake
I'm not sure, but I don't think there is a mistake. If you cleanup
the ring, you don't need to cancel the commands in qfull_queue.
> and we just continue the loop if TCMU_CMD_BIT_INFLIGHT is not set. I
> think we want to do:
>
> for cmd in qfull_list
> target_complete_cmd()
So I think, this is not really necessary, but after
clear_bit(TCMU_DEV_BIT_BROKEN, &udev->flags);
we could do
run_qfull_queue(udev, false);
right?
>
> for cmd in idr
> target_complete_cmd()
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] tcmu: wrong input from userspace can confuse tcmu
2020-04-30 15:10 [RFC] tcmu: wrong input from userspace can confuse tcmu Bodo Stroesser
2020-04-30 15:59 ` Mike Christie
2020-04-30 16:40 ` Bodo Stroesser
@ 2020-04-30 17:29 ` Mike Christie
2 siblings, 0 replies; 4+ messages in thread
From: Mike Christie @ 2020-04-30 17:29 UTC (permalink / raw)
To: target-devel
On 4/30/20 11:40 AM, Bodo Stroesser wrote:
>
>
> On 04/30/20 17:59, Mike Christie wrote:
>> On 4/30/20 10:10 AM, Bodo Stroesser wrote:
>>> Hi,
>>>
>>> When tcmu queues a new command - no matter whether in command
>>> ring or in qfull_queue - a cmd_id from IDR udev->commands is
>>> assigned to the command.
>>>
>>> If userspaces sends a wrong command completion containing the
>>> cmd_id of a command on the qfull_queue, tcmu_handle_completions()
>>> finds the command in the IDR and calls tcmu_handle_completion()
>>> for it. This might do some nasty things, because commands in
>>> qfull_queue do not have a valid dbi list.
>>>
>>> Of course this is easy to fix. E.g.:
>>>
>>>
>>> --- a/drivers/target/target_core_user.c 2020-04-30
>>> 14:15:36.177184668 +0200
>>> +++ b/drivers/target/target_core_user.c 2020-04-30
>>> 14:29:43.331882066 +0200
>>> @@ -1242,13 +1242,14 @@ static unsigned int tcmu_handle_completi
>>> }
>>> WARN_ON(tcmu_hdr_get_op(entry->hdr.len_op) != TCMU_OP_CMD);
>>> - cmd = idr_remove(&udev->commands, entry->hdr.cmd_id);
>>> - if (!cmd) {
>>> + cmd = idr_find(&udev->commands, entry->hdr.cmd_id);
>>> + if (!cmd || !test_bit(TCMU_CMD_BIT_INFLIGHT, &cmd->flags)) {
>>> pr_err("cmd_id %u not found, ring is broken\n",
>>> entry->hdr.cmd_id);
>>> set_bit(TCMU_DEV_BIT_BROKEN, &udev->flags);
>>> break;
>>> }
>>> + idr_remove(&udev->commands, entry->hdr.cmd_id);
>>> tcmu_handle_completion(cmd, entry);
>>>
>>> To avoid the second idr_*() call in main data path, it would also
>>> be possible to not replace the idr_remove() by idr_find(), but
>>> in case cmd was found without TCMU_CMD_BIT_INFLIGHT being set,
>>> during error handling re-add the cmd to the idr with the same id
>>> as before by calling idr_alloc(...,cmd_id, crmdd_id+1, ...).
>>>
>>>
>>> OTOH, I'm wondering whether the better solution would be to
>>> remove idr_alloc() from tcmu_setup_cmd_timer() and add it
>>> to queue_cmd_ring() immediately before or after it calls
>>> tcmu_setup_cmd_timer().
>>> Doing so would mean that commands in qfull_queue no longer have
>>> a cmd_id (which is not necessary AFAICS) and therefore the problem
>>
>> It's done for logging, but its only debug logging so we could just print
>> the pointer value.
>>
>
> Yes, and then add the pointer value to the pr_debug that prints the
> assigned cmd_id after idr_alloc()
>
>>> cannot happen, because tcmu_handle_completions() cannot find them
>>> in the IDR.
>>>
>>
>> This is probably best if it's not a lot of trouble.
>
> I already spent some time to check the necessary changes. I think
> there is no trouble, just some small changes.
> So, yes, the bigger fix is probably better.
>
> I'll wait till next week for further comments and then send a patch.
>
>>
>> If you go this route then in tcmu_reset_ring I think I made a mistake
>
> I'm not sure, but I don't think there is a mistake. If you cleanup
> the ring, you don't need to cancel the commands in qfull_queue.
I was thinking about your userspace restart PGR question in another
mail. Between the time userspace stopped and restarted the session->id
to I_T nexus mapping could have changed, so id now refers to a different
I_T nexus.
Code path wise, it would be easier if we restarted the inflight and
non-inflight commands the same way.
>
>> and we just continue the loop if TCMU_CMD_BIT_INFLIGHT is not set. I
>> think we want to do:
>>
>> for cmd in qfull_list
>> target_complete_cmd()
> So I think, this is not really necessary, but after
> clear_bit(TCMU_DEV_BIT_BROKEN, &udev->flags);
>
> we could do
> run_qfull_queue(udev, false);
>
> right?
>
>>
>> for cmd in idr
>> target_complete_cmd()
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-30 17:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-30 15:10 [RFC] tcmu: wrong input from userspace can confuse tcmu Bodo Stroesser
2020-04-30 15:59 ` Mike Christie
2020-04-30 16:40 ` Bodo Stroesser
2020-04-30 17:29 ` Mike Christie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.