1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

Peter Maydell posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181016175236.5840-1-peter.maydell@linaro.org
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
hw/acpi/nvdimm.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Peter Maydell 7 years ago 
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Automatically generated patch, tested with "make check" only.

 hw/acpi/nvdimm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 27eeb6609f5..e53b2cb6819 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
     int size;
 
     read_fit = (NvdimmFuncReadFITIn *)in->arg3;
-    le32_to_cpus(&read_fit->offset);
+    read_fit->offset = le32_to_cpu(read_fit->offset);
 
     fit = fit_buf->fit;
 
@@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
     int size;
 
     get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
-    le32_to_cpus(&get_label_data->offset);
-    le32_to_cpus(&get_label_data->length);
+    get_label_data->offset = le32_to_cpu(get_label_data->offset);
+    get_label_data->length = le32_to_cpu(get_label_data->length);
 
     nvdimm_debug("Read Label Data: offset %#x length %#x.\n",
                  get_label_data->offset, get_label_data->length);
@@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
 
     set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
 
-    le32_to_cpus(&set_label_data->offset);
-    le32_to_cpus(&set_label_data->length);
+    set_label_data->offset = le32_to_cpu(set_label_data->offset);
+    set_label_data->length = le32_to_cpu(set_label_data->length);
 
     nvdimm_debug("Write Label Data: offset %#x length %#x.\n",
                  set_label_data->offset, set_label_data->length);
@@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     in = g_new(NvdimmDsmIn, 1);
     cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
 
-    le32_to_cpus(&in->revision);
-    le32_to_cpus(&in->function);
-    le32_to_cpus(&in->handle);
+    in->revision = le32_to_cpu(in->revision);
+    in->function = le32_to_cpu(in->function);
+    in->handle = le32_to_cpu(in->handle);
 
     nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
                  in->handle, in->function);
-- 
2.19.0
Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Stefan Hajnoczi 7 years ago 
On Tue, Oct 16, 2018 at 06:52:36PM +0100, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Automatically generated patch, tested with "make check" only.
> 
>  hw/acpi/nvdimm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Philippe Mathieu-Daudé 7 years ago 
On 16/10/2018 19:52, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> Automatically generated patch, tested with "make check" only.
> 
>  hw/acpi/nvdimm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 27eeb6609f5..e53b2cb6819 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>      int size;
>  
>      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> -    le32_to_cpus(&read_fit->offset);
> +    read_fit->offset = le32_to_cpu(read_fit->offset);
>  
>      fit = fit_buf->fit;
>  
> @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      int size;
>  
>      get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
> -    le32_to_cpus(&get_label_data->offset);
> -    le32_to_cpus(&get_label_data->length);
> +    get_label_data->offset = le32_to_cpu(get_label_data->offset);
> +    get_label_data->length = le32_to_cpu(get_label_data->length);
>  
>      nvdimm_debug("Read Label Data: offset %#x length %#x.\n",
>                   get_label_data->offset, get_label_data->length);
> @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>  
>      set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
>  
> -    le32_to_cpus(&set_label_data->offset);
> -    le32_to_cpus(&set_label_data->length);
> +    set_label_data->offset = le32_to_cpu(set_label_data->offset);
> +    set_label_data->length = le32_to_cpu(set_label_data->length);
>  
>      nvdimm_debug("Write Label Data: offset %#x length %#x.\n",
>                   set_label_data->offset, set_label_data->length);
> @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      in = g_new(NvdimmDsmIn, 1);
>      cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
>  
> -    le32_to_cpus(&in->revision);
> -    le32_to_cpus(&in->function);
> -    le32_to_cpus(&in->handle);
> +    in->revision = le32_to_cpu(in->revision);
> +    in->function = le32_to_cpu(in->function);
> +    in->handle = le32_to_cpu(in->handle);
>  
>      nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
>                   in->handle, in->function);
>
Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Peter Maydell 7 years ago 
Ping? This patch got reviewed but does not seem to have
made it into anybody's tree.

thanks
-- PMM

