linux-kernel - Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7ec33848-67a6-069a-132c-f8550f6e090f@linux.com>
Date:   Mon, 4 Nov 2019 00:44:12 +0300
From:   Alexander Popov <alex.popov@...ux.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Hans Verkuil <hverkuil-cisco@...all.nl>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Security Officers <security@...nel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexander Popov <alex.popov@...ux.com>
Subject: Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race
 conditions on streaming stop

On 03.11.2019 19:45, Linus Torvalds wrote:
> On Sat, Nov 2, 2019 at 12:03 PM Alexander Popov <alex.popov@...ux.com> wrote:
>>
>> -               mutex_lock(&dev->mutex);
>> +               if (!mutex_trylock(&dev->mutex)) {
>> +                       schedule_timeout(1);
>> +                       continue;
>> +               }
>> +
> 
> I just realized that this too is wrong. It _works_, but because it
> doesn't actually set the task state to anything particular before
> scheduling, it's basically pointless. It calls the scheduler, but it
> won't delay anything, because the task stays runnable.

Linus, thanks for noticing that.

I've double-checked: I didn't manage to get a deadlock with schedule_timeout(1)
on the kernel with CONFIG_FREEZER disabled and CONFIG_PREEMPT_NONE=y.
But setting a bigger timeout argument (e.g. 1000) doesn't change the behavior,
which agrees with your statement.

> So what you presumably want to use is either "cond_resched()" (to make
> sure others get to run with no delay) or
> "schedule_timeout_uninterruptible(1)" which actually sets the process
> state to TASK_UNINTERRUPTIBLE.

I changed it to schedule_timeout_uninterruptible(1).

I'll send the v4 shortly as a reply to this thread, because I refer to it in the
oss-security mailing list.

> The above works, but it's basically nonsensical. My bad for not
> noticing earlier.

Thank you, now I know.

Best regards,
Alexander