Proposal: use disk sequence numbers to avoid races in blkback

Proposal: use disk sequence numbers to avoid races in blkback

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 4038 bytes --]

Proposal: Check disk sequence numbers in blkback
================================================

Currently, adding block devices to a domain is racy.  libxl writes the
major and minor number of the device to XenStore, but it does not keep
the block device open until blkback has opened it.  This creates a race
condition, as it is possible for the device to be destroyed and another
device allocated with the same major and minor numbers.  Loop devices
are the most obvious example, since /dev/loop0 can be reused again and
again, but the same problem can also happen with device-mapper devices.
If the major and minor numbers are reused before blkback has attached to
the device, blkback will pass the wrong device to the domain, with
obvious security consequences.

Other programs on Linux have the same problem, and a solution was
committed upstream in the form of disk sequence numbers.  A disk
sequence number, or diskseq, is a 64-bit unsigned monotonically
increasing counter.  The combination of a major and minor number and a
disk sequence number uniquely identifies a block device for the entire
uptime of the system.

I propose that blkback check for an unsigned 64-bit hexadecimal XenStore
entry named “diskseq”.  If the entry exists, blkback checks that the
number stored there matches the disk sequence number of the device.  If
it does not exist, the check is skipped.  If reading the entry fails for
any other reason, the entry is malformed, or if the sequence number is
wrong, blkback refuses to export the device.

The toolstack changes are more involved for two reasons:

1. To ensure that loop devices are not leaked if the toolstack crashes,
   they must be created with the delete-on-close flag set.  This
   requires that the toolstack hold the device open until blkback has
   acquired a handle to it.

2. For block devices that are opened by path, the toolstack needs to
   ensure that the device it has opened is actually the device it
   intended to open.  This requires device-specific verification of the
   open file descriptor.  This is not needed for regular files, as the
   LOOP_CONFIGURE ioctl is called on an existing loop device and sets
   its backing file.

The first is fairly easy in C.  It can be accomplished by means of a
XenStore watch on the “status” entry.  Once that watch fires, blkback
has opened the device, so the toolstack can safely close its file
descriptor.

The second is significantly more difficult.  It requires the block
script to be aware of at least device-mapper devices and LVM2 logical
volumes.  The general technique is common to all block devices: obtain
the sequence number (via the BLKGETDISKSEQ() ioctl) and its major and
minor numbers (via fstat()).  Then open /sys/dev/block/MAJOR:MINOR to
get a directory file descriptor, and use openat(2) and read(2) to get
various sysfs attributes.  Finally, read the diskseq sysfs attribute and
check that it matches the sequence number from BLKGETDISKSEQ().
Alternatively, one can use device-specific methods, such as
device-mapper ioctls.

Device-mapper devices can be detected via the ‘dm/name’ sysfs attribute,
which must match the name under ‘/dev/mapper/’.  If the name is of the
form ‘/dev/X/Y’, and the ‘dm/uuid’ attribute starts with the literal
string “LVM-”, then the expected ‘dm/name’ attribute should be found by
doubling all ‘-’ characters in X and Y, and then joining X and Y with
another ‘-’.  This accounts for LVM2 logical volumes.  Alternatively,
one can use device-mapper ioctls to both check if a device is a
device-mapper device, and to obtain its name and UUID.  I plan on going
with the latter route.

There are *many* other rules that might need to be followed, but these
are the most important ones.  In particular, this is sufficient for
device-mapper devices, which are by far the most important case for now.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Proposal: use disk sequence numbers to avoid races in blkback

[Roger Pau Monné]

On Thu, May 05, 2022 at 08:30:17PM -0400, Demi Marie Obenour wrote:
> Proposal: Check disk sequence numbers in blkback
> ================================================
> 
> Currently, adding block devices to a domain is racy.  libxl writes the
> major and minor number of the device to XenStore, but it does not keep
> the block device open until blkback has opened it.  This creates a race
> condition, as it is possible for the device to be destroyed and another
> device allocated with the same major and minor numbers.  Loop devices
> are the most obvious example, since /dev/loop0 can be reused again and
> again, but the same problem can also happen with device-mapper devices.
> If the major and minor numbers are reused before blkback has attached to
> the device, blkback will pass the wrong device to the domain, with
> obvious security consequences.
> 
> Other programs on Linux have the same problem, and a solution was
> committed upstream in the form of disk sequence numbers.  A disk
> sequence number, or diskseq, is a 64-bit unsigned monotonically
> increasing counter.  The combination of a major and minor number and a
> disk sequence number uniquely identifies a block device for the entire
> uptime of the system.

