[PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Guenter Roeck]

sparc64 images fail to compile with gcc 11.x, reporting the following
errors.

arch/sparc/kernel/mdesc.c:647:22: error:
	'strcmp' reading 1 or more bytes from a region of size 0
arch/sparc/kernel/mdesc.c:692:22: error:
	'strcmp' reading 1 or more bytes from a region of size 0
arch/sparc/kernel/mdesc.c:719:21:
	error: 'strcmp' reading 1 or more bytes from a region of size 0

The underlying problem is that node_block() returns a pointer beyond
the end of struct mdesc_hdr. gcc 11.x detects that and reports the error.
Adding an additional zero-length field to struct mdesc_hdr and pointing
to that field fixes the problem.

Cc: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
My apologies if a similar patch was submitted already; I was unable to find it.
I did find the following patch:
    https://git.busybox.net/buildroot/commit/?id=6e1106b4a9aee25d1556310d5cd1cb6dde2e6e3f
but I failed to find it in patchwork or on lore.kernel.org, and it
seems to be more expensive than the solution suggested here.

 arch/sparc/kernel/mdesc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index 8e645ddac58e..c67bdcc23727 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -39,6 +39,7 @@ struct mdesc_hdr {
 	u32	node_sz; /* node block size */
 	u32	name_sz; /* name block size */
 	u32	data_sz; /* data block size */
+	char	data[0];
 } __attribute__((aligned(16)));
 
 struct mdesc_elem {
@@ -612,7 +613,7 @@ EXPORT_SYMBOL(mdesc_get_node_info);
 
 static struct mdesc_elem *node_block(struct mdesc_hdr *mdesc)
 {
-	return (struct mdesc_elem *) (mdesc + 1);
+	return (struct mdesc_elem *) (mdesc->data);
 }
 
 static void *name_block(struct mdesc_hdr *mdesc)
-- 
2.33.0

Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Guenter Roeck]

On 9/13/21 11:02 AM, Sam Ravnborg wrote:
> Hi Guenter,
> 
> On Mon, Sep 13, 2021 at 09:37:12AM -0700, Guenter Roeck wrote:
>> sparc64 images fail to compile with gcc 11.x, reporting the following
>> errors.
>>
>> arch/sparc/kernel/mdesc.c:647:22: error:
>> 	'strcmp' reading 1 or more bytes from a region of size 0
>> arch/sparc/kernel/mdesc.c:692:22: error:
>> 	'strcmp' reading 1 or more bytes from a region of size 0
>> arch/sparc/kernel/mdesc.c:719:21:
>> 	error: 'strcmp' reading 1 or more bytes from a region of size 0
>>
>> The underlying problem is that node_block() returns a pointer beyond
>> the end of struct mdesc_hdr. gcc 11.x detects that and reports the error.
>> Adding an additional zero-length field to struct mdesc_hdr and pointing
>> to that field fixes the problem.
>>
>> Cc: Arnd Bergmann <arnd@kernel.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> My apologies if a similar patch was submitted already; I was unable to find it.
>> I did find the following patch:
>>      https://git.busybox.net/buildroot/commit/?id=6e1106b4a9aee25d1556310d5cd1cb6dde2e6e3f
>> but I failed to find it in patchwork or on lore.kernel.org, and it
>> seems to be more expensive than the solution suggested here.
>>
>>   arch/sparc/kernel/mdesc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
>> index 8e645ddac58e..c67bdcc23727 100644
>> --- a/arch/sparc/kernel/mdesc.c
>> +++ b/arch/sparc/kernel/mdesc.c
>> @@ -39,6 +39,7 @@ struct mdesc_hdr {
>>   	u32	node_sz; /* node block size */
>>   	u32	name_sz; /* name block size */
>>   	u32	data_sz; /* data block size */
>> +	char	data[0];
>>   } __attribute__((aligned(16)));
> 
> I do not think this will works.
> See following comment:
>   * mdesc_hdr and mdesc_elem describe the layout of the data structure
>   * we get from the Hypervisor.
> 
> With the above change you increased the size from 16 to 32 bytes,
> and any code using sizeof(struct mdesc_hdr) will now point too far in
> memory for the second and subsequent entries.
> 
> I did not take any closer look, but this was from a quick analysis.
> 

