Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Philippe Mathieu-Daudé]

On 8/4/20 3:47 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 04, 2020 at 02:26:22PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Stefan,
>>>
>>> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
>>>> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
>>>> creating a memory region from a file. This functionality is needed since
>>>> the underlying host file may not allow writing.
>>>>
>>>> Add a bool readonly argument to memory_region_init_ram_from_file() and
>>>> the APIs it calls.
>>>>
>>>> Extend memory_region_init_ram_from_file() rather than introducing a
>>>> memory_region_init_rom_from_file() API so that callers can easily make a
>>>> choice between read/write and read-only at runtime without calling
>>>> different APIs.
>>>
>>> What happens if we call:
>>>
>>>  memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
>>>  memory_region_set_readonly(mr, false);
>>
>> In case my error is not obvious, I meant:
>>
>>    memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
>>    memory_region_set_readonly(mr, false);
> 
> Since the mmap was made using PROT_READ any store instructions to the
> memory will fault.
> 
> Is there some scenario where memory_region_set_readonly() is called? I
> can't find one.

Not in the current code base, but I was wondering about the API abuses.

I see in the next patch the property is protected:

    if (host_memory_backend_mr_inited(backend)) {
        error_setg(errp, "cannot change property 'readonly' of %s.",
                   object_get_typename(o));
        return;
    }

By using memory_region_set_readonly() you bypass this protection.

Maybe not something to worry.

Anyway for the patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Stefan
>