Seems fine to me, this is just an extra check to make sure the block
device opened by blkback is the one that user space intended.  I would
see diskseq as a kind of checksum.

> I propose that blkback check for an unsigned 64-bit hexadecimal XenStore
> entry named “diskseq”.  If the entry exists, blkback checks that the
> number stored there matches the disk sequence number of the device.  If
> it does not exist, the check is skipped.  If reading the entry fails for
> any other reason, the entry is malformed, or if the sequence number is
> wrong, blkback refuses to export the device.
> 
> The toolstack changes are more involved for two reasons:
> 
> 1. To ensure that loop devices are not leaked if the toolstack crashes,
>    they must be created with the delete-on-close flag set.  This
>    requires that the toolstack hold the device open until blkback has
>    acquired a handle to it.

Does this work with loop devices?  I would expect that you need to
issue a losetup call to detach the device.

Even more, the loop device is created by the block script, but there's
also a window between the block script execution and the toolstack
knowing about the device, which could also allow for a leak?

> 
> 2. For block devices that are opened by path, the toolstack needs to
>    ensure that the device it has opened is actually the device it
>    intended to open.  This requires device-specific verification of the
>    open file descriptor.  This is not needed for regular files, as the
>    LOOP_CONFIGURE ioctl is called on an existing loop device and sets
>    its backing file.
> 
> The first is fairly easy in C.  It can be accomplished by means of a
> XenStore watch on the “status” entry.  Once that watch fires, blkback
> has opened the device, so the toolstack can safely close its file
> descriptor.

Does the toolstack really need to close the device?  What harm does it
do to keep the handle open until the domain is destroyed?

What about disk hotplug?  Which entity will keep the device opened in
that case?  Is xl block-attach going to block until the device
switches to the connected state?

> The second is significantly more difficult.  It requires the block
> script to be aware of at least device-mapper devices and LVM2 logical
> volumes.  The general technique is common to all block devices: obtain
> the sequence number (via the BLKGETDISKSEQ() ioctl) and its major and
> minor numbers (via fstat()).  Then open /sys/dev/block/MAJOR:MINOR to
> get a directory file descriptor, and use openat(2) and read(2) to get
> various sysfs attributes.  Finally, read the diskseq sysfs attribute and
> check that it matches the sequence number from BLKGETDISKSEQ().
> Alternatively, one can use device-specific methods, such as
> device-mapper ioctls.
> 
> Device-mapper devices can be detected via the ‘dm/name’ sysfs attribute,
> which must match the name under ‘/dev/mapper/’.  If the name is of the
> form ‘/dev/X/Y’, and the ‘dm/uuid’ attribute starts with the literal
> string “LVM-”, then the expected ‘dm/name’ attribute should be found by
> doubling all ‘-’ characters in X and Y, and then joining X and Y with
> another ‘-’.  This accounts for LVM2 logical volumes.  Alternatively,
> one can use device-mapper ioctls to both check if a device is a
> device-mapper device, and to obtain its name and UUID.  I plan on going
> with the latter route.

Likely a stupid remark, but needs obviously needs to be kept to Linux
only.

Thanks, Roger.

Re: Proposal: use disk sequence numbers to avoid races in blkback

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 6419 bytes --]

On Tue, May 10, 2022 at 12:57:48PM +0200, Roger Pau Monné wrote:
> On Thu, May 05, 2022 at 08:30:17PM -0400, Demi Marie Obenour wrote:
> > Proposal: Check disk sequence numbers in blkback
> > ================================================
> > 
> > Currently, adding block devices to a domain is racy.  libxl writes the
> > major and minor number of the device to XenStore, but it does not keep
> > the block device open until blkback has opened it.  This creates a race
> > condition, as it is possible for the device to be destroyed and another
> > device allocated with the same major and minor numbers.  Loop devices
> > are the most obvious example, since /dev/loop0 can be reused again and
> > again, but the same problem can also happen with device-mapper devices.
> > If the major and minor numbers are reused before blkback has attached to
> > the device, blkback will pass the wrong device to the domain, with
> > obvious security consequences.
> > 
> > Other programs on Linux have the same problem, and a solution was
> > committed upstream in the form of disk sequence numbers.  A disk
> > sequence number, or diskseq, is a 64-bit unsigned monotonically
> > increasing counter.  The combination of a major and minor number and a
> > disk sequence number uniquely identifies a block device for the entire
> > uptime of the system.
> 
> Seems fine to me, this is just an extra check to make sure the block
> device opened by blkback is the one that user space intended.  I would
> see diskseq as a kind of checksum.