On 16 October 2018 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
>
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Automatically generated patch, tested with "make check" only.
>
>  hw/acpi/nvdimm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 27eeb6609f5..e53b2cb6819 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>      int size;
>
>      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> -    le32_to_cpus(&read_fit->offset);
> +    read_fit->offset = le32_to_cpu(read_fit->offset);
>
>      fit = fit_buf->fit;
>
> @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>      int size;
>
>      get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
> -    le32_to_cpus(&get_label_data->offset);
> -    le32_to_cpus(&get_label_data->length);
> +    get_label_data->offset = le32_to_cpu(get_label_data->offset);
> +    get_label_data->length = le32_to_cpu(get_label_data->length);
>
>      nvdimm_debug("Read Label Data: offset %#x length %#x.\n",
>                   get_label_data->offset, get_label_data->length);
> @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>
>      set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
>
> -    le32_to_cpus(&set_label_data->offset);
> -    le32_to_cpus(&set_label_data->length);
> +    set_label_data->offset = le32_to_cpu(set_label_data->offset);
> +    set_label_data->length = le32_to_cpu(set_label_data->length);
>
>      nvdimm_debug("Write Label Data: offset %#x length %#x.\n",
>                   set_label_data->offset, set_label_data->length);
> @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>      in = g_new(NvdimmDsmIn, 1);
>      cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
>
> -    le32_to_cpus(&in->revision);
> -    le32_to_cpus(&in->function);
> -    le32_to_cpus(&in->handle);
> +    in->revision = le32_to_cpu(in->revision);
> +    in->function = le32_to_cpu(in->function);
> +    in->handle = le32_to_cpu(in->handle);
>
>      nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
>                   in->handle, in->function);
> --
Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Peter Maydell 6 years, 11 months ago 
Since nobody responded to my ping of a week ago I propose to just
apply this to master...

thanks
-- PMM

On 5 November 2018 at 14:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping? This patch got reviewed but does not seem to have
> made it into anybody's tree.
>
> thanks
> -- PMM
>
> On 16 October 2018 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Taking the address of a field in a packed struct is a bad idea, because
>> it might not be actually aligned enough for that pointer type (and
>> thus cause a crash on dereference on some host architectures). Newer
>> versions of clang warn about this. Avoid the bug by not using the
>> "modify in place" byte swapping functions.
>>
>> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Automatically generated patch, tested with "make check" only.
>>
>>  hw/acpi/nvdimm.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 27eeb6609f5..e53b2cb6819 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
>>      int size;
>>
>>      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
>> -    le32_to_cpus(&read_fit->offset);
>> +    read_fit->offset = le32_to_cpu(read_fit->offset);
>>
>>      fit = fit_buf->fit;
>>
>> @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>>      int size;
>>
>>      get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
>> -    le32_to_cpus(&get_label_data->offset);
>> -    le32_to_cpus(&get_label_data->length);
>> +    get_label_data->offset = le32_to_cpu(get_label_data->offset);
>> +    get_label_data->length = le32_to_cpu(get_label_data->length);
>>
>>      nvdimm_debug("Read Label Data: offset %#x length %#x.\n",
>>                   get_label_data->offset, get_label_data->length);
>> @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
>>
>>      set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
>>
>> -    le32_to_cpus(&set_label_data->offset);
>> -    le32_to_cpus(&set_label_data->length);
>> +    set_label_data->offset = le32_to_cpu(set_label_data->offset);
>> +    set_label_data->length = le32_to_cpu(set_label_data->length);
>>
>>      nvdimm_debug("Write Label Data: offset %#x length %#x.\n",
>>                   set_label_data->offset, set_label_data->length);
>> @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>      in = g_new(NvdimmDsmIn, 1);
>>      cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
>>
>> -    le32_to_cpus(&in->revision);
>> -    le32_to_cpus(&in->function);
>> -    le32_to_cpus(&in->handle);
>> +    in->revision = le32_to_cpu(in->revision);
>> +    in->function = le32_to_cpu(in->function);
>> +    in->handle = le32_to_cpu(in->handle);
>>
>>      nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
>>                   in->handle, in->function);
>> --
Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Michael S. Tsirkin 6 years, 11 months ago 
On Mon, Nov 12, 2018 at 02:42:16PM +0000, Peter Maydell wrote:
> Since nobody responded to my ping of a week ago I propose to just
> apply this to master...
> 
> thanks
> -- PMM

