Hi
On 18.10.2016 16:07, Felipe Balbi wrote:
Hi,
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?"
drivers/usb/host/xhci-ring.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff -puN drivers/usb/host/xhci-ring.c~xhci-fix-empty-list
drivers/usb/host/xhci-ring.c
--- linux/drivers/usb/host/xhci-ring.c~xhci-fix-empty-list 2016-10-18
21:20:07.973055018 +0900
+++ linux-hirofumi/drivers/usb/host/xhci-ring.c 2016-10-18 21:21:35.949060365
+0900
@@ -1336,7 +1336,12 @@ static void handle_cmd_completion(struct
return;
}
- cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);
+ 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.
@@ -1350,9 +1355,10 @@ static void handle_cmd_completion(struct
Before the lines below, there's a call to del_timer() and a
tracepoint. I have a feeling we don't want those to happen if the list
is corrupt. Perhaps you should skip del_timer() and adding a log to
trace buffer, no?
I think those are fine, they shouldn't matter. With a empty command list
we don't wan't any command timeout timer to trigger, and the trancepoint
prints out info about the command trb on the command ring and event
trb on event ring. It doesn't touch the struct command in the command list.
-Mathias
--
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