Ideally, diskseq would be the primary means of identifying a device, but
that isn’t an option without more substantial changes, sadly.

> > I propose that blkback check for an unsigned 64-bit hexadecimal XenStore
> > entry named “diskseq”.  If the entry exists, blkback checks that the
> > number stored there matches the disk sequence number of the device.  If
> > it does not exist, the check is skipped.  If reading the entry fails for
> > any other reason, the entry is malformed, or if the sequence number is
> > wrong, blkback refuses to export the device.
> > 
> > The toolstack changes are more involved for two reasons:
> > 
> > 1. To ensure that loop devices are not leaked if the toolstack crashes,
> >    they must be created with the delete-on-close flag set.  This
> >    requires that the toolstack hold the device open until blkback has
> >    acquired a handle to it.
> 
> Does this work with loop devices?  I would expect that you need to
> issue a losetup call to detach the device.

That is what the autoclear flag is for.  It will cause the device to be
destroyed by the kernel as soon as the last handle to it has been
closed.  This is why the toolstack needs to hold a file descriptor to
the device.

> Even more, the loop device is created by the block script, but there's
> also a window between the block script execution and the toolstack
> knowing about the device, which could also allow for a leak?

For this to work, either the toolstack or block script will need to open
the file and perform loop(4) ioctls to assign the file descriptor to a
loop device.  This cannot be done by a shell script, so I plan on using
a C program to perform these tasks.  In Qubes OS, I expect this program
to replace the block script entirely, as performance is critical and
flexibility less so.  For upstream, I recommend having the block script
be a script that calls this C program.

> > 2. For block devices that are opened by path, the toolstack needs to
> >    ensure that the device it has opened is actually the device it
> >    intended to open.  This requires device-specific verification of the
> >    open file descriptor.  This is not needed for regular files, as the
> >    LOOP_CONFIGURE ioctl is called on an existing loop device and sets
> >    its backing file.
> > 
> > The first is fairly easy in C.  It can be accomplished by means of a
> > XenStore watch on the “status” entry.  Once that watch fires, blkback
> > has opened the device, so the toolstack can safely close its file
> > descriptor.
> 
> Does the toolstack really need to close the device?  What harm does it
> do to keep the handle open until the domain is destroyed?

This would cause no harm, but it also would not help either, so I do not
see any advantages to doing it.

> What about disk hotplug?  Which entity will keep the device opened in
> that case?  Is xl block-attach going to block until the device
> switches to the connected state?

Whichever program opens the file will need to do this.  This could be
the program that is using libxl or the block script that libxl invokes.
I am not familiar with xl block-attach as Qubes OS uses a custom wrapper
around libvirt.

> > The second is significantly more difficult.  It requires the block
> > script to be aware of at least device-mapper devices and LVM2 logical
> > volumes.  The general technique is common to all block devices: obtain
> > the sequence number (via the BLKGETDISKSEQ() ioctl) and its major and
> > minor numbers (via fstat()).  Then open /sys/dev/block/MAJOR:MINOR to
> > get a directory file descriptor, and use openat(2) and read(2) to get
> > various sysfs attributes.  Finally, read the diskseq sysfs attribute and
> > check that it matches the sequence number from BLKGETDISKSEQ().
> > Alternatively, one can use device-specific methods, such as
> > device-mapper ioctls.
> > 
> > Device-mapper devices can be detected via the ‘dm/name’ sysfs attribute,
> > which must match the name under ‘/dev/mapper/’.  If the name is of the
> > form ‘/dev/X/Y’, and the ‘dm/uuid’ attribute starts with the literal
> > string “LVM-”, then the expected ‘dm/name’ attribute should be found by
> > doubling all ‘-’ characters in X and Y, and then joining X and Y with
> > another ‘-’.  This accounts for LVM2 logical volumes.  Alternatively,
> > one can use device-mapper ioctls to both check if a device is a
> > device-mapper device, and to obtain its name and UUID.  I plan on going
> > with the latter route.
> 
> Likely a stupid remark, but needs obviously needs to be kept to Linux
> only.

