Re: [PATCH v2 9/9] memory: assert MemoryRegionOps callbacks are defined

From: Li Qiang <liq3ea@gmail.com>
To: P J P <ppandit@redhat.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Prasad J Pandit" <pjp@fedoraproject.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Lei Sun" <slei.casper@gmail.com>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 9/9] memory: assert MemoryRegionOps callbacks are defined
Date: Mon, 29 Jun 2020 19:28:44 +0800	[thread overview]
Message-ID: <CAKXe6SJHVSaEB1jfYwsiw1WjR7S7R61jkRZ4cHPXg_jfkz1FMQ@mail.gmail.com> (raw)
In-Reply-To: <20200624185523.762240-10-ppandit@redhat.com>

P J P <ppandit@redhat.com> 于2020年6月25日周四 上午3:00写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When registering a MemoryRegionOps object, assert that its
> read/write callback methods are defined. This avoids potential
> guest crash via a NULL pointer dereference.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  memory.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> Update v2: assert read/write_with_attrs methods
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05717.html
>
> diff --git a/memory.c b/memory.c
> index 2f15a4b250..3d635a1bca 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1496,7 +1496,13 @@ void memory_region_init_io(MemoryRegion *mr,
>                             uint64_t size)
>  {
>      memory_region_init(mr, owner, name, size);
> -    mr->ops = ops ? ops : &unassigned_mem_ops;
> +    if (ops) {
> +        assert(ops->read || ops->read_with_attrs);
> +        assert(ops->write || ops->write_with_attrs);
> +        mr->ops = ops;
> +    } else {
> +        mr->ops = &unassigned_mem_ops;
> +    }
>      mr->opaque = opaque;
>      mr->terminates = true;
>  }
> @@ -1674,6 +1680,8 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>  {
>      Error *err = NULL;
>      assert(ops);
> +    assert(ops->read || ops->read_with_attrs);

Though here is not 100% right as the ROM device can has no read callback.
However the device can change between ROMD mode or MMIO mode.
So I think it's ok the developer can provide a null read callback even if they
don't need it.

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> +    assert(ops->write || ops->write_with_attrs);
>      memory_region_init(mr, owner, name, size);
>      mr->ops = ops;
>      mr->opaque = opaque;
> --
> 2.26.2
>