| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMz4kuKu1Fr1rnfp=UesimDtkA=Gmx_rNr=Dm7NKVdQFjGZhww@mail.gmail.com>
Date: Mon, 5 Dec 2016 10:59:39 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: mathias.nyman@...el.com, Greg KH <gregkh@...uxfoundation.org>
Cc: USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>,
Baolin Wang <baolin.wang@...aro.org>
Subject: Re: [PATCH] usb: host: xhci: Handle the right timeout command
On 2 December 2016 at 19:21, Baolin Wang <baolin.wang@...aro.org> wrote:
> If a command event is found on the event ring during an interrupt,
> we need to stop the command timer with del_timer(). Since del_timer()
> can fail if the timer is running and waiting on the xHCI lock, then
> it maybe get the wrong timeout command in xhci_handle_command_timeout()
> if host fetched a new command and updated the xhci->current_cmd in
> handle_cmd_completion(). For this situation, we need a way to signal
> to the command timer that everything is fine and it should exit.
>
> We should introduce a counter (xhci->current_cmd_pending) for the number
> of pending commands. If we need to cancel the command timer and del_timer()
> succeeds, we decrement the number of pending commands. If del_timer() fails,
> we leave the number of pending commands alone.
>
> For handling timeout command, in xhci_handle_command_timeout() we will check
> the counter after decrementing it, if the counter (xhci->current_cmd_pending)
> is 0, which means xhci->current_cmd is the right timeout command. If the
> counter (xhci->current_cmd_pending) is greater than 0, which means current
> timeout command has been handled by host and host has fetched new command
> as xhci->current_cmd, then just return and wait for new current command.
>
> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
> ---
> This patch is based on Lu Baolu's new fix patch:
> usb: xhci: fix possible wild pointer
> ---
> drivers/usb/host/xhci-ring.c | 26 +++++++++++++++++++++++++-
> drivers/usb/host/xhci.h | 1 +
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 62dd1c7..a62904e 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1253,6 +1253,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
> if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
> !(xhci->xhc_state & XHCI_STATE_DYING)) {
> xhci->current_cmd = cur_cmd;
> + xhci->current_cmd_pending++;
> mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
> xhci_ring_cmd_db(xhci);
> }
> @@ -1274,6 +1275,21 @@ void xhci_handle_command_timeout(unsigned long data)
> return;
> }
>
> + xhci->current_cmd_pending--;
I realized I should move the decrement before 'xhci->current_cmd'
checking, and I will wait for a while for new comments before sending
out v2 patch.
> + /*
> + * If the current_cmd_pending is 0, which means current command is
> + * timeout.
> + *
> + * If the current_cmd_pending is greater than 0, which means current
> + * timeout command has been handled by host and host has fetched new
> + * command as xhci->current_cmd, then just return and wait for new
> + * current command.
> + */
> + if (xhci->current_cmd_pending > 0) {
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + return;
> + }
> +
> if (xhci->current_cmd->status == COMP_CMD_ABORT)
> second_timeout = true;
> xhci->current_cmd->status = COMP_CMD_ABORT;
> @@ -1336,7 +1352,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
>
> cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);
>
> - del_timer(&xhci->cmd_timer);
> + /*
> + * If the command timer is running on another CPU, we don't decrement
> + * current_cmd_pending, since we didn't successfully stop the command
> + * timer.
> + */
> + if (del_timer(&xhci->cmd_timer))
> + xhci->current_cmd_pending--;
>
> trace_xhci_cmd_completion(cmd_trb, (struct xhci_generic_trb *) event);
>
> @@ -1424,6 +1446,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
> if (cmd->cmd_list.next != &xhci->cmd_list) {
> xhci->current_cmd = list_entry(cmd->cmd_list.next,
> struct xhci_command, cmd_list);
> + xhci->current_cmd_pending++;
> mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
> } else if (xhci->current_cmd == cmd) {
> xhci->current_cmd = NULL;
> @@ -3924,6 +3947,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
> if (xhci->cmd_list.next == &cmd->cmd_list &&
> !timer_pending(&xhci->cmd_timer)) {
> xhci->current_cmd = cmd;
> + xhci->current_cmd_pending++;
> mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
> }
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 9dbaacf..5d81257 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1567,6 +1567,7 @@ struct xhci_hcd {
> unsigned int cmd_ring_reserved_trbs;
> struct timer_list cmd_timer;
> struct xhci_command *current_cmd;
> + u32 current_cmd_pending;
> struct xhci_ring *event_ring;
> struct xhci_erst erst;
> /* Scratchpad */
> --
> 1.7.9.5
>
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists