linux-kernel - Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep

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: <55674412.rAimUmdW3X@wuerfel>
Date:	Fri, 07 Feb 2014 11:17:19 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Hans Verkuil <hverkuil@...all.nl>
Cc:	linux-kernel@...r.kernel.org,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	linux-media@...r.kernel.org
Subject: Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

On Friday 07 February 2014 10:32:28 Hans Verkuil wrote:
>         mutex_lock(&dev->lock);
>         if (dev->rdsstat == 0)
>                 cadet_start_rds(dev);
> -       if (dev->rdsin == dev->rdsout) {
> +       while (dev->rdsin == dev->rdsout) {
>                 if (file->f_flags & O_NONBLOCK) {
>                         i = -EWOULDBLOCK;
>                         goto unlock;
>                 }
>                 mutex_unlock(&dev->lock);
> -               interruptible_sleep_on(&dev->read_queue);
> +               if (wait_event_interruptible(&dev->read_queue,
> +                                            dev->rdsin != dev->rdsout))
> +                       return -EINTR;
>                 mutex_lock(&dev->lock);
>         }
>         while (i < count && dev->rdsin != dev->rdsout)
> 

This will normally work, but now the mutex is no longer
protecting the shared access to the dev->rdsin and
dev->rdsout variables, which was evidently the intention
of the author of the original code.

AFAICT, the possible result is a similar race as before:
if once CPU changes dev->rdsin after the process in
cadet_read dropped the lock, the wakeup may get lost.

It's quite possible this race never happens in practice,
but the code is probably still wrong.

If you think we don't actually need the lock to check
"dev->rdsin != dev->rdsout", the code can be simplified
further, to

	if ((dev->rdsin == dev->rdsout) && (file->f_flags & O_NONBLOCK)) {
	        return -EWOULDBLOCK;
	i = wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);
	if (i)
		return i;
	
	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/