Re: [PATCH v2] block: Improve backing file validation

[address@hidden]

On 12/05/2021 23.10, Kevin Wolf wrote:
> Am 11.05.2021 um 10:35 hat Daniel P. Berrangé geschrieben:
>> On Tue, May 11, 2021 at 01:55:18PM +0800, Li Zhijian wrote:
>>>   void bdrv_img_create(const char *filename, const char *fmt,
>>>                        const char *base_filename, const char *base_fmt,
>>>                        char *options, uint64_t img_size, int flags, bool 
>>> quiet,
>>> @@ -6507,13 +6538,7 @@ void bdrv_img_create(const char *filename, const 
>>> char *fmt,
>>>   
>>>       backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>>>       if (backing_file) {
>>> -        if (!strcmp(filename, backing_file)) {
>>> -            error_setg(errp, "Error: Trying to create an image with the "
>>> -                             "same filename as the backing file");
>>> -            goto out;
>>> -        }
>>> -        if (backing_file[0] == '\0') {
>>> -            error_setg(errp, "Expected backing file name, got empty 
>>> string");
>>> +        if (!validate_backing_file(filename, backing_file, errp)) {
>>>               goto out;
>>>           }
>>>       }
>> Thinking about this again, this seems to be quite high in the generic block
>> layer code. As such I don't think we can assume that the backing file here
>> is actually a plain file on disk. IIUC the backing file could still be any
>> of the block drivers. Only once we get down into the protocol specific
>> drivers can be validate the type of backend.
> Yes, you definitely can't assume that filename is really a local file
> name here. It could be any other protocol supported by QEMU, or even use
> the json: pseudo-protocol.
>
>> I'm not sure what the right way to deal with that is, so perhaps Kevin or
>> Max can make a suggestion.
> Can we just keep the backing file open with write permissions unshared
> so that locking will fail for the new image?

*

Not sure if i have understood.  In my understanding, open(2) cannot support 
'open with write permissions unshared',

it has to cooperate with flock(2)/fcntl(2) to accomplish writing exclusively.


Currently, qemu block also doesn't support 'open with write permissions 
unshared', but i found something:

#define BDRV_O_NO_SHARE0x0001 /* don't share permissions */


And I have tried below changes and expect the block fails to write the image.

@@ -6563,7 +6563,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,

assert(full_backing);

/* backing files always opened read-only */

- back_flags = flags;

+ back_flags = flags | BDRV_O_NO_SHARE;

back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);

backing_options = qdict_new();


But in practice, the image is created successfully.

So do you mean we should implement a new flag like 'BDRV_O_NO_SHARE_WR' to 
handle this

*

Thanks
Zhijian

>   Or would that error
> condition be detected too late so that the image would already be
> truncated?

>
> Kevin
>
>
>