Indeed so.  I have CC’d Mariusz Zaborski to check if FreeBSD needs any
similar changes.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Proposal: use disk sequence numbers to avoid races in blkback

[Roger Pau Monné]

On Tue, May 10, 2022 at 12:22:51PM -0400, Demi Marie Obenour wrote:
> On Tue, May 10, 2022 at 12:57:48PM +0200, Roger Pau Monné wrote:
> > On Thu, May 05, 2022 at 08:30:17PM -0400, Demi Marie Obenour wrote:
> > > Proposal: Check disk sequence numbers in blkback
> > > ================================================
> > > 
> > > Currently, adding block devices to a domain is racy.  libxl writes the
> > > major and minor number of the device to XenStore, but it does not keep
> > > the block device open until blkback has opened it.  This creates a race
> > > condition, as it is possible for the device to be destroyed and another
> > > device allocated with the same major and minor numbers.  Loop devices
> > > are the most obvious example, since /dev/loop0 can be reused again and
> > > again, but the same problem can also happen with device-mapper devices.
> > > If the major and minor numbers are reused before blkback has attached to
> > > the device, blkback will pass the wrong device to the domain, with
> > > obvious security consequences.
> > > 
> > > Other programs on Linux have the same problem, and a solution was
> > > committed upstream in the form of disk sequence numbers.  A disk
> > > sequence number, or diskseq, is a 64-bit unsigned monotonically
> > > increasing counter.  The combination of a major and minor number and a
> > > disk sequence number uniquely identifies a block device for the entire
> > > uptime of the system.
> > 
> > Seems fine to me, this is just an extra check to make sure the block
> > device opened by blkback is the one that user space intended.  I would
> > see diskseq as a kind of checksum.
> 
> Ideally, diskseq would be the primary means of identifying a device, but
> that isn’t an option without more substantial changes, sadly.
> 
> > > I propose that blkback check for an unsigned 64-bit hexadecimal XenStore
> > > entry named “diskseq”.  If the entry exists, blkback checks that the
> > > number stored there matches the disk sequence number of the device.  If
> > > it does not exist, the check is skipped.  If reading the entry fails for
> > > any other reason, the entry is malformed, or if the sequence number is
> > > wrong, blkback refuses to export the device.
> > > 
> > > The toolstack changes are more involved for two reasons:
> > > 
> > > 1. To ensure that loop devices are not leaked if the toolstack crashes,
> > >    they must be created with the delete-on-close flag set.  This
> > >    requires that the toolstack hold the device open until blkback has
> > >    acquired a handle to it.
> > 
> > Does this work with loop devices?  I would expect that you need to
> > issue a losetup call to detach the device.
> 
> That is what the autoclear flag is for.  It will cause the device to be
> destroyed by the kernel as soon as the last handle to it has been
> closed.  This is why the toolstack needs to hold a file descriptor to
> the device.

What would happen if the backend closes the device (because the
connection is torn down) and then try to open it again (because the
guest has triggered a reconnection)?

> > Even more, the loop device is created by the block script, but there's
> > also a window between the block script execution and the toolstack
> > knowing about the device, which could also allow for a leak?
> 
> For this to work, either the toolstack or block script will need to open
> the file and perform loop(4) ioctls to assign the file descriptor to a
> loop device.  This cannot be done by a shell script, so I plan on using
> a C program to perform these tasks.  In Qubes OS, I expect this program
> to replace the block script entirely, as performance is critical and
> flexibility less so.  For upstream, I recommend having the block script
> be a script that calls this C program.

block scripts can be plain binary executables, so I think it would be
fine for libxl to just call the executable directly.

> > > 2. For block devices that are opened by path, the toolstack needs to
> > >    ensure that the device it has opened is actually the device it
> > >    intended to open.  This requires device-specific verification of the
> > >    open file descriptor.  This is not needed for regular files, as the
> > >    LOOP_CONFIGURE ioctl is called on an existing loop device and sets
> > >    its backing file.
> > > 
> > > The first is fairly easy in C.  It can be accomplished by means of a
> > > XenStore watch on the “status” entry.  Once that watch fires, blkback
> > > has opened the device, so the toolstack can safely close its file
> > > descriptor.
> > 
> > Does the toolstack really need to close the device?  What harm does it
> > do to keep the handle open until the domain is destroyed?
> 
> This would cause no harm, but it also would not help either, so I do not
> see any advantages to doing it.