Sorry. My LPC talk proposal suddenly got accepted and
I was scrambling to get ready.

Please feel free to apply this for now:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Generally I think we need to rethink our approach to endian-ness. I
think we want to tag fields with specific endian-ness and use static
checkers to verify it. That is how Linux does it.
In particular this will mean no swapping bytes in place.

But that's a subject for another day.


> On 5 November 2018 at 14:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Ping? This patch got reviewed but does not seem to have
> > made it into anybody's tree.
> >
> > thanks
> > -- PMM
> >
> > On 16 October 2018 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> Taking the address of a field in a packed struct is a bad idea, because
> >> it might not be actually aligned enough for that pointer type (and
> >> thus cause a crash on dereference on some host architectures). Newer
> >> versions of clang warn about this. Avoid the bug by not using the
> >> "modify in place" byte swapping functions.
> >>
> >> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >> Automatically generated patch, tested with "make check" only.
> >>
> >>  hw/acpi/nvdimm.c | 16 ++++++++--------
> >>  1 file changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >> index 27eeb6609f5..e53b2cb6819 100644
> >> --- a/hw/acpi/nvdimm.c
> >> +++ b/hw/acpi/nvdimm.c
> >> @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in,
> >>      int size;
> >>
> >>      read_fit = (NvdimmFuncReadFITIn *)in->arg3;
> >> -    le32_to_cpus(&read_fit->offset);
> >> +    read_fit->offset = le32_to_cpu(read_fit->offset);
> >>
> >>      fit = fit_buf->fit;
> >>
> >> @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> >>      int size;
> >>
> >>      get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
> >> -    le32_to_cpus(&get_label_data->offset);
> >> -    le32_to_cpus(&get_label_data->length);
> >> +    get_label_data->offset = le32_to_cpu(get_label_data->offset);
> >> +    get_label_data->length = le32_to_cpu(get_label_data->length);
> >>
> >>      nvdimm_debug("Read Label Data: offset %#x length %#x.\n",
> >>                   get_label_data->offset, get_label_data->length);
> >> @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> >>
> >>      set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
> >>
> >> -    le32_to_cpus(&set_label_data->offset);
> >> -    le32_to_cpus(&set_label_data->length);
> >> +    set_label_data->offset = le32_to_cpu(set_label_data->offset);
> >> +    set_label_data->length = le32_to_cpu(set_label_data->length);
> >>
> >>      nvdimm_debug("Write Label Data: offset %#x length %#x.\n",
> >>                   set_label_data->offset, set_label_data->length);
> >> @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> >>      in = g_new(NvdimmDsmIn, 1);
> >>      cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
> >>
> >> -    le32_to_cpus(&in->revision);
> >> -    le32_to_cpus(&in->function);
> >> -    le32_to_cpus(&in->handle);
> >> +    in->revision = le32_to_cpu(in->revision);
> >> +    in->function = le32_to_cpu(in->function);
> >> +    in->handle = le32_to_cpu(in->handle);
> >>
> >>      nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
> >>                   in->handle, in->function);
> >> --
Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs
Posted by Peter Maydell 6 years, 11 months ago 
On 12 November 2018 at 15:01, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Nov 12, 2018 at 02:42:16PM +0000, Peter Maydell wrote:
>> Since nobody responded to my ping of a week ago I propose to just
>> apply this to master...
>>
>> thanks
>> -- PMM
>
> Sorry. My LPC talk proposal suddenly got accepted and
> I was scrambling to get ready.
>
> Please feel free to apply this for now:
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Thanks.

> Generally I think we need to rethink our approach to endian-ness. I
> think we want to tag fields with specific endian-ness and use static
> checkers to verify it. That is how Linux does it.
> In particular this will mean no swapping bytes in place.

Yeah, I agree that swapping-in-place is definitely not ideal.
Personally I prefer to avoid having structs that try to match
in-memory binary layouts entirely (ie no packed structs).

> But that's a subject for another day.

Indeed.

-- PMM

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.