Sorry, I didn't realize that a field of size 0 increases the structure size
on sparc. I had checked the size of the old and the new structure with gcc
on x86_64 and didn't see a field size increase.

Guenter

---
Test code I had used:

#include <stddef.h>
#include <stdio.h>

typedef unsigned int u32;

struct mdesc_hdr {
         u32     version; /* Transport version */
         u32     node_sz; /* node block size */
         u32     name_sz; /* name block size */
         u32     data_sz; /* data block size */
} __attribute__((aligned(16)));

struct mdesc_hdr2 {
         u32     version; /* Transport version */
         u32     node_sz; /* node block size */
         u32     name_sz; /* name block size */
         u32     data_sz; /* data block size */
         char    data[0];
} __attribute__((aligned(16)));

int main()
{
	printf("%ld %ld\n", sizeof(struct mdesc_hdr), sizeof(struct mdesc_hdr2));

	return 0;
}

Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Anatoly Pugachev]

On Tue, Sep 14, 2021 at 3:54 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/13/21 11:02 AM, Sam Ravnborg wrote:
> > Hi Guenter,
> >
> > On Mon, Sep 13, 2021 at 09:37:12AM -0700, Guenter Roeck wrote:
> >> sparc64 images fail to compile with gcc 11.x, reporting the following
> >> errors.
> >>
> >> arch/sparc/kernel/mdesc.c:647:22: error:
> >>      'strcmp' reading 1 or more bytes from a region of size 0
> >> arch/sparc/kernel/mdesc.c:692:22: error:
> >>      'strcmp' reading 1 or more bytes from a region of size 0
> >> arch/sparc/kernel/mdesc.c:719:21:
> >>      error: 'strcmp' reading 1 or more bytes from a region of size 0
> >>
> >> The underlying problem is that node_block() returns a pointer beyond
> >> the end of struct mdesc_hdr. gcc 11.x detects that and reports the error.
> >> Adding an additional zero-length field to struct mdesc_hdr and pointing
> >> to that field fixes the problem.
> >>
> >> Cc: Arnd Bergmann <arnd@kernel.org>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> My apologies if a similar patch was submitted already; I was unable to find it.
> >> I did find the following patch:
> >>      https://git.busybox.net/buildroot/commit/?id=6e1106b4a9aee25d1556310d5cd1cb6dde2e6e3f
> >> but I failed to find it in patchwork or on lore.kernel.org, and it
> >> seems to be more expensive than the solution suggested here.
> >>
> >>   arch/sparc/kernel/mdesc.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> >> index 8e645ddac58e..c67bdcc23727 100644
> >> --- a/arch/sparc/kernel/mdesc.c
> >> +++ b/arch/sparc/kernel/mdesc.c
> >> @@ -39,6 +39,7 @@ struct mdesc_hdr {
> >>      u32     node_sz; /* node block size */
> >>      u32     name_sz; /* name block size */
> >>      u32     data_sz; /* data block size */
> >> +    char    data[0];
> >>   } __attribute__((aligned(16)));
> >
> > I do not think this will works.
> > See following comment:
> >   * mdesc_hdr and mdesc_elem describe the layout of the data structure
> >   * we get from the Hypervisor.
> >
> > With the above change you increased the size from 16 to 32 bytes,
> > and any code using sizeof(struct mdesc_hdr) will now point too far in
> > memory for the second and subsequent entries.
> >
> > I did not take any closer look, but this was from a quick analysis.
> >
>
> Sorry, I didn't realize that a field of size 0 increases the structure size
> on sparc. I had checked the size of the old and the new structure with gcc
> on x86_64 and didn't see a field size increase.
>
> Guenter
>
> ---
> Test code I had used:
>
> #include <stddef.h>
> #include <stdio.h>
>
> typedef unsigned int u32;
>
> struct mdesc_hdr {
>          u32     version; /* Transport version */
>          u32     node_sz; /* node block size */
>          u32     name_sz; /* name block size */
>          u32     data_sz; /* data block size */
> } __attribute__((aligned(16)));
>
> struct mdesc_hdr2 {
>          u32     version; /* Transport version */
>          u32     node_sz; /* node block size */
>          u32     name_sz; /* name block size */
>          u32     data_sz; /* data block size */
>          char    data[0];
> } __attribute__((aligned(16)));
>
> int main()
> {
>         printf("%ld %ld\n", sizeof(struct mdesc_hdr), sizeof(struct mdesc_hdr2));
>
>         return 0;
> }

