Mathias Nyman <mathias.ny...@intel.com> writes:
>> OGAWA Hirofumi <hirof...@mail.parknet.co.jp> writes:
>>> If race with timeout or stop event on empty cmd_list was happened,
>>> handle_cmd_completion() deferences wrong pointer from xhci->cmd_list.
>>>
>>> So this makes sure we don't dereference wrong pointer from empty
>>> xhci->cmd_list.
>>>
>>> Signed-off-by: OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
>>> ---
>
> Thanks, nice catch.
>
> What kernel is this based on, There is a partial fix in 4.8:
>
> commit 33be126510974e2eb9679f1ca9bca4f67ee4c4c7
> xhci: always handle "Command Ring Stopped" events
>
> Looking at the existing code the case you describe could be possible if
> handle_cmd_completion is racing with:
> - command timed out twice and xhci_cleanup_command_queue() is called
> - or command timed out and we fail to restart command ring (assume host is
> dead)
>
> It would be interesting to know how this was triggered, were there any error
> messages like:
> "Abort command ring failed"
> or with xhci debugging enabled something like:
> "command timed out twice, ring start fail?"
Ah, sorry for confusing. I wrote this for older kernel (3.8.x, and not
for vanilla). So 4.8 might not have race what I saw actually.
(33be126510974e2eb9679f1ca9bca4f67ee4c4c7 seems to be prevent it)
However, even if 4.8 fixed that case, if there is spurious event or
driver state confusing, fetch and dereference empty list is possible and
bad thing. So, IMO, it would be better to make sure to avoid it.
>>> + if (list_empty(&xhci->cmd_list))
>>> + cmd = NULL;
>>> + else {
>>> + cmd = list_first_entry(&xhci->cmd_list, struct xhci_command,
>>> + cmd_list);
>>> + }
>>
>> These few lines could become a nice helper. Something like:
>>
>> static inline struct xhci_command *next_command(struct xhci_hcd *xhci)
>> {
>> if (list_empty(&xhci->cmd_list))
>> return NULL;
>> return list_first_entry(&xhci->cmd_list, struct xhci_command, cmd_list);
>> }
>>
>
> Both works for me, helper function, or code in this patch.
There are some opencode list_head usages too. It might be better to
change at once as list cleanup if need.
E.g.
/* restart timer if this wasn't the last command */
if (cmd->cmd_list.next != &xhci->cmd_list) {
is
if (!list_is_last(&cmd->cmd_list, &xhci->cmd_list)) {
Well, so, if next_command() cleanup is not required, I'd like to use as
is, in this patch.
Thanks.
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
