netdev - Re: [PATCH] irda: remove BKL from irnet open function

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 for Android: free password hash cracker in your pocket

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

Message-Id: <201002012220.30887.arnd@arndb.de>
Date:	Mon, 1 Feb 2010 22:20:30 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Thadeu Lima de Souza Cascardo <cascardo@...oscopio.com>
Cc:	John Kacur <jkacur@...il.com>, Samuel Ortiz <samuel@...tiz.org>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] irda: remove BKL from irnet open function

On Monday 01 February 2010, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Feb 01, 2010 at 09:32:30PM +0100, John Kacur wrote:
> > On Mon, Feb 1, 2010 at 7:18 PM, Thadeu Lima de Souza Cascardo
> > <cascardo@...oscopio.com> wrote:
> And is it possible that ioctl will be called before open returns? If it
> is, then, yes, this is not safe. But I don't really believe the case. Or
> is it?

ioctl may be called on an open file descriptor while open is called
by another thread to open a second file descriptor for the same device.

> Or is it only possible to happen with different struct file*? In that
> case, open is only allocating and initializing the irnet_socket *ap.
> Then, ioctl uses it. There is some race between the different ioctls,
> but no race between open/ioctl for different opened devices. That is, a
> process may open /dev/irnet while another process is issuing ioctls to
> its own opened /dev/irnet.

right.

> Besides, dev_irnet_ioctl uses the file private_data to get to the
> irnet_socket, which is the last thing the open call does. I assume doing
> an attribution to a pointer is atomic in all architectures supported by
> Linux currently, isn't it?

The pointer assignment is atomic, but it may not be synchronized to data
pointed to it. On Alpha (probably no others so far), this would result
in irnet_socket seen as uninitialized after the pointer to is can be
seen as valid if there was no locking. No architecture would read an
invalid pointer though.

I guess that what John was trying to point out actually is the fact that
you shouldn't really do the BKL removal in one function only but rather
do it for the whole driver at once. The irnet driver uses the BKL in
open, ioctl and llseek, so if you want to clean up that driver, please
introduce proper locking in the driver and do all of the three.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html