used the code above on my sparc64 installation:

copy-paste code to 123.c file

$ gcc-11 123.c
$ ./a.out
16 16
$ uname -a
Linux ttip 5.15.0-rc1 #273 SMP Mon Sep 13 12:47:14 MSK 2021 sparc64 GNU/Linux

RE: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[David Laight]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: 13 September 2021 19:53
> To: Sam Ravnborg <sam@ravnborg.org>
> Cc: David S . Miller <davem@davemloft.net>; sparclinux@vger.kernel.org; linux-kernel@vger.kernel.org;
> Arnd Bergmann <arnd@kernel.org>
> Subject: Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x
> 
> On 9/13/21 11:02 AM, Sam Ravnborg wrote:
> > Hi Guenter,
> >
> > On Mon, Sep 13, 2021 at 09:37:12AM -0700, Guenter Roeck wrote:
> >> sparc64 images fail to compile with gcc 11.x, reporting the following
> >> errors.
> >>
> >> arch/sparc/kernel/mdesc.c:647:22: error:
> >> 	'strcmp' reading 1 or more bytes from a region of size 0
> >> arch/sparc/kernel/mdesc.c:692:22: error:
> >> 	'strcmp' reading 1 or more bytes from a region of size 0
> >> arch/sparc/kernel/mdesc.c:719:21:
> >> 	error: 'strcmp' reading 1 or more bytes from a region of size 0
> >>
> >> The underlying problem is that node_block() returns a pointer beyond
> >> the end of struct mdesc_hdr. gcc 11.x detects that and reports the error.
> >> Adding an additional zero-length field to struct mdesc_hdr and pointing
> >> to that field fixes the problem.
> >>
> >> Cc: Arnd Bergmann <arnd@kernel.org>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> My apologies if a similar patch was submitted already; I was unable to find it.
> >> I did find the following patch:
> >>      https://git.busybox.net/buildroot/commit/?id=6e1106b4a9aee25d1556310d5cd1cb6dde2e6e3f
> >> but I failed to find it in patchwork or on lore.kernel.org, and it
> >> seems to be more expensive than the solution suggested here.
> >>
> >>   arch/sparc/kernel/mdesc.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> >> index 8e645ddac58e..c67bdcc23727 100644
> >> --- a/arch/sparc/kernel/mdesc.c
> >> +++ b/arch/sparc/kernel/mdesc.c
> >> @@ -39,6 +39,7 @@ struct mdesc_hdr {
> >>   	u32	node_sz; /* node block size */
> >>   	u32	name_sz; /* name block size */
> >>   	u32	data_sz; /* data block size */
> >> +	char	data[0];
> >>   } __attribute__((aligned(16)));
> >
> > I do not think this will works.
> > See following comment:
> >   * mdesc_hdr and mdesc_elem describe the layout of the data structure
> >   * we get from the Hypervisor.
> >
> > With the above change you increased the size from 16 to 32 bytes,
> > and any code using sizeof(struct mdesc_hdr) will now point too far in
> > memory for the second and subsequent entries.
> >
> > I did not take any closer look, but this was from a quick analysis.
> >
> 
> Sorry, I didn't realize that a field of size 0 increases the structure size
> on sparc. I had checked the size of the old and the new structure with gcc
> on x86_64 and didn't see a field size increase.

