[PATCH] Fix Bug 10504 - losetup possible circular locking

[PATCH] Fix Bug 10504 - losetup possible circular locking

[Nikanth Karthikesan]

With CONFIG_PROVE_LOCKING enabled

$ losetup /dev/loop0 file
$ losetup -o 32256 /dev/loop1 /dev/loop0 

$ losetup -d /dev/loop1
$ losetup -d /dev/loop0

triggers a [ INFO: possible circular locking dependency detected ]

I think this warning is a false positive. 

Open/close on a loop device acquires bd_mutex of the device before
acquiring lo_ctl_mutex of the same device. For ioctl(LOOP_CLR_FD) after
acquiring lo_ctl_mutex, fput on the backing_file might acquire the bd_mutex of
a device, if backing file is a device and this is the last reference to the
file being dropped . But it is guaranteed that it is impossible to have a
circular list of backing devices.(say loop2->loop1->loop0->loop2 is not
possible), which guarantees that this can never deadlock.

So this warning should be suppressed. It is very difficult to annotate lockdep
not to warn here in the correct way. A simple way to silence lockdep could be
to mark the lo_ctl_mutex in ioctl to be a sub class, but this might mask some
other real bugs.

--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1164,7 +1164,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 	struct loop_device *lo = bdev->bd_disk->private_data;
 	int err;
 
-	mutex_lock(&lo->lo_ctl_mutex);
+	mutex_lock_nested(&lo->lo_ctl_mutex, 1);
 	switch (cmd) {
 	case LOOP_SET_FD:
 		err = loop_set_fd(lo, mode, bdev, arg);

Or actually marking the bd_mutex after lo_ctl_mutex as a sub class could be 
a better solution.

Luckily it is easy to avoid calling fput on backing file with lo_ctl_mutex
held, so no lockdep annotation is required. 

If you do not like the special handling of the lo_ctl_mutex just for the
LOOP_CLR_FD ioctl in lo_ioctl(), the mutex handling could be moved inside
each of the individual ioctl handlers and I could send you another patch.

Thanks
Nikanth Karthikesan

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

Fix Bug 10504 - losetup possible circular locking

Avoid triggering a circular dependency warning by calling fput on the backing 
file with lo_ctl_mutex held. If the backing file is a device, fput might try 
to acquire bd_mutex of a that device which triggers a circular dependency 
warning.

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index edbaac6..5588f67 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -942,11 +942,18 @@ static int loop_clr_fd(struct loop_device *lo, struct block_device *bdev)
 		bd_set_size(bdev, 0);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
-	fput(filp);
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
 	if (max_part > 0)
 		ioctl_by_bdev(bdev, BLKRRPART, 0);
+	mutex_unlock(&lo->lo_ctl_mutex);
+	/*
+	 * Need not hold lo_ctl_mutex to fput backing file.
+	 * Calling fput holding lo_ctl_mutex triggers a circular
+	 * lock dependency possibility warning as fput can take
+	 * bd_mutex which is usually taken before lo_ctl_mutex.
+	 */
+	fput(filp);
 	return 0;
 }
 
@@ -1173,7 +1180,10 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		err = loop_change_fd(lo, bdev, arg);
 		break;
 	case LOOP_CLR_FD:
+		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */
 		err = loop_clr_fd(lo, bdev);
+		if (!err)
+			goto out_unlocked;
 		break;
 	case LOOP_SET_STATUS:
 		err = loop_set_status_old(lo, (struct loop_info __user *) arg);
@@ -1191,6 +1201,8 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
 	}
 	mutex_unlock(&lo->lo_ctl_mutex);
+
+out_unlocked:
 	return err;
 }
 

Re: [PATCH] Fix Bug 10504 - losetup possible circular locking

[Nikanth Karthikesan]

Hi Jens

Did you get to look at this? Can you ACK/NACK this one?

Thanks
Nikanth

