* [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
@ 2021-03-23 22:48 Gustavo A. R. Silva
2021-03-25 13:45 ` David Laight
2021-03-29 14:57 ` Chuck Lever
0 siblings, 2 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-23 22:48 UTC (permalink / raw)
To: J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, Gustavo A. R. Silva, linux-hardening
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Use an anonymous union with a couple of anonymous structs in order to
keep userspace unchanged:
$ pahole -C nfs_fhbase_new fs/nfsd/nfsfh.o
struct nfs_fhbase_new {
union {
struct {
__u8 fb_version_aux; /* 0 1 */
__u8 fb_auth_type_aux; /* 1 1 */
__u8 fb_fsid_type_aux; /* 2 1 */
__u8 fb_fileid_type_aux; /* 3 1 */
__u32 fb_auth[1]; /* 4 4 */
}; /* 0 8 */
struct {
__u8 fb_version; /* 0 1 */
__u8 fb_auth_type; /* 1 1 */
__u8 fb_fsid_type; /* 2 1 */
__u8 fb_fileid_type; /* 3 1 */
__u32 fb_auth_flex[0]; /* 4 0 */
}; /* 0 4 */
}; /* 0 8 */
/* size: 8, cachelines: 1, members: 1 */
/* last cacheline: 8 bytes */
};
Also, this helps with the ongoing efforts to enable -Warray-bounds by
fixing the following warnings:
fs/nfsd/nfsfh.c: In function ‘nfsd_set_fh_dentry’:
fs/nfsd/nfsfh.c:191:41: warning: array subscript 1 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
191 | ntohl((__force __be32)fh->fh_fsid[1])));
| ~~~~~~~~~~~^~~
./include/linux/kdev_t.h:12:46: note: in definition of macro ‘MKDEV’
12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
| ^~
./include/uapi/linux/byteorder/little_endian.h:40:26: note: in expansion of macro ‘__swab32’
40 | #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
| ^~~~~~~~
./include/linux/byteorder/generic.h:136:21: note: in expansion of macro ‘__be32_to_cpu’
136 | #define ___ntohl(x) __be32_to_cpu(x)
| ^~~~~~~~~~~~~
./include/linux/byteorder/generic.h:140:18: note: in expansion of macro ‘___ntohl’
140 | #define ntohl(x) ___ntohl(x)
| ^~~~~~~~
fs/nfsd/nfsfh.c:191:8: note: in expansion of macro ‘ntohl’
191 | ntohl((__force __be32)fh->fh_fsid[1])));
| ^~~~~
fs/nfsd/nfsfh.c:192:32: warning: array subscript 2 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
192 | fh->fh_fsid[1] = fh->fh_fsid[2];
| ~~~~~~~~~~~^~~
fs/nfsd/nfsfh.c:192:15: warning: array subscript 1 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
192 | fh->fh_fsid[1] = fh->fh_fsid[2];
| ~~~~~~~~~~~^~~
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
include/uapi/linux/nfsd/nfsfh.h | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/include/uapi/linux/nfsd/nfsfh.h b/include/uapi/linux/nfsd/nfsfh.h
index ff0ca88b1c8f..427294dd56a1 100644
--- a/include/uapi/linux/nfsd/nfsfh.h
+++ b/include/uapi/linux/nfsd/nfsfh.h
@@ -64,13 +64,24 @@ struct nfs_fhbase_old {
* in include/linux/exportfs.h for currently registered values.
*/
struct nfs_fhbase_new {
- __u8 fb_version; /* == 1, even => nfs_fhbase_old */
- __u8 fb_auth_type;
- __u8 fb_fsid_type;
- __u8 fb_fileid_type;
- __u32 fb_auth[1];
-/* __u32 fb_fsid[0]; floating */
-/* __u32 fb_fileid[0]; floating */
+ union {
+ struct {
+ __u8 fb_version_aux; /* == 1, even => nfs_fhbase_old */
+ __u8 fb_auth_type_aux;
+ __u8 fb_fsid_type_aux;
+ __u8 fb_fileid_type_aux;
+ __u32 fb_auth[1];
+ /* __u32 fb_fsid[0]; floating */
+ /* __u32 fb_fileid[0]; floating */
+ };
+ struct {
+ __u8 fb_version; /* == 1, even => nfs_fhbase_old */
+ __u8 fb_auth_type;
+ __u8 fb_fsid_type;
+ __u8 fb_fileid_type;
+ __u32 fb_auth_flex[]; /* flexible-array member */
+ };
+ };
};
struct knfsd_fh {
@@ -97,7 +108,7 @@ struct knfsd_fh {
#define fh_fsid_type fh_base.fh_new.fb_fsid_type
#define fh_auth_type fh_base.fh_new.fb_auth_type
#define fh_fileid_type fh_base.fh_new.fb_fileid_type
-#define fh_fsid fh_base.fh_new.fb_auth
+#define fh_fsid fh_base.fh_new.fb_auth_flex
/* Do not use, provided for userspace compatiblity. */
#define fh_auth fh_base.fh_new.fb_auth
--
2.27.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-25 13:45 ` David Laight
@ 2021-03-25 13:18 ` Gustavo A. R. Silva
2021-03-25 15:29 ` David Laight
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-25 13:18 UTC (permalink / raw)
To: David Laight, 'Gustavo A. R. Silva',
J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, linux-hardening
On 3/25/21 08:45, David Laight wrote:
> From: Gustavo A. R. Silva
>> Sent: 23 March 2021 22:49
>>
>> There is a regular need in the kernel to provide a way to declare having
>> a dynamically sized set of trailing elements in a structure. Kernel code
>> should always use “flexible array members”[1] for these cases. The older
>> style of one-element or zero-length arrays should no longer be used[2].
>>
>> Use an anonymous union with a couple of anonymous structs in order to
>> keep userspace unchanged:
>>
>> $ pahole -C nfs_fhbase_new fs/nfsd/nfsfh.o
>> struct nfs_fhbase_new {
>> union {
>> struct {
>> __u8 fb_version_aux; /* 0 1 */
>> __u8 fb_auth_type_aux; /* 1 1 */
>> __u8 fb_fsid_type_aux; /* 2 1 */
>> __u8 fb_fileid_type_aux; /* 3 1 */
>> __u32 fb_auth[1]; /* 4 4 */
>> }; /* 0 8 */
>> struct {
>> __u8 fb_version; /* 0 1 */
>> __u8 fb_auth_type; /* 1 1 */
>> __u8 fb_fsid_type; /* 2 1 */
>> __u8 fb_fileid_type; /* 3 1 */
>> __u32 fb_auth_flex[0]; /* 4 0 */
>> }; /* 0 4 */
>> }; /* 0 8 */
>>
>> /* size: 8, cachelines: 1, members: 1 */
>> /* last cacheline: 8 bytes */
>> };
>
> Could you use the simpler:
>> struct nfs_fhbase_new {
>> __u8 fb_version;
>> __u8 fb_auth_type;
>> __u8 fb_fsid_type;
>> __u8 fb_fileid_type;
>> union {
>> __u32 fb_auth[1];
>> __u32 fb_auth_flex[0];
>> };
>> };
>
> Although I'm not certain flexible arrays are supported
> as the last element of a union.
Nope; this is not allowed: https://godbolt.org/z/14vd4o8na
--
Gustavo
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-23 22:48 [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member Gustavo A. R. Silva
@ 2021-03-25 13:45 ` David Laight
2021-03-25 13:18 ` Gustavo A. R. Silva
2021-03-29 14:57 ` Chuck Lever
1 sibling, 1 reply; 9+ messages in thread
From: David Laight @ 2021-03-25 13:45 UTC (permalink / raw)
To: 'Gustavo A. R. Silva', J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, linux-hardening
From: Gustavo A. R. Silva
> Sent: 23 March 2021 22:49
>
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
>
> Use an anonymous union with a couple of anonymous structs in order to
> keep userspace unchanged:
>
> $ pahole -C nfs_fhbase_new fs/nfsd/nfsfh.o
> struct nfs_fhbase_new {
> union {
> struct {
> __u8 fb_version_aux; /* 0 1 */
> __u8 fb_auth_type_aux; /* 1 1 */
> __u8 fb_fsid_type_aux; /* 2 1 */
> __u8 fb_fileid_type_aux; /* 3 1 */
> __u32 fb_auth[1]; /* 4 4 */
> }; /* 0 8 */
> struct {
> __u8 fb_version; /* 0 1 */
> __u8 fb_auth_type; /* 1 1 */
> __u8 fb_fsid_type; /* 2 1 */
> __u8 fb_fileid_type; /* 3 1 */
> __u32 fb_auth_flex[0]; /* 4 0 */
> }; /* 0 4 */
> }; /* 0 8 */
>
> /* size: 8, cachelines: 1, members: 1 */
> /* last cacheline: 8 bytes */
> };
Could you use the simpler:
> struct nfs_fhbase_new {
> __u8 fb_version;
> __u8 fb_auth_type;
> __u8 fb_fsid_type;
> __u8 fb_fileid_type;
> union {
> __u32 fb_auth[1];
> __u32 fb_auth_flex[0];
> };
> };
Although I'm not certain flexible arrays are supported
as the last element of a union.
You might need to use a named anonymous structure for the
four __u8 fields and create two different structures that
include the extra field on the end.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-25 13:18 ` Gustavo A. R. Silva
@ 2021-03-25 15:29 ` David Laight
2021-03-25 21:12 ` Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2021-03-25 15:29 UTC (permalink / raw)
To: 'Gustavo A. R. Silva', 'Gustavo A. R. Silva',
J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, linux-hardening
From: Gustavo A. R. Silva
> Sent: 25 March 2021 13:18
>
> On 3/25/21 08:45, David Laight wrote:
> > From: Gustavo A. R. Silva
> >> Sent: 23 March 2021 22:49
> >>
> >> There is a regular need in the kernel to provide a way to declare having
> >> a dynamically sized set of trailing elements in a structure. Kernel code
> >> should always use “flexible array members”[1] for these cases. The older
> >> style of one-element or zero-length arrays should no longer be used[2].
> >>
> >> Use an anonymous union with a couple of anonymous structs in order to
> >> keep userspace unchanged:
> >>
> >> $ pahole -C nfs_fhbase_new fs/nfsd/nfsfh.o
> >> struct nfs_fhbase_new {
> >> union {
> >> struct {
> >> __u8 fb_version_aux; /* 0 1 */
> >> __u8 fb_auth_type_aux; /* 1 1 */
> >> __u8 fb_fsid_type_aux; /* 2 1 */
> >> __u8 fb_fileid_type_aux; /* 3 1 */
> >> __u32 fb_auth[1]; /* 4 4 */
> >> }; /* 0 8 */
> >> struct {
> >> __u8 fb_version; /* 0 1 */
> >> __u8 fb_auth_type; /* 1 1 */
> >> __u8 fb_fsid_type; /* 2 1 */
> >> __u8 fb_fileid_type; /* 3 1 */
> >> __u32 fb_auth_flex[0]; /* 4 0 */
> >> }; /* 0 4 */
> >> }; /* 0 8 */
> >>
> >> /* size: 8, cachelines: 1, members: 1 */
> >> /* last cacheline: 8 bytes */
> >> };
> >
> > Could you use the simpler:
> >> struct nfs_fhbase_new {
> >> __u8 fb_version;
> >> __u8 fb_auth_type;
> >> __u8 fb_fsid_type;
> >> __u8 fb_fileid_type;
> >> union {
> >> __u32 fb_auth[1];
> >> __u32 fb_auth_flex[0];
> >> };
> >> };
> >
> > Although I'm not certain flexible arrays are supported
> > as the last element of a union.
>
> Nope; this is not allowed: https://godbolt.org/z/14vd4o8na
Nothing an extra 'struct {__u32 fb_auth_flex[0]; }'; won't solve.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-25 15:29 ` David Laight
@ 2021-03-25 21:12 ` Gustavo A. R. Silva
2021-03-26 8:17 ` David Laight
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-25 21:12 UTC (permalink / raw)
To: David Laight, 'Gustavo A. R. Silva',
J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, linux-hardening
On 3/25/21 10:29, David Laight wrote:
>>>
>>> Could you use the simpler:
>>>> struct nfs_fhbase_new {
>>>> __u8 fb_version;
>>>> __u8 fb_auth_type;
>>>> __u8 fb_fsid_type;
>>>> __u8 fb_fileid_type;
>>>> union {
>>>> __u32 fb_auth[1];
>>>> __u32 fb_auth_flex[0];
>>>> };
>>>> };
>>>
>>> Although I'm not certain flexible arrays are supported
>>> as the last element of a union.
>>
>> Nope; this is not allowed: https://godbolt.org/z/14vd4o8na
>
> Nothing an extra 'struct {__u32 fb_auth_flex[0]; }'; won't solve.
We don't want to introduce zero-length arrays [1].
--
Gustavo
[1] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-25 21:12 ` Gustavo A. R. Silva
@ 2021-03-26 8:17 ` David Laight
2021-03-26 14:10 ` Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2021-03-26 8:17 UTC (permalink / raw)
To: 'Gustavo A. R. Silva', 'Gustavo A. R. Silva',
J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, linux-hardening
From: Gustavo A. R. Silva
> Sent: 25 March 2021 21:12
>
> On 3/25/21 10:29, David Laight wrote:
>
> >>>
> >>> Could you use the simpler:
> >>>> struct nfs_fhbase_new {
> >>>> __u8 fb_version;
> >>>> __u8 fb_auth_type;
> >>>> __u8 fb_fsid_type;
> >>>> __u8 fb_fileid_type;
> >>>> union {
> >>>> __u32 fb_auth[1];
> >>>> __u32 fb_auth_flex[0];
> >>>> };
> >>>> };
> >>>
> >>> Although I'm not certain flexible arrays are supported
> >>> as the last element of a union.
> >>
> >> Nope; this is not allowed: https://godbolt.org/z/14vd4o8na
> >
> > Nothing an extra 'struct {__u32 fb_auth_flex[0]; }'; won't solve.
>
> We don't want to introduce zero-length arrays [1].
I probably meant to write [] not [0] - doesn't affect the idea.
The real problem is that the compiler is likely to start rejecting
references to a flex array that go beyond the end of the outer
structure.
Thinking back, isn't fb_auth[] at least one entry long?
So it could be:
struct nfs_fhbase_new {
__u8 fb_version;
__u8 fb_auth_type;
__u8 fb_fsid_type;
__u8 fb_fileid_type;
__u32 fb_auth[1];
__u32 fb_auth_extra[];
};
(I've missed the 0 out this time...)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-26 8:17 ` David Laight
@ 2021-03-26 14:10 ` Gustavo A. R. Silva
0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-26 14:10 UTC (permalink / raw)
To: David Laight, 'Gustavo A. R. Silva',
J. Bruce Fields, Chuck Lever
Cc: linux-nfs, linux-kernel, linux-hardening
On 3/26/21 03:17, David Laight wrote:
> From: Gustavo A. R. Silva
>> Sent: 25 March 2021 21:12
>>
>> On 3/25/21 10:29, David Laight wrote:
>>
>>>>>
>>>>> Could you use the simpler:
>>>>>> struct nfs_fhbase_new {
>>>>>> __u8 fb_version;
>>>>>> __u8 fb_auth_type;
>>>>>> __u8 fb_fsid_type;
>>>>>> __u8 fb_fileid_type;
>>>>>> union {
>>>>>> __u32 fb_auth[1];
>>>>>> __u32 fb_auth_flex[0];
>>>>>> };
>>>>>> };
>>>>>
>>>>> Although I'm not certain flexible arrays are supported
>>>>> as the last element of a union.
>>>>
>>>> Nope; this is not allowed: https://godbolt.org/z/14vd4o8na
>>>
>>> Nothing an extra 'struct {__u32 fb_auth_flex[0]; }'; won't solve.
>>
>> We don't want to introduce zero-length arrays [1].
>
> I probably meant to write [] not [0] - doesn't affect the idea.
>
> The real problem is that the compiler is likely to start rejecting
> references to a flex array that go beyond the end of the outer
> structure.
>
> Thinking back, isn't fb_auth[] at least one entry long?
> So it could be:
>
> struct nfs_fhbase_new {
> __u8 fb_version;
> __u8 fb_auth_type;
> __u8 fb_fsid_type;
> __u8 fb_fileid_type;
> __u32 fb_auth[1];
> __u32 fb_auth_extra[];
> };
I don't think this is a great idea because, contrary to the change I'm
proposing, in this case memory regions for fb_auth and fb_auth_extra
don't actually overlap.
--
Gustavo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-29 14:57 ` Chuck Lever
@ 2021-03-29 14:51 ` Gustavo A. R. Silva
0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-03-29 14:51 UTC (permalink / raw)
To: chucklever, Gustavo A. R. Silva
Cc: J. Bruce Fields, Linux NFS Mailing List,
Linux Kernel Mailing List, linux-hardening
On 3/29/21 09:57, Chuck Lever wrote:
> Sorry for the reply via gmail, the original patch did not show up in
> my Oracle mailbox.
>
> I've been waiting for a resolution of this thread (and perhaps a
> Reviewed-by). But in
> the meantime I've committed this, provisionally, to the for-next topic branch in
>
> git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
Awesome. :)
Thanks, Chuck.
--
Gustavo
> On Wed, Mar 24, 2021 at 4:39 AM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
>>
>> There is a regular need in the kernel to provide a way to declare having
>> a dynamically sized set of trailing elements in a structure. Kernel code
>> should always use “flexible array members”[1] for these cases. The older
>> style of one-element or zero-length arrays should no longer be used[2].
>>
>> Use an anonymous union with a couple of anonymous structs in order to
>> keep userspace unchanged:
>>
>> $ pahole -C nfs_fhbase_new fs/nfsd/nfsfh.o
>> struct nfs_fhbase_new {
>> union {
>> struct {
>> __u8 fb_version_aux; /* 0 1 */
>> __u8 fb_auth_type_aux; /* 1 1 */
>> __u8 fb_fsid_type_aux; /* 2 1 */
>> __u8 fb_fileid_type_aux; /* 3 1 */
>> __u32 fb_auth[1]; /* 4 4 */
>> }; /* 0 8 */
>> struct {
>> __u8 fb_version; /* 0 1 */
>> __u8 fb_auth_type; /* 1 1 */
>> __u8 fb_fsid_type; /* 2 1 */
>> __u8 fb_fileid_type; /* 3 1 */
>> __u32 fb_auth_flex[0]; /* 4 0 */
>> }; /* 0 4 */
>> }; /* 0 8 */
>>
>> /* size: 8, cachelines: 1, members: 1 */
>> /* last cacheline: 8 bytes */
>> };
>>
>> Also, this helps with the ongoing efforts to enable -Warray-bounds by
>> fixing the following warnings:
>>
>> fs/nfsd/nfsfh.c: In function ‘nfsd_set_fh_dentry’:
>> fs/nfsd/nfsfh.c:191:41: warning: array subscript 1 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>> 191 | ntohl((__force __be32)fh->fh_fsid[1])));
>> | ~~~~~~~~~~~^~~
>> ./include/linux/kdev_t.h:12:46: note: in definition of macro ‘MKDEV’
>> 12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
>> | ^~
>> ./include/uapi/linux/byteorder/little_endian.h:40:26: note: in expansion of macro ‘__swab32’
>> 40 | #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
>> | ^~~~~~~~
>> ./include/linux/byteorder/generic.h:136:21: note: in expansion of macro ‘__be32_to_cpu’
>> 136 | #define ___ntohl(x) __be32_to_cpu(x)
>> | ^~~~~~~~~~~~~
>> ./include/linux/byteorder/generic.h:140:18: note: in expansion of macro ‘___ntohl’
>> 140 | #define ntohl(x) ___ntohl(x)
>> | ^~~~~~~~
>> fs/nfsd/nfsfh.c:191:8: note: in expansion of macro ‘ntohl’
>> 191 | ntohl((__force __be32)fh->fh_fsid[1])));
>> | ^~~~~
>> fs/nfsd/nfsfh.c:192:32: warning: array subscript 2 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>> 192 | fh->fh_fsid[1] = fh->fh_fsid[2];
>> | ~~~~~~~~~~~^~~
>> fs/nfsd/nfsfh.c:192:15: warning: array subscript 1 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
>> 192 | fh->fh_fsid[1] = fh->fh_fsid[2];
>> | ~~~~~~~~~~~^~~
>>
>> [1] https://en.wikipedia.org/wiki/Flexible_array_member
>> [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Link: https://github.com/KSPP/linux/issues/109
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> include/uapi/linux/nfsd/nfsfh.h | 27 +++++++++++++++++++--------
>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/uapi/linux/nfsd/nfsfh.h b/include/uapi/linux/nfsd/nfsfh.h
>> index ff0ca88b1c8f..427294dd56a1 100644
>> --- a/include/uapi/linux/nfsd/nfsfh.h
>> +++ b/include/uapi/linux/nfsd/nfsfh.h
>> @@ -64,13 +64,24 @@ struct nfs_fhbase_old {
>> * in include/linux/exportfs.h for currently registered values.
>> */
>> struct nfs_fhbase_new {
>> - __u8 fb_version; /* == 1, even => nfs_fhbase_old */
>> - __u8 fb_auth_type;
>> - __u8 fb_fsid_type;
>> - __u8 fb_fileid_type;
>> - __u32 fb_auth[1];
>> -/* __u32 fb_fsid[0]; floating */
>> -/* __u32 fb_fileid[0]; floating */
>> + union {
>> + struct {
>> + __u8 fb_version_aux; /* == 1, even => nfs_fhbase_old */
>> + __u8 fb_auth_type_aux;
>> + __u8 fb_fsid_type_aux;
>> + __u8 fb_fileid_type_aux;
>> + __u32 fb_auth[1];
>> + /* __u32 fb_fsid[0]; floating */
>> + /* __u32 fb_fileid[0]; floating */
>> + };
>> + struct {
>> + __u8 fb_version; /* == 1, even => nfs_fhbase_old */
>> + __u8 fb_auth_type;
>> + __u8 fb_fsid_type;
>> + __u8 fb_fileid_type;
>> + __u32 fb_auth_flex[]; /* flexible-array member */
>> + };
>> + };
>> };
>>
>> struct knfsd_fh {
>> @@ -97,7 +108,7 @@ struct knfsd_fh {
>> #define fh_fsid_type fh_base.fh_new.fb_fsid_type
>> #define fh_auth_type fh_base.fh_new.fb_auth_type
>> #define fh_fileid_type fh_base.fh_new.fb_fileid_type
>> -#define fh_fsid fh_base.fh_new.fb_auth
>> +#define fh_fsid fh_base.fh_new.fb_auth_flex
>>
>> /* Do not use, provided for userspace compatiblity. */
>> #define fh_auth fh_base.fh_new.fb_auth
>> --
>> 2.27.0
>>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
2021-03-23 22:48 [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member Gustavo A. R. Silva
2021-03-25 13:45 ` David Laight
@ 2021-03-29 14:57 ` Chuck Lever
2021-03-29 14:51 ` Gustavo A. R. Silva
1 sibling, 1 reply; 9+ messages in thread
From: Chuck Lever @ 2021-03-29 14:57 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: J. Bruce Fields, Linux NFS Mailing List,
Linux Kernel Mailing List, linux-hardening
Sorry for the reply via gmail, the original patch did not show up in
my Oracle mailbox.
I've been waiting for a resolution of this thread (and perhaps a
Reviewed-by). But in
the meantime I've committed this, provisionally, to the for-next topic branch in
git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
On Wed, Mar 24, 2021 at 4:39 AM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
>
> Use an anonymous union with a couple of anonymous structs in order to
> keep userspace unchanged:
>
> $ pahole -C nfs_fhbase_new fs/nfsd/nfsfh.o
> struct nfs_fhbase_new {
> union {
> struct {
> __u8 fb_version_aux; /* 0 1 */
> __u8 fb_auth_type_aux; /* 1 1 */
> __u8 fb_fsid_type_aux; /* 2 1 */
> __u8 fb_fileid_type_aux; /* 3 1 */
> __u32 fb_auth[1]; /* 4 4 */
> }; /* 0 8 */
> struct {
> __u8 fb_version; /* 0 1 */
> __u8 fb_auth_type; /* 1 1 */
> __u8 fb_fsid_type; /* 2 1 */
> __u8 fb_fileid_type; /* 3 1 */
> __u32 fb_auth_flex[0]; /* 4 0 */
> }; /* 0 4 */
> }; /* 0 8 */
>
> /* size: 8, cachelines: 1, members: 1 */
> /* last cacheline: 8 bytes */
> };
>
> Also, this helps with the ongoing efforts to enable -Warray-bounds by
> fixing the following warnings:
>
> fs/nfsd/nfsfh.c: In function ‘nfsd_set_fh_dentry’:
> fs/nfsd/nfsfh.c:191:41: warning: array subscript 1 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 191 | ntohl((__force __be32)fh->fh_fsid[1])));
> | ~~~~~~~~~~~^~~
> ./include/linux/kdev_t.h:12:46: note: in definition of macro ‘MKDEV’
> 12 | #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi))
> | ^~
> ./include/uapi/linux/byteorder/little_endian.h:40:26: note: in expansion of macro ‘__swab32’
> 40 | #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
> | ^~~~~~~~
> ./include/linux/byteorder/generic.h:136:21: note: in expansion of macro ‘__be32_to_cpu’
> 136 | #define ___ntohl(x) __be32_to_cpu(x)
> | ^~~~~~~~~~~~~
> ./include/linux/byteorder/generic.h:140:18: note: in expansion of macro ‘___ntohl’
> 140 | #define ntohl(x) ___ntohl(x)
> | ^~~~~~~~
> fs/nfsd/nfsfh.c:191:8: note: in expansion of macro ‘ntohl’
> 191 | ntohl((__force __be32)fh->fh_fsid[1])));
> | ^~~~~
> fs/nfsd/nfsfh.c:192:32: warning: array subscript 2 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 192 | fh->fh_fsid[1] = fh->fh_fsid[2];
> | ~~~~~~~~~~~^~~
> fs/nfsd/nfsfh.c:192:15: warning: array subscript 1 is above array bounds of ‘__u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
> 192 | fh->fh_fsid[1] = fh->fh_fsid[2];
> | ~~~~~~~~~~~^~~
>
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/109
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> include/uapi/linux/nfsd/nfsfh.h | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/nfsd/nfsfh.h b/include/uapi/linux/nfsd/nfsfh.h
> index ff0ca88b1c8f..427294dd56a1 100644
> --- a/include/uapi/linux/nfsd/nfsfh.h
> +++ b/include/uapi/linux/nfsd/nfsfh.h
> @@ -64,13 +64,24 @@ struct nfs_fhbase_old {
> * in include/linux/exportfs.h for currently registered values.
> */
> struct nfs_fhbase_new {
> - __u8 fb_version; /* == 1, even => nfs_fhbase_old */
> - __u8 fb_auth_type;
> - __u8 fb_fsid_type;
> - __u8 fb_fileid_type;
> - __u32 fb_auth[1];
> -/* __u32 fb_fsid[0]; floating */
> -/* __u32 fb_fileid[0]; floating */
> + union {
> + struct {
> + __u8 fb_version_aux; /* == 1, even => nfs_fhbase_old */
> + __u8 fb_auth_type_aux;
> + __u8 fb_fsid_type_aux;
> + __u8 fb_fileid_type_aux;
> + __u32 fb_auth[1];
> + /* __u32 fb_fsid[0]; floating */
> + /* __u32 fb_fileid[0]; floating */
> + };
> + struct {
> + __u8 fb_version; /* == 1, even => nfs_fhbase_old */
> + __u8 fb_auth_type;
> + __u8 fb_fsid_type;
> + __u8 fb_fileid_type;
> + __u32 fb_auth_flex[]; /* flexible-array member */
> + };
> + };
> };
>
> struct knfsd_fh {
> @@ -97,7 +108,7 @@ struct knfsd_fh {
> #define fh_fsid_type fh_base.fh_new.fb_fsid_type
> #define fh_auth_type fh_base.fh_new.fb_auth_type
> #define fh_fileid_type fh_base.fh_new.fb_fileid_type
> -#define fh_fsid fh_base.fh_new.fb_auth
> +#define fh_fsid fh_base.fh_new.fb_auth_flex
>
> /* Do not use, provided for userspace compatiblity. */
> #define fh_auth fh_base.fh_new.fb_auth
> --
> 2.27.0
>
--
When the world is being engulfed by a comet, go ahead and excrete
where you want to.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-29 16:12 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 22:48 [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member Gustavo A. R. Silva
2021-03-25 13:45 ` David Laight
2021-03-25 13:18 ` Gustavo A. R. Silva
2021-03-25 15:29 ` David Laight
2021-03-25 21:12 ` Gustavo A. R. Silva
2021-03-26 8:17 ` David Laight
2021-03-26 14:10 ` Gustavo A. R. Silva
2021-03-29 14:57 ` Chuck Lever
2021-03-29 14:51 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.