clang output doesn't change:

https://godbolt.org/z/bTeeq19j1

gcc ought to generate the same size.

It ought to be 'char data[];' though.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Guenter Roeck]

On 9/14/21 7:17 AM, David Laight wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: 13 September 2021 19:53
>> To: Sam Ravnborg <sam@ravnborg.org>
>> Cc: David S . Miller <davem@davemloft.net>; sparclinux@vger.kernel.org; linux-kernel@vger.kernel.org;
>> Arnd Bergmann <arnd@kernel.org>
>> Subject: Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x
>>
>> On 9/13/21 11:02 AM, Sam Ravnborg wrote:
>>> Hi Guenter,
>>>
>>> On Mon, Sep 13, 2021 at 09:37:12AM -0700, Guenter Roeck wrote:
>>>> sparc64 images fail to compile with gcc 11.x, reporting the following
>>>> errors.
>>>>
>>>> arch/sparc/kernel/mdesc.c:647:22: error:
>>>> 	'strcmp' reading 1 or more bytes from a region of size 0
>>>> arch/sparc/kernel/mdesc.c:692:22: error:
>>>> 	'strcmp' reading 1 or more bytes from a region of size 0
>>>> arch/sparc/kernel/mdesc.c:719:21:
>>>> 	error: 'strcmp' reading 1 or more bytes from a region of size 0
>>>>
>>>> The underlying problem is that node_block() returns a pointer beyond
>>>> the end of struct mdesc_hdr. gcc 11.x detects that and reports the error.
>>>> Adding an additional zero-length field to struct mdesc_hdr and pointing
>>>> to that field fixes the problem.
>>>>
>>>> Cc: Arnd Bergmann <arnd@kernel.org>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> My apologies if a similar patch was submitted already; I was unable to find it.
>>>> I did find the following patch:
>>>>       https://git.busybox.net/buildroot/commit/?id=6e1106b4a9aee25d1556310d5cd1cb6dde2e6e3f
>>>> but I failed to find it in patchwork or on lore.kernel.org, and it
>>>> seems to be more expensive than the solution suggested here.
>>>>
>>>>    arch/sparc/kernel/mdesc.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
>>>> index 8e645ddac58e..c67bdcc23727 100644
>>>> --- a/arch/sparc/kernel/mdesc.c
>>>> +++ b/arch/sparc/kernel/mdesc.c
>>>> @@ -39,6 +39,7 @@ struct mdesc_hdr {
>>>>    	u32	node_sz; /* node block size */
>>>>    	u32	name_sz; /* name block size */
>>>>    	u32	data_sz; /* data block size */
>>>> +	char	data[0];
>>>>    } __attribute__((aligned(16)));
>>>
>>> I do not think this will works.
>>> See following comment:
>>>    * mdesc_hdr and mdesc_elem describe the layout of the data structure
>>>    * we get from the Hypervisor.
>>>
>>> With the above change you increased the size from 16 to 32 bytes,
>>> and any code using sizeof(struct mdesc_hdr) will now point too far in
>>> memory for the second and subsequent entries.
>>>
>>> I did not take any closer look, but this was from a quick analysis.
>>>
>>
>> Sorry, I didn't realize that a field of size 0 increases the structure size
>> on sparc. I had checked the size of the old and the new structure with gcc
>> on x86_64 and didn't see a field size increase.
> 
> clang output doesn't change:
> 
> https://godbolt.org/z/bTeeq19j1
> 
> gcc ought to generate the same size.
> 
> It ought to be 'char data[];' though.
> 

I am never sure if [] or [0] is "correct". Anyway, is there agreement that this
is an acceptable solution ? I'll be happy to resend if that is the case.

Guenter

Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Arnd Bergmann]

