Re: [Qemu-devel] [PATCH] memory: check write/read_with_attrs in memory dispatch

[Li Qiang]

Ping...

It makes sense as when we use 'memory_region_read_accessor' we check
mr->ops->read.
but when we use 'memory_region_read_with_attrs_accessor', we doesn't check
this.

Thanks,
Li Qiang

Li Qiang  于2018年11月13日周二 上午9:42写道：

> This can avoid the NULL-deref if the rm doesn't has a
> read/write nor write/read_with_attrs callback.
>
> Signed-off-by: Li Qiang 
> ---
>  memory.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index d14c6dec1d..3baf5857b9 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1377,13 +1377,15 @@ static MemTxResult
> memory_region_dispatch_read1(MemoryRegion *mr,
>   mr->ops->impl.max_access_size,
>   memory_region_read_accessor,
>   mr, attrs);
> -} else {
> +} else if (mr->ops->read_with_attrs) {
>  return access_with_adjusted_size(addr, pval, size,
>   mr->ops->impl.min_access_size,
>   mr->ops->impl.max_access_size,
>
> memory_region_read_with_attrs_accessor,
>   mr, attrs);
>  }
> +
> +return MEMTX_DECODE_ERROR;
>  }
>
>  MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
> @@ -1454,7 +1456,7 @@ MemTxResult
> memory_region_dispatch_write(MemoryRegion *mr,
>   mr->ops->impl.max_access_size,
>   memory_region_write_accessor, mr,
>   attrs);
> -} else {
> +} else if (mr->ops->write_with_attrs) {
>  return
>  access_with_adjusted_size(addr, , size,
>mr->ops->impl.min_access_size,
> @@ -1462,6 +1464,8 @@ MemTxResult
> memory_region_dispatch_write(MemoryRegion *mr,
>
>  memory_region_write_with_attrs_accessor,
>mr, attrs);
>  }
> +
> +return MEMTX_DECODE_ERROR;
>  }
>
>  void memory_region_init_io(MemoryRegion *mr,
> --
> 2.11.0
>
>

Re: [Qemu-devel] [PATCH] memory: check write/read_with_attrs in memory dispatch

[Li Qiang]

Peter Maydell  于2018年11月13日周二 下午5:49写道：

> On 13 November 2018 at 01:42, Li Qiang  wrote:
> > This can avoid the NULL-deref if the rm doesn't has a
> > read/write nor write/read_with_attrs callback.
> >
> > Signed-off-by: Li Qiang 
> > ---
> >  memory.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
>
> Alternative approach -- assert that every MemoryRegionOps has
> pointers to callbacks in it, when it is registered in
> memory_region_init_io()
>

Actually I have considered this approach, but I rember Paolo remind me that
some
of this MR without read callback's is valid because the read can never be
called, such as
'notdirty_mem_ops'. So I choose add a check here.



> and memory_region_init_rom_device_nomigrate().
>
> I don't have a strong opinion on which is better, but I guess
> I slightly favour requiring devices to be specific about what
> their read/write behaviour is.
>
> Do we have many devices that legitimately only want to implement
> one of read and write, not both ?


AFAICS, the device has just one read or write is not uncommon.
But nearly all of them implement a nop function does nothing.
So there just very little lack of the read or write function.

Thanks,
Li Qiang


>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] memory: check write/read_with_attrs in memory dispatch

[Peter Maydell]

On 13 November 2018 at 01:42, Li Qiang  wrote:
> This can avoid the NULL-deref if the rm doesn't has a
> read/write nor write/read_with_attrs callback.
>
> Signed-off-by: Li Qiang 
> ---
>  memory.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Alternative approach -- assert that every MemoryRegionOps has
pointers to callbacks in it, when it is registered in memory_region_init_io()
and memory_region_init_rom_device_nomigrate().

I don't have a strong opinion on which is better, but I guess
I slightly favour requiring devices to be specific about what
their read/write behaviour is.

Do we have many devices that legitimately only want to implement
one of read and write, not both ?

thanks
-- PMM

[Qemu-devel] [PATCH] memory: check write/read_with_attrs in memory dispatch

[Li Qiang]

This can avoid the NULL-deref if the rm doesn't has a
read/write nor write/read_with_attrs callback.

Signed-off-by: Li Qiang 
---
 memory.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index d14c6dec1d..3baf5857b9 100644
--- a/memory.c
+++ b/memory.c
@@ -1377,13 +1377,15 @@ static MemTxResult 
memory_region_dispatch_read1(MemoryRegion *mr,
  mr->ops->impl.max_access_size,
  memory_region_read_accessor,
  mr, attrs);
-} else {
+} else if (mr->ops->read_with_attrs) {
 return access_with_adjusted_size(addr, pval, size,
  mr->ops->impl.min_access_size,
  mr->ops->impl.max_access_size,
  
memory_region_read_with_attrs_accessor,
  mr, attrs);
 }
+
+return MEMTX_DECODE_ERROR;
 }
 
 MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
@@ -1454,7 +1456,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
  mr->ops->impl.max_access_size,
  memory_region_write_accessor, mr,
  attrs);
-} else {
+} else if (mr->ops->write_with_attrs) {
 return
 access_with_adjusted_size(addr, , size,
   mr->ops->impl.min_access_size,
@@ -1462,6 +1464,8 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
   memory_region_write_with_attrs_accessor,
   mr, attrs);
 }
+
+return MEMTX_DECODE_ERROR;
 }
 
 void memory_region_init_io(MemoryRegion *mr,
-- 
2.11.0