On Thursday 12 March 2009 13:41:12 Nikanth Karthikesan wrote:
> With CONFIG_PROVE_LOCKING enabled
>
> $ losetup /dev/loop0 file
> $ losetup -o 32256 /dev/loop1 /dev/loop0
>
> $ losetup -d /dev/loop1
> $ losetup -d /dev/loop0
>
> triggers a [ INFO: possible circular locking dependency detected ]
>
> I think this warning is a false positive.
>
> Open/close on a loop device acquires bd_mutex of the device before
> acquiring lo_ctl_mutex of the same device. For ioctl(LOOP_CLR_FD) after
> acquiring lo_ctl_mutex, fput on the backing_file might acquire the bd_mutex
> of a device, if backing file is a device and this is the last reference to
> the file being dropped . But it is guaranteed that it is impossible to have
> a circular list of backing devices.(say loop2->loop1->loop0->loop2 is not
> possible), which guarantees that this can never deadlock.
>
> So this warning should be suppressed. It is very difficult to annotate
> lockdep not to warn here in the correct way. A simple way to silence
> lockdep could be to mark the lo_ctl_mutex in ioctl to be a sub class, but
> this might mask some other real bugs.
>
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1164,7 +1164,7 @@ static int lo_ioctl(struct block_device *bdev,
> fmode_t mode, struct loop_device *lo = bdev->bd_disk->private_data;
>  	int err;
>
> -	mutex_lock(&lo->lo_ctl_mutex);
> +	mutex_lock_nested(&lo->lo_ctl_mutex, 1);
>  	switch (cmd) {
>  	case LOOP_SET_FD:
>  		err = loop_set_fd(lo, mode, bdev, arg);
>
> Or actually marking the bd_mutex after lo_ctl_mutex as a sub class could be
> a better solution.
>
> Luckily it is easy to avoid calling fput on backing file with lo_ctl_mutex
> held, so no lockdep annotation is required.
>
> If you do not like the special handling of the lo_ctl_mutex just for the
> LOOP_CLR_FD ioctl in lo_ioctl(), the mutex handling could be moved inside
> each of the individual ioctl handlers and I could send you another patch.
>
> Thanks
> Nikanth Karthikesan
>
> Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
>
> ---
>
> Fix Bug 10504 - losetup possible circular locking
>
> Avoid triggering a circular dependency warning by calling fput on the
> backing file with lo_ctl_mutex held. If the backing file is a device, fput
> might try to acquire bd_mutex of a that device which triggers a circular
> dependency warning.
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index edbaac6..5588f67 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -942,11 +942,18 @@ static int loop_clr_fd(struct loop_device *lo, struct
> block_device *bdev) bd_set_size(bdev, 0);
>  	mapping_set_gfp_mask(filp->f_mapping, gfp);
>  	lo->lo_state = Lo_unbound;
> -	fput(filp);
>  	/* This is safe: open() is still holding a reference. */
>  	module_put(THIS_MODULE);
>  	if (max_part > 0)
>  		ioctl_by_bdev(bdev, BLKRRPART, 0);
> +	mutex_unlock(&lo->lo_ctl_mutex);
> +	/*
> +	 * Need not hold lo_ctl_mutex to fput backing file.
> +	 * Calling fput holding lo_ctl_mutex triggers a circular
> +	 * lock dependency possibility warning as fput can take
> +	 * bd_mutex which is usually taken before lo_ctl_mutex.
> +	 */
> +	fput(filp);
>  	return 0;
>  }
>
> @@ -1173,7 +1180,10 @@ static int lo_ioctl(struct block_device *bdev,
> fmode_t mode, err = loop_change_fd(lo, bdev, arg);
>  		break;
>  	case LOOP_CLR_FD:
> +		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */
>  		err = loop_clr_fd(lo, bdev);
> +		if (!err)
> +			goto out_unlocked;
>  		break;
>  	case LOOP_SET_STATUS:
>  		err = loop_set_status_old(lo, (struct loop_info __user *) arg);
> @@ -1191,6 +1201,8 @@ static int lo_ioctl(struct block_device *bdev,
> fmode_t mode, err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
>  	}
>  	mutex_unlock(&lo->lo_ctl_mutex);
> +
> +out_unlocked:
>  	return err;
>  }

Re: [PATCH] Fix Bug 10504 - losetup possible circular locking

[Jens Axboe]

On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> Hi Jens
> 
> Did you get to look at this? Can you ACK/NACK this one?

It looks fine, I have applied it.

> 
> Thanks
> Nikanth
> 
> On Thursday 12 March 2009 13:41:12 Nikanth Karthikesan wrote:
> > With CONFIG_PROVE_LOCKING enabled
> >
> > $ losetup /dev/loop0 file
> > $ losetup -o 32256 /dev/loop1 /dev/loop0
> >
> > $ losetup -d /dev/loop1
> > $ losetup -d /dev/loop0
> >
> > triggers a [ INFO: possible circular locking dependency detected ]
> >
> > I think this warning is a false positive.
> >
> > Open/close on a loop device acquires bd_mutex of the device before
> > acquiring lo_ctl_mutex of the same device. For ioctl(LOOP_CLR_FD) after
> > acquiring lo_ctl_mutex, fput on the backing_file might acquire the bd_mutex
> > of a device, if backing file is a device and this is the last reference to
> > the file being dropped . But it is guaranteed that it is impossible to have
> > a circular list of backing devices.(say loop2->loop1->loop0->loop2 is not
> > possible), which guarantees that this can never deadlock.
> >
> > So this warning should be suppressed. It is very difficult to annotate
> > lockdep not to warn here in the correct way. A simple way to silence
> > lockdep could be to mark the lo_ctl_mutex in ioctl to be a sub class, but
> > this might mask some other real bugs.
> >
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1164,7 +1164,7 @@ static int lo_ioctl(struct block_device *bdev,
> > fmode_t mode, struct loop_device *lo = bdev->bd_disk->private_data;
> >  	int err;
> >
> > -	mutex_lock(&lo->lo_ctl_mutex);
> > +	mutex_lock_nested(&lo->lo_ctl_mutex, 1);
> >  	switch (cmd) {
> >  	case LOOP_SET_FD:
> >  		err = loop_set_fd(lo, mode, bdev, arg);
> >
> > Or actually marking the bd_mutex after lo_ctl_mutex as a sub class could be
> > a better solution.
> >
> > Luckily it is easy to avoid calling fput on backing file with lo_ctl_mutex
> > held, so no lockdep annotation is required.
> >
> > If you do not like the special handling of the lo_ctl_mutex just for the
> > LOOP_CLR_FD ioctl in lo_ioctl(), the mutex handling could be moved inside
> > each of the individual ioctl handlers and I could send you another patch.
> >
> > Thanks
> > Nikanth Karthikesan
> >
> > Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>
> >
> > ---
> >
> > Fix Bug 10504 - losetup possible circular locking
> >
> > Avoid triggering a circular dependency warning by calling fput on the
> > backing file with lo_ctl_mutex held. If the backing file is a device, fput
> > might try to acquire bd_mutex of a that device which triggers a circular
> > dependency warning.
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index edbaac6..5588f67 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -942,11 +942,18 @@ static int loop_clr_fd(struct loop_device *lo, struct
> > block_device *bdev) bd_set_size(bdev, 0);
> >  	mapping_set_gfp_mask(filp->f_mapping, gfp);
> >  	lo->lo_state = Lo_unbound;
> > -	fput(filp);
> >  	/* This is safe: open() is still holding a reference. */
> >  	module_put(THIS_MODULE);
> >  	if (max_part > 0)
> >  		ioctl_by_bdev(bdev, BLKRRPART, 0);
> > +	mutex_unlock(&lo->lo_ctl_mutex);
> > +	/*
> > +	 * Need not hold lo_ctl_mutex to fput backing file.
> > +	 * Calling fput holding lo_ctl_mutex triggers a circular
> > +	 * lock dependency possibility warning as fput can take
> > +	 * bd_mutex which is usually taken before lo_ctl_mutex.
> > +	 */
> > +	fput(filp);
> >  	return 0;
> >  }
> >
> > @@ -1173,7 +1180,10 @@ static int lo_ioctl(struct block_device *bdev,
> > fmode_t mode, err = loop_change_fd(lo, bdev, arg);
> >  		break;
> >  	case LOOP_CLR_FD:
> > +		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */
> >  		err = loop_clr_fd(lo, bdev);
> > +		if (!err)
> > +			goto out_unlocked;
> >  		break;
> >  	case LOOP_SET_STATUS:
> >  		err = loop_set_status_old(lo, (struct loop_info __user *) arg);
> > @@ -1191,6 +1201,8 @@ static int lo_ioctl(struct block_device *bdev,
> > fmode_t mode, err = lo->ioctl ? lo->ioctl(lo, cmd, arg) : -EINVAL;
> >  	}
> >  	mutex_unlock(&lo->lo_ctl_mutex);
> > +
> > +out_unlocked:
> >  	return err;
> >  }
> 
> 

-- 
Jens Axboe

Re: [PATCH] Fix Bug 10504 - losetup possible circular locking

[Nikanth Karthikesan]

On Tuesday 24 March 2009 17:00:32 Jens Axboe wrote:
> On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> > Hi Jens
> >
> > Did you get to look at this? Can you ACK/NACK this one?
>
> It looks fine, I have applied it.
>

Thanks. But it looks like, the patch I quoted in the changelog had the "---" 
delimiter, which made you commit without the full changelog and signed-off. 
Sorry for that mistake.

Thanks
Nikanth

Re: [PATCH] Fix Bug 10504 - losetup possible circular locking

[Jens Axboe]

On Thu, Mar 26 2009, Nikanth Karthikesan wrote:
> On Tuesday 24 March 2009 17:00:32 Jens Axboe wrote:
> > On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> > > Hi Jens
> > >
> > > Did you get to look at this? Can you ACK/NACK this one?
> >
> > It looks fine, I have applied it.
> >
> 
> Thanks. But it looks like, the patch I quoted in the changelog had the "---" 
> delimiter, which made you commit without the full changelog and signed-off. 
> Sorry for that mistake.

That was actually done on purpose, I wanted the whole thing there! It
was a lot more useful than the short summary. Never be afraid to write
exhaustive changelogs, I even put the test app in there for some cases
when that makes sense.

-- 
Jens Axboe

Re: [PATCH] Fix Bug 10504 - losetup possible circular locking

[Nikanth Karthikesan]

On Thursday 26 March 2009 15:18:47 Jens Axboe wrote:
> On Thu, Mar 26 2009, Nikanth Karthikesan wrote:
> > On Tuesday 24 March 2009 17:00:32 Jens Axboe wrote:
> > > On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> > > > Hi Jens
> > > >
> > > > Did you get to look at this? Can you ACK/NACK this one?
> > >
> > > It looks fine, I have applied it.
> >
> > Thanks. But it looks like, the patch I quoted in the changelog had the
> > "---" delimiter, which made you commit without the full changelog and
> > signed-off. Sorry for that mistake.
>
> That was actually done on purpose, I wanted the whole thing there! It
> was a lot more useful than the short summary. Never be afraid to write
> exhaustive changelogs, I even put the test app in there for some cases
> when that makes sense.

Yes, but because of a delimiter '---' in my changelog, the git log has only 
half of the changelog. IOW, the whole thing is not there.

Thanks
Nikanth

Re: [PATCH] Fix Bug 10504 - losetup possible circular locking

[Jens Axboe]

On Thu, Mar 26 2009, Nikanth Karthikesan wrote:
> On Thursday 26 March 2009 15:18:47 Jens Axboe wrote:
> > On Thu, Mar 26 2009, Nikanth Karthikesan wrote:
> > > On Tuesday 24 March 2009 17:00:32 Jens Axboe wrote:
> > > > On Tue, Mar 24 2009, Nikanth Karthikesan wrote:
> > > > > Hi Jens
> > > > >
> > > > > Did you get to look at this? Can you ACK/NACK this one?
> > > >
> > > > It looks fine, I have applied it.
> > >
> > > Thanks. But it looks like, the patch I quoted in the changelog had the
> > > "---" delimiter, which made you commit without the full changelog and
> > > signed-off. Sorry for that mistake.
> >
> > That was actually done on purpose, I wanted the whole thing there! It
> > was a lot more useful than the short summary. Never be afraid to write
> > exhaustive changelogs, I even put the test app in there for some cases
> > when that makes sense.
> 
> Yes, but because of a delimiter '---' in my changelog, the git log has only 
> half of the changelog. IOW, the whole thing is not there.

Hmm right, it does appear to be missing everything from (and including)
the

--- a/drivers/block/loop.c

part. I'll fix it up, thanks for noticing!

-- 
Jens Axboe