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

[Alexander Popov]

On 03.11.2019 19:45, Linus Torvalds wrote:
> On Sat, Nov 2, 2019 at 12:03 PM Alexander Popov <alex.popov@xxxxxxxxx> 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