Re: [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open()

[Kevin Wolf]

Am 23.04.2021 um 11:43 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.04.2021 19:43, Kevin Wolf wrote:
> > Normally, blk_new_open() just shares all permissions. This was fine
> > originally when permissions only protected against uses in the same
> > process because no other part of the code would actually get to access
> > the block nodes opened with blk_new_open(). However, since we use it for
> > file locking now, unsharing permissions becomes desirable.
> > 
> > Add a new BDRV_O_NO_SHARE flag that is used in blk_new_open() to unshare
> > any permissions that can be unshared.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   include/block/block.h |  1 +
> >   block/block-backend.c | 19 +++++++++++++------
> >   2 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index b3f6e509d4..735db05a39 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -101,6 +101,7 @@ typedef struct HDGeometry {
> >       uint32_t cylinders;
> >   } HDGeometry;
> > +#define BDRV_O_NO_SHARE    0x0001 /* don't share permissons */
> >   #define BDRV_O_RDWR        0x0002
> >   #define BDRV_O_RESIZE      0x0004 /* request permission for resizing the 
> > node */
> >   #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save 
> > writes in a snapshot */
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 413af51f3b..61a10ea860 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -398,15 +398,19 @@ BlockBackend *blk_new_open(const char *filename, 
> > const char *reference,
> >       BlockBackend *blk;
> >       BlockDriverState *bs;
> >       uint64_t perm = 0;
> > +    uint64_t shared = BLK_PERM_ALL;
> > -    /* blk_new_open() is mainly used in .bdrv_create implementations and 
> > the
> > -     * tools where sharing isn't a concern because the BDS stays private, 
> > so we
> > -     * just request permission according to the flags.
> > +    /*
> > +     * blk_new_open() is mainly used in .bdrv_create implementations and 
> > the
> > +     * tools where sharing isn't a major concern because the BDS stays 
> > private
> > +     * and the file is generally not supposed to be used by a second 
> > process,
> > +     * so we just request permission according to the flags.
> >        *
> >        * The exceptions are xen_disk and blockdev_init(); in these cases, 
> > the
> >        * caller of blk_new_open() doesn't make use of the permissions, but 
> > they
> >        * shouldn't hurt either. We can still share everything here because 
> > the
> > -     * guest devices will add their own blockers if they can't share. */
> > +     * guest devices will add their own blockers if they can't share.
> > +     */
> 
> Probably old comment "We can still share everything" is a bit in
> conflict with commit message (and new logic).

I read that paragraph as referring to xen_disk and blockdev_init() only,
which still don't pass BDRV_O_NO_SHARE after this series. The comment
explains why they don't have to pass it.

Kevin