From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Torokhov Subject: Re: Potential data race in psmouse_interrupt Date: Fri, 28 Aug 2015 11:34:03 -0700 Message-ID: References: <20150723130844.GA29125@pali> <20150729115326.GE13518@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-oi0-f54.google.com ([209.85.218.54]:33308 "EHLO mail-oi0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752051AbbH1SeE (ORCPT ); Fri, 28 Aug 2015 14:34:04 -0400 In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Dmitry Vyukov Cc: =?UTF-8?Q?Pali_Roh=C3=A1r?= , Andrey Konovalov , Hans de Goede , Mathias Gottschlag , Shailendra Verma , Rusty Russell , "Luis R. Rodriguez" , Thomas Hellstrom , "linux-input@vger.kernel.org" , LKML , Kostya Serebryany , Alexander Potapenko , ktsan@googlegroups.com On Fri, Aug 28, 2015 at 11:32 AM, Dmitry Torokhov wrote: > On Fri, Aug 28, 2015 at 11:08 AM, Dmitry Vyukov wrote: >> On Fri, Aug 28, 2015 at 7:51 PM, Dmitry Torokhov >> wrote: >>> On Fri, Aug 28, 2015 at 10:34 AM, Dmitry Vyukov wrote: >>>> Hello, >>>> >>>> I am looking at this code in __ps2_command again: >>>> >>>> /* >>>> * The reset command takes a long time to execute. >>>> */ >>>> timeout = msecs_to_jiffies(command == PS2_CMD_RESET_BAT ? 4000 : 500); >>>> >>>> timeout = wait_event_timeout(ps2dev->wait, >>>> !(READ_ONCE(ps2dev->flags) & PS2_FLAG_CMD1), timeout); >>>> >>>> if (smp_load_acquire(&ps2dev->cmdcnt) && >>>> !(smp_load_acquire(&ps2dev->flags) & PS2_FLAG_CMD1)) { >>>> timeout = ps2_adjust_timeout(ps2dev, command, timeout); >>>> wait_event_timeout(ps2dev->wait, >>>> !(smp_load_acquire(&ps2dev->flags) & >>>> PS2_FLAG_CMD), timeout); >>>> } >>>> >>>> if (param) >>>> for (i = 0; i < receive; i++) >>>> param[i] = ps2dev->cmdbuf[(receive - 1) - i]; >>>> >>>> >>>> Here are two moments I don't understand: >>>> 1. The last parameter of ps2_adjust_timeout is timeout in jiffies (it >>>> is compared against 100ms). However, timeout is assigned to result of >>>> wait_event_timeout, which returns 0 or 1. This does not make sense to >>>> me. What am I missing? >>> >>> The fact that wait_event_timeout can return value greater than one: >>> >>> * Returns: >>> * 0 if the @condition evaluated to %false after the @timeout elapsed, >>> * 1 if the @condition evaluated to %true after the @timeout elapsed, >>> * or the remaining jiffies (at least 1) if the @condition evaluated >>> ^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> >> OK, makes sense now! >> >>>> 2. This code pays great attention to timeouts, but in the end I don't >>>> see how it handles timeouts. That is, if a timeout is happened, we >>>> still copyout (garbage) from cmdbuf. What am I missing here? >>> >>> Once upon a time wait_event() did not return positive value when >>> timeout expired and then condition satisfied. So we just examine the >>> final state (psmpouse->cmdcnt should be 0 if command actually >>> succeeded) and even if we copy in garbage nobody should care since >>> we'll return error in this case. >> >> >> I see. >> But the cmdcnt is re-read after copying out response. So it is >> possible that we read garbage response, but then read cmdcnt==0 and >> return OK to caller. > > That assumes that we actually timed out, and while we were copying the > data the response finally came. > >> >> So far I have something along the following lines to fix data races in libps2.c > > I don't know, maybe we should simply move call to > serio_pause_rx(ps2dev->serio) higher, before we check ps2dev->cmdcnt, > and move copying of the buffer down, after checking cmdcnt. By the way, please either drop ktsan group from public postngs or open it to post from public. Thanks. -- Dmitry