Re: [PATCH] block: Add option to use driver whitelist even in tools

[Kevin Wolf <kwolf@redhat.com>]

Am 09.07.2021 um 19:45 hat Eric Blake geschrieben:
> On Fri, Jul 09, 2021 at 06:41:41PM +0200, Kevin Wolf wrote:
> > Currently, the block driver whitelists are only applied for the system
> > emulator. All other binaries still give unrestricted access to all block
> > drivers. There are use cases where this made sense because the main
> > concern was avoiding customers running VMs on less optimised block
> > drivers and getting bad performance. Allowing the same image format e.g.
> > as a target for 'qemu-img convert' is not a problem then.
> > 
> > However, if the concern is the supportability of the driver in general,
> > either in full or when used read-write, not applying the list driver
> > whitelist in tools doesn't help - especially since qemu-nbd and
> > qemu-storage-daemon now give access to more or less the same operations
> > in block drivers as running a system emulator.
> > 
> > In order to address this, introduce a new configure option that enforces
> > the driver whitelist in all binaries.
> 
> Is it feasible that someone would want two separate lists: one for
> qemu (which runs run efficiently) and another for tools (which ones do
> we support at all)?  As written, your patch offers no chance to
> distinguish between the two.

Possibly. However, supporting a second list would require a much larger
code change than this patch, so I'd say this is a problem we should only
solve when someone actually has it.

> Also, is now a good time to join the bandwagon on picking a more
> descriptive name (such as 'allow-list') for this terminology?

I don't have an opinion on the time, but I do have an opinion on using a
separate email thread for it. :-)

Initially I tried to find a way not to use "whitelist" in the new option
name, but that only made things inconsistent and confusing, and renaming
the existing options is definitely out of scope for this patch.

> > +++ b/block.c
> > @@ -6162,6 +6162,9 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
> >  
> >  void bdrv_init(void)
> >  {
> > +#ifdef CONFIG_BDRV_WHITELIST_TOOLS
> > +    use_bdrv_whitelist = 1;
> 
> Pre-existing, but this variable seems like a natural candidate to be
> bool instead of int.

Yes, I guess we could have a cleanup patch there.

Kevin