On Tue, Sep 14, 2021 at 4:24 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 9/14/21 7:17 AM, David Laight wrote:
> >> Sorry, I didn't realize that a field of size 0 increases the structure size
> >> on sparc. I had checked the size of the old and the new structure with gcc
> >> on x86_64 and didn't see a field size increase.
> >
> > clang output doesn't change:
> >
> > https://godbolt.org/z/bTeeq19j1
> >
> > gcc ought to generate the same size.
> >
> > It ought to be 'char data[];' though.
> >
>
> I am never sure if [] or [0] is "correct". Anyway, is there agreement that this
> is an acceptable solution ? I'll be happy to resend if that is the case.

Yes, looks good to me, in the [] version. I think the [0] version can be
interpreted as a zero-length array that may not be accessed, while the
[] flexible array syntax clearly means that extra data follows, and it's
part of the C standard now, while [0] is a gcc extension.

        Arnd

RE: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[David Laight]

From: Arnd Bergmann
> Sent: 14 September 2021 15:54
> 
> On Tue, Sep 14, 2021 at 4:24 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > On 9/14/21 7:17 AM, David Laight wrote:
> > >> Sorry, I didn't realize that a field of size 0 increases the structure size
> > >> on sparc. I had checked the size of the old and the new structure with gcc
> > >> on x86_64 and didn't see a field size increase.
> > >
> > > clang output doesn't change:
> > >
> > > https://godbolt.org/z/bTeeq19j1
> > >
> > > gcc ought to generate the same size.
> > >
> > > It ought to be 'char data[];' though.
> > >
> >
> > I am never sure if [] or [0] is "correct". Anyway, is there agreement that this
> > is an acceptable solution ? I'll be happy to resend if that is the case.
> 
> Yes, looks good to me, in the [] version. I think the [0] version can be
> interpreted as a zero-length array that may not be accessed, while the
> [] flexible array syntax clearly means that extra data follows, and it's
> part of the C standard now, while [0] is a gcc extension.

More problematic is where is the correct place for the 'char data[]'.
It follows the header rather than being part of it.

So the:
	data = (void *)(hdr + 1);
construct (I've lost the original patch) is absolutely descriptive.

gcc is getting to be a real PITA for system coding.

For this particular check 'size 0' ought to be 'size unknown'
and always valid.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Guenter Roeck]

On Tue, Sep 14, 2021 at 03:03:51PM +0000, David Laight wrote:
> From: Arnd Bergmann
> > Sent: 14 September 2021 15:54
> > 
> > On Tue, Sep 14, 2021 at 4:24 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > On 9/14/21 7:17 AM, David Laight wrote:
> > > >> Sorry, I didn't realize that a field of size 0 increases the structure size
> > > >> on sparc. I had checked the size of the old and the new structure with gcc
> > > >> on x86_64 and didn't see a field size increase.
> > > >
> > > > clang output doesn't change:
> > > >
> > > > https://godbolt.org/z/bTeeq19j1
> > > >
> > > > gcc ought to generate the same size.
> > > >
> > > > It ought to be 'char data[];' though.
> > > >
> > >
> > > I am never sure if [] or [0] is "correct". Anyway, is there agreement that this
> > > is an acceptable solution ? I'll be happy to resend if that is the case.
> > 
> > Yes, looks good to me, in the [] version. I think the [0] version can be
> > interpreted as a zero-length array that may not be accessed, while the
> > [] flexible array syntax clearly means that extra data follows, and it's
> > part of the C standard now, while [0] is a gcc extension.
> 
> More problematic is where is the correct place for the 'char data[]'.
> It follows the header rather than being part of it.

I personally always prefer the simple solution, and I don't really care
about such nuances. I take it as granted that a header is followed by data,
and I think that a zero-length field at the end of a header is a perfectly
valid means to express that, but that is just my personal opinion.

Anyway, I take that as non-agreement and won't resend at this time.

Thanks,
Guenter