diff mbox series

block, loop: Increment diskseq when releasing a loop device

Message ID 20230601222656.2062-1-demi@invisiblethingslab.com
State New
Headers
Series block, loop: Increment diskseq when releasing a loop device |

Commit Message

Demi Marie Obenour June 1, 2023, 10:26 p.m. UTC 
  The previous patch for checking diskseq in blkback is not enough to
prevent the following race:

1. Program X opens a loop device
2. Program X gets the diskseq of the loop device.
3. Program X associates a file with the loop device.
4. Program X passes the loop device major, minor, and diskseq to
   something.
5. Program X exits.
6. Program Y detaches the file from the loop device.
7. Program Y attaches a different file to the loop device.
8. The opener finally gets around to opening the loop device and checks
   that the diskseq is what it expects it to be.  Even though the
   diskseq is the expected value, the result is that the opener is
   accessing the wrong file.

To prevent this race condition, increment the diskseq of a loop device
when it is detached from its file descriptor.  This causes blkback (or
any other program, for that matter) to fail at step 8.  Export the
inc_diskseq() function to make this possible.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
I considered destroying the loop device altogether instead of bumping
its diskseq, but was not able to accomplish that.  Suggestions welcome.
---
 block/genhd.c        | 1 +
 drivers/block/loop.c | 6 ++++++
 2 files changed, 7 insertions(+)

Comments

Christoph Hellwig June 7, 2023, 7:42 a.m. UTC | #1 
> +++ b/block/genhd.c
> @@ -1502,3 +1502,4 @@ void inc_diskseq(struct gendisk *disk)
>  {
>  	disk->diskseq = atomic64_inc_return(&diskseq);
>  }
> +EXPORT_SYMBOL(inc_diskseq);

I really do not like exporting this as a lowlevel function.  If we
increment the sequence it should be part of a higher level operation.

> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1205,6 +1205,12 @@ static void __loop_clr_fd(struct loop_device *lo, bool release)
>  	if (!part_shift)
>  		set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
>  	mutex_lock(&lo->lo_mutex);
> +
> +	/*
> +	 * Increment the disk sequence number, so that userspace knows this
> +	 * device now points to something else.
> +	 */
> +	inc_diskseq(lo->lo_disk);

And I'm not sure why we even need this.  __loop_clr_fd
already calls disk_force_media_change, which calls inc_diskseq.
Why do we need an extra increment?
Demi Marie Obenour June 7, 2023, 3:23 p.m. UTC | #2 
On Wed, Jun 07, 2023 at 12:42:11AM -0700, Christoph Hellwig wrote:
> > +++ b/block/genhd.c
> > @@ -1502,3 +1502,4 @@ void inc_diskseq(struct gendisk *disk)
> >  {
> >  	disk->diskseq = atomic64_inc_return(&diskseq);
> >  }
> > +EXPORT_SYMBOL(inc_diskseq);
> 
> I really do not like exporting this as a lowlevel function.  If we
> increment the sequence it should be part of a higher level operation.

Fair!

> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1205,6 +1205,12 @@ static void __loop_clr_fd(struct loop_device *lo, bool release)
> >  	if (!part_shift)
> >  		set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
> >  	mutex_lock(&lo->lo_mutex);
> > +
> > +	/*
> > +	 * Increment the disk sequence number, so that userspace knows this
> > +	 * device now points to something else.
> > +	 */
> > +	inc_diskseq(lo->lo_disk);
> 
> And I'm not sure why we even need this.  __loop_clr_fd
> already calls disk_force_media_change, which calls inc_diskseq.
> Why do we need an extra increment?

How does disk_force_media_change() call inc_diskseq()?  I don’t see any
calls in the source code.  I’m going to use systemtap to see if there is
an indirect call chain.
Demi Marie Obenour June 7, 2023, 4:42 p.m. UTC | #3 
On Wed, Jun 07, 2023 at 11:23:00AM -0400, Demi Marie Obenour wrote:
> > > --- a/drivers/block/loop.c
> > > +++ b/drivers/block/loop.c
> > > @@ -1205,6 +1205,12 @@ static void __loop_clr_fd(struct loop_device *lo, bool release)
> > >  	if (!part_shift)
> > >  		set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
> > >  	mutex_lock(&lo->lo_mutex);
> > > +
> > > +	/*
> > > +	 * Increment the disk sequence number, so that userspace knows this
> > > +	 * device now points to something else.
> > > +	 */
> > > +	inc_diskseq(lo->lo_disk);
> > 
> > And I'm not sure why we even need this.  __loop_clr_fd
> > already calls disk_force_media_change, which calls inc_diskseq.
> > Why do we need an extra increment?
> 
> How does disk_force_media_change() call inc_diskseq()?  I don’t see any
> calls in the source code.  I’m going to use systemtap to see if there is
> an indirect call chain.

Were you thinking of bdev_check_media_change()?  That can call
inc_diskseq() via this call chain:

  bdev_check_media_change()
    disk_clear_events()
      disk_check_events()
        inc_diskseq()

disk_force_media_change() does not call inc_diskseq(), and I checked
that calling losetup -D does not change the diskseq of a loop device.
From what you have writte, I’m pretty sure that’s a bug in
disk_force_media_change(), though.  I’ll send a v3 that adds this call.
diff mbox series

Patch

diff --git a/block/genhd.c b/block/genhd.c
index 1cb489b927d50ab06a84a4bfd6913ca8ba7318d4..c0ca2c387732171321555cd57565fbc606768505 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1502,3 +1502,4 @@  void inc_diskseq(struct gendisk *disk)
 {
 	disk->diskseq = atomic64_inc_return(&diskseq);
 }
+EXPORT_SYMBOL(inc_diskseq);
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index bc31bb7072a2cb7294d32066f5d0aa14130349b4..05ea5fb41508b4106f184dd6b4c37942716bdcac 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1205,6 +1205,12 @@  static void __loop_clr_fd(struct loop_device *lo, bool release)
 	if (!part_shift)
 		set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 	mutex_lock(&lo->lo_mutex);
+
+	/*
+	 * Increment the disk sequence number, so that userspace knows this
+	 * device now points to something else.
+	 */
+	inc_diskseq(lo->lo_disk);
 	lo->lo_state = Lo_unbound;
 	mutex_unlock(&lo->lo_mutex);