Well, seems more complex because you need more synchronization between
blkback and the toolstack in order to detect when blkback has opened
the device.  If this is not strictly required I would rather avoid it:
more complexity just leads to more errors.

> > What about disk hotplug?  Which entity will keep the device opened in
> > that case?  Is xl block-attach going to block until the device
> > switches to the connected state?
> 
> Whichever program opens the file will need to do this.  

This is not trivial to implement with xl, as `xl block-attach` is a
short-lived command that just populates the xenstore entries for the
to be attached device, runs the hotplug script and exits after that.
I'm not sure we would want to change `xl block-attach` behavior to
wait until the backend has opened the device.

> This could be
> the program that is using libxl or the block script that libxl invokes.
> I am not familiar with xl block-attach as Qubes OS uses a custom wrapper
> around libvirt.
> 
> > > The second is significantly more difficult.  It requires the block
> > > script to be aware of at least device-mapper devices and LVM2 logical
> > > volumes.  The general technique is common to all block devices: obtain
> > > the sequence number (via the BLKGETDISKSEQ() ioctl) and its major and
> > > minor numbers (via fstat()).  Then open /sys/dev/block/MAJOR:MINOR to
> > > get a directory file descriptor, and use openat(2) and read(2) to get
> > > various sysfs attributes.  Finally, read the diskseq sysfs attribute and
> > > check that it matches the sequence number from BLKGETDISKSEQ().
> > > Alternatively, one can use device-specific methods, such as
> > > device-mapper ioctls.
> > > 
> > > Device-mapper devices can be detected via the ‘dm/name’ sysfs attribute,
> > > which must match the name under ‘/dev/mapper/’.  If the name is of the
> > > form ‘/dev/X/Y’, and the ‘dm/uuid’ attribute starts with the literal
> > > string “LVM-”, then the expected ‘dm/name’ attribute should be found by
> > > doubling all ‘-’ characters in X and Y, and then joining X and Y with
> > > another ‘-’.  This accounts for LVM2 logical volumes.  Alternatively,
> > > one can use device-mapper ioctls to both check if a device is a
> > > device-mapper device, and to obtain its name and UUID.  I plan on going
> > > with the latter route.
> > 
> > Likely a stupid remark, but needs obviously needs to be kept to Linux
> > only.
> 
> Indeed so.  I have CC’d Mariusz Zaborski to check if FreeBSD needs any
> similar changes.

So you know Mariusz, small world I guess, I've been to quite some BSD
conferences with him.  Hope you are doing fine Mariusz, long time no
see due to the covid mess.

FreeBSD blkback doesn't use loop devices because the kernel has an
interface to read files (so blkback can open raw files directly), so
I think this is all unneeded.

My comment was mostly iff this is implemented it needs to be contained
to Linux specific files (ie: libxl_linux.c).

Thanks, Roger.

Re: Proposal: use disk sequence numbers to avoid races in blkback

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 6870 bytes --]

