Re: possible deadlock in blkdev_reread_part

[Eric Biggers]

On Wed, Nov 01, 2017 at 10:02:44PM +0300, 'Dmitry Vyukov' via syzkaller-bugs wrote:
> 
> Still happens on linux-next 36ef71cae353f88fd6e095e2aaa3e5953af1685d (Oct 20).
> Note repro needs to be compiled with -m32
> 
> [  243.819514] ======================================================
> [  243.820949] WARNING: possible circular locking dependency detected
> [  243.822417] 4.14.0-rc5-next-20171018 #15 Not tainted
> [  243.823592] ------------------------------------------------------
> [  243.825012] a.out/11871 is trying to acquire lock:
> [  243.826182]  (&bdev->bd_mutex){+.+.}, at: [<ffffffff8245f13e>]
> blkdev_reread_part+0x1e/0x40
> [  243.828317]
> [  243.828317] but task is already holding lock:
> [  243.829669]  (&lo->lo_ctl_mutex#2){+.+.}, at: [<ffffffff83867189>]
> lo_compat_ioctl+0x119/0x150
> [  243.831728]
> [  243.831728] which lock already depends on the new lock.
> [  243.831728]
> [  243.833373]

Here's a simplified reproducer:

	#include <fcntl.h>
	#include <linux/loop.h>
	#include <sys/ioctl.h>
	#include <unistd.h>

	int main()
	{
		int loopfd, fd;
		struct loop_info info = { .lo_flags = LO_FLAGS_PARTSCAN };

		loopfd = open("/dev/loop0", O_RDWR);

		fd = open("/bin/ls", O_RDONLY);

		ioctl(loopfd, LOOP_SET_FD, fd);

		ioctl(loopfd, LOOP_SET_STATUS, &info);
	}

It still needs to be compiled with -m32.  The reason is that lo_ioctl() has:

	mutex_lock_nested(&lo->lo_ctl_mutex, 1);

but lo_compat_ioctl() has:

	mutex_lock(&lo->lo_ctl_mutex);

But ->lo_ctl_mutex isn't actually being nested under itself, so I don't think
the "nested" annotation is actually appropriate.

It seems that ->bd_mutex is held while opening and closing block devices, which
should rank it above both ->lo_ctl_mutex and loop_index_mutex (see lo_open() and
lo_release()).

But blkdev_reread_part(), which takes ->bd_mutex, is called from some of the
ioctls while ->lo_ctl_mutex is held.

Perhaps we should call blkdev_reread_part() at the end of the ioctls, after
->lo_ctl_mutex has been dropped?  But it looks like that can do I/O to the
device, which probably could race with loop_clr_fd()...

Or perhaps we should just take both locks for the ioctls, in the order
->bd_mutex, then ->lo_ctl_mutex -- and then use __blkdev_reread_part()?

Eric