| Message ID | 20210913163712.922188-1-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | sparc: mdesc: Fix compile error seen with gcc 11.x | expand |
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; }
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
> -----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)
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
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
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)
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
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)
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(-)