On Wed, May 11, 2022 at 09:37:54AM +0200, Roger Pau Monné wrote:
> On Tue, May 10, 2022 at 12:22:51PM -0400, Demi Marie Obenour wrote:
> > On Tue, May 10, 2022 at 12:57:48PM +0200, Roger Pau Monné wrote:
> > > On Thu, May 05, 2022 at 08:30:17PM -0400, Demi Marie Obenour wrote:
> > > > Proposal: Check disk sequence numbers in blkback
> > > > ================================================
> > > > 
> > > > Currently, adding block devices to a domain is racy.  libxl writes the
> > > > major and minor number of the device to XenStore, but it does not keep
> > > > the block device open until blkback has opened it.  This creates a race
> > > > condition, as it is possible for the device to be destroyed and another
> > > > device allocated with the same major and minor numbers.  Loop devices
> > > > are the most obvious example, since /dev/loop0 can be reused again and
> > > > again, but the same problem can also happen with device-mapper devices.
> > > > If the major and minor numbers are reused before blkback has attached to
> > > > the device, blkback will pass the wrong device to the domain, with
> > > > obvious security consequences.
> > > > 
> > > > Other programs on Linux have the same problem, and a solution was
> > > > committed upstream in the form of disk sequence numbers.  A disk
> > > > sequence number, or diskseq, is a 64-bit unsigned monotonically
> > > > increasing counter.  The combination of a major and minor number and a
> > > > disk sequence number uniquely identifies a block device for the entire
> > > > uptime of the system.
> > > 
> > > Seems fine to me, this is just an extra check to make sure the block
> > > device opened by blkback is the one that user space intended.  I would
> > > see diskseq as a kind of checksum.
> > 
> > Ideally, diskseq would be the primary means of identifying a device, but
> > that isn’t an option without more substantial changes, sadly.
> > 
> > > > I propose that blkback check for an unsigned 64-bit hexadecimal XenStore
> > > > entry named “diskseq”.  If the entry exists, blkback checks that the
> > > > number stored there matches the disk sequence number of the device.  If
> > > > it does not exist, the check is skipped.  If reading the entry fails for
> > > > any other reason, the entry is malformed, or if the sequence number is
> > > > wrong, blkback refuses to export the device.
> > > > 
> > > > The toolstack changes are more involved for two reasons:
> > > > 
> > > > 1. To ensure that loop devices are not leaked if the toolstack crashes,
> > > >    they must be created with the delete-on-close flag set.  This
> > > >    requires that the toolstack hold the device open until blkback has
> > > >    acquired a handle to it.
> > > 
> > > Does this work with loop devices?  I would expect that you need to
> > > issue a losetup call to detach the device.
> > 
> > That is what the autoclear flag is for.  It will cause the device to be
> > destroyed by the kernel as soon as the last handle to it has been
> > closed.  This is why the toolstack needs to hold a file descriptor to
> > the device.
> 
> What would happen if the backend closes the device (because the
> connection is torn down) and then try to open it again (because the
> guest has triggered a reconnection)?

The reconnect attempt will fail for loop devices, and may fail for
device-mapper devices.  Unless somebody holds the device open until it
has been removed from the guest, reconnect is inherently racy.  That
somebody could be the toolstack, a daemon such as libvirtd, or the
kernel.

> > > Even more, the loop device is created by the block script, but there's
> > > also a window between the block script execution and the toolstack
> > > knowing about the device, which could also allow for a leak?
> > 
> > For this to work, either the toolstack or block script will need to open
> > the file and perform loop(4) ioctls to assign the file descriptor to a
> > loop device.  This cannot be done by a shell script, so I plan on using
> > a C program to perform these tasks.  In Qubes OS, I expect this program
> > to replace the block script entirely, as performance is critical and
> > flexibility less so.  For upstream, I recommend having the block script
> > be a script that calls this C program.
> 
> block scripts can be plain binary executables, so I think it would be
> fine for libxl to just call the executable directly.

Marek had suggested that keeping the block script a script would be
useful for admins, but I am fine with just using a binary.

> > > > 2. For block devices that are opened by path, the toolstack needs to
> > > >    ensure that the device it has opened is actually the device it
> > > >    intended to open.  This requires device-specific verification of the
> > > >    open file descriptor.  This is not needed for regular files, as the
> > > >    LOOP_CONFIGURE ioctl is called on an existing loop device and sets
> > > >    its backing file.
> > > > 
> > > > The first is fairly easy in C.  It can be accomplished by means of a
> > > > XenStore watch on the “status” entry.  Once that watch fires, blkback
> > > > has opened the device, so the toolstack can safely close its file
> > > > descriptor.
> > > 
> > > Does the toolstack really need to close the device?  What harm does it
> > > do to keep the handle open until the domain is destroyed?
> > 
> > This would cause no harm, but it also would not help either, so I do not
> > see any advantages to doing it.
> 
> Well, seems more complex because you need more synchronization between
> blkback and the toolstack in order to detect when blkback has opened
> the device.  If this is not strictly required I would rather avoid it:
> more complexity just leads to more errors.

All of this synchronization can be handled by the block script.

> > > What about disk hotplug?  Which entity will keep the device opened in
> > > that case?  Is xl block-attach going to block until the device
> > > switches to the connected state?
> > 
> > Whichever program opens the file will need to do this.  
> 
> This is not trivial to implement with xl, as `xl block-attach` is a
> short-lived command that just populates the xenstore entries for the
> to be attached device, runs the hotplug script and exits after that.
> I'm not sure we would want to change `xl block-attach` behavior to
> wait until the backend has opened the device.

Anything else is racy, unless there is some program that could keep the
FD open in the background.  Welcome to the ugly world of Linux block
device semantics.  The problem with not using autoclear is that there is
no race-free way I am aware of to clean up the loop device.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]