From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To: chucklever@gmail.com, "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: "J. Bruce Fields" <bfields@fieldses.org>,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-hardening@vger.kernel.org
Subject: Re: [PATCH][next] UAPI: nfsfh.h: Replace one-element array with flexible-array member
Date: Mon, 29 Mar 2021 09:51:33 -0500 [thread overview]
Message-ID: <f2333f4b-15ea-cb3e-eea0-d349b469e082@embeddedor.com> (raw)
In-Reply-To: <CAFMMQGtZLNt-jsFFEJVGZaOQyVxckXEGbtan8oDZ-WUNSHkGWg@mail.gmail.com>
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
>>
>
>
prev parent reply other threads:[~2021-03-29 16:12 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f2333f4b-15ea-cb3e-eea0-d349b469e082@embeddedor.com \
--to=gustavo@embeddedor.com \
--cc=bfields@fieldses.org \
--cc=chucklever@gmail.com \
--cc=gustavoars@kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.