| 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: <CAMz4ku+c1k47Or_sYPeQEwX3LJoynepcRSEtfosAzoyjrS+2SQ@mail.gmail.com>
Date: Mon, 19 Dec 2016 19:34:28 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: mathias.nyman@...el.com, Greg KH <gregkh@...uxfoundation.org>,
Lu Baolu <baolu.lu@...ux.intel.com>,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>,
"Lu, Baolu" <baolu.lu@...el.com>
Subject: Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi Mathias,
On 19 December 2016 at 18:33, Mathias Nyman
<mathias.nyman@...ux.intel.com> wrote:
> On 13.12.2016 05:21, Baolin Wang wrote:
>>
>> Hi Mathias,
>>
>> On 12 December 2016 at 23:52, Mathias Nyman
>> <mathias.nyman@...ux.intel.com> wrote:
>>>
>>> On 05.12.2016 09:51, Baolin Wang 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.
>>>
>>>
>>>
>>> Ah, right, this could actually happen.
>>>
>>>>
>>>>
>>>> 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.
>>>
>>>
>>>
>>> A counter like this could work.
>>>
>>> Writing the abort bit can generate either ABORT+STOP, or just STOP
>>> event, this seems to cover both.
>>>
>>> quick check, case 1: timeout and cmd completion at the same time.
>>>
>>> cpu1 cpu2
>>>
>>> queue_command(first), p++ (=1)
>>> queue_command(more),
>>> --completion irq fires-- -- timer times out at same time--
>>> handle_cmd_completion() handle_cmd_timeout(),)
>>> lock(xhci_lock ) spin_on(xhci_lock)
>>> del_timer() fail, p (=1, nochange)
>>> cur_cmd = list_next(), p++ (=2)
>>> unlock(xhci_lock)
>>> lock(xhci_lock)
>>> p-- (=1)
>>> if (p > 0), exit
>>> OK works
>>>
>>> case 2: normal timeout case with ABORT+STOP, no race.
>>>
>>> cpu1 cpu2
>>>
>>> queue_command(first), p++ (=1)
>>> queue_command(more),
>>> handle_cmd_timeout()
>>> p-- (P=0), don't exit
>>> mod_timer(), p++ (P=1)
>>> write_abort_bit()
>>> handle_cmd_comletion(ABORT)
>>> del_timer(), ok, p-- (p = 0)
>>> handle_cmd_completion(STOP)
>>> del_timer(), fail, (P=0)
>>> handle_stopped_cmd_ring()
>>> cur_cmd = list_next(), p++ (=1)
>>> mod_timer()
>>>
>>> OK, works, and same for just STOP case, with the only difference that
>>> during handle_cmd_completion(STOP) p is decremented (p--)
>>
>>
>> Yes, that's the cases what I want to handle, thanks for your explicit
>> explanation.
>>
>
> Gave this some more thought over the weekend, and this implementation
> doesn't solve the case when the last command times out and races with the
> completion handler:
>
> cpu1 cpu2
>
> queue_command(first), p++ (=1)
> --completion irq fires-- -- timer times out at same time--
> handle_cmd_completion() handle_cmd_timeout(),)
> lock(xhci_lock ) spin_on(xhci_lock)
> del_timer() fail, p (=1, nochange)
> no more commands, P (=1, nochange)
> unlock(xhci_lock)
> lock(xhci_lock)
> p-- (=0)
> p == 0, continue, even if we should
> not.
> For this we still need to rely on
> checking cur_cmd == NULL in the timeout function.
> (Baolus patch sets it to NULL if there are no more commands pending)
As I pointed out in patch 1 of this patchset, this patchset is based
on Lu Baolu's new fix patch:
usb: xhci: fix possible wild pointer
https://www.spinics.net/lists/linux-usb/msg150219.html
After applying Baolu's patch, after decrement the counter, we will
check the xhci->cur_command if is NULL. So in this situation:
cpu1 cpu2
queue_command(first), p++ (=1)
--completion irq fires-- -- timer times out at same time--
handle_cmd_completion() handle_cmd_timeout(),)
lock(xhci_lock ) spin_on(xhci_lock)
del_timer() fail, p (=1, nochange)
no more commands, P (=1, nochange)
unlock(xhci_lock)
lock(xhci_lock)
p-- (=0)
no current command, return
if (!xhci->current_cmd) {
unlock(xhci_lock);
return;
}
It can work.
>
> And then we could replace the whole counter with a simple check if the
> timeout timer
> is pending in the timeout function:
>
> xhci_handle_command_timeout()
> lock()
> if (!cur_cmd || timer_pending(timeout_timer)) {
> unlock();
> return;
> }
>
> -Mathias
>
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists