Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
On Tue, Jun 28, 2022 at 10:54:58AM -0700, Kees Cook wrote: > which must also be assuming it's a header. So probably better to just > drop the driver_data field? I don't see anything using it (that I can > find) besides as a sanity-check that the field exists and is at the end > of the struct. The field is guaranteeing alignment of the following structure. IIRC there are a few cases that we don't have a u64 already to force this. Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
On Tue, Jun 28, 2022 at 09:27:21AM +0200, Geert Uytterhoeven wrote: > Hi Gustavo, > > Thanks for your patch! > > On Mon, Jun 27, 2022 at 8:04 PM Gustavo A. R. Silva > 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]. > > These rules apply to the kernel, but uapi is not considered part of the > kernel, so different rules apply. Uapi header files should work with > whatever compiler that can be used for compiling userspace. Right, userspace isn't bound by these rules, but the kernel ends up consuming these structures, so we need to fix them. The [0] -> [] changes (when they are not erroneously being used within other structures) is valid for all compilers. Flexible arrays are C99; it's been 23 years. :) But, yes, where we DO break stuff we need to workaround it, etc. -- Kees Cook ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
On Mon, Jun 27, 2022 at 09:40:52PM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 27, 2022 at 08:27:37PM +0200, Daniel Borkmann wrote:
> > [...]
> > Fyi, this breaks BPF CI:
> >
> > https://github.com/kernel-patches/bpf/runs/7078719372?check_suite_focus=true
> >
> > [...]
> > progs/map_ptr_kern.c:314:26: error: field 'trie_key' with variable sized
> > type 'struct bpf_lpm_trie_key' not at the end of a struct or class is a GNU
> > extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > struct bpf_lpm_trie_key trie_key;
> > ^
The issue here seems to be a collision between "unknown array size"
and known sizes:
struct bpf_lpm_trie_key {
__u32 prefixlen; /* up to 32 for AF_INET, 128 for AF_INET6 */
__u8data[0];/* Arbitrary size */
};
struct lpm_key {
struct bpf_lpm_trie_key trie_key;
__u32 data;
};
This is treating trie_key as a header, which it's not: it's a complete
structure. :)
Perhaps:
struct lpm_key {
__u32 prefixlen;
__u32 data;
};
I don't see anything else trying to include bpf_lpm_trie_key.
>
> This will break the rdma-core userspace as well, with a similar
> error:
>
> /usr/bin/clang-13 -DVERBS_DEBUG -Dibverbs_EXPORTS -Iinclude
> -I/usr/include/libnl3 -I/usr/include/drm -g -O2 -fdebug-prefix-map=/__w/1/s=.
> -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time
> -D_FORTIFY_SOURCE=2 -Wmissing-prototypes -Wmissing-declarations
> -Wwrite-strings -Wformat=2 -Wcast-function-type -Wformat-nonliteral
> -Wdate-time -Wnested-externs -Wshadow -Wstrict-prototypes
> -Wold-style-definition -Werror -Wredundant-decls -g -fPIC -std=gnu11 -MD
> -MT libibverbs/CMakeFiles/ibverbs.dir/cmd_flow.c.o -MF
> libibverbs/CMakeFiles/ibverbs.dir/cmd_flow.c.o.d -o
> libibverbs/CMakeFiles/ibverbs.dir/cmd_flow.c.o -c ../libibverbs/cmd_flow.c
> In file included from ../libibverbs/cmd_flow.c:33:
> In file included from include/infiniband/cmd_write.h:36:
> In file included from include/infiniband/cmd_ioctl.h:41:
> In file included from include/infiniband/verbs.h:48:
> In file included from include/infiniband/verbs_api.h:66:
> In file included from include/infiniband/ib_user_ioctl_verbs.h:38:
> include/rdma/ib_user_verbs.h:436:34: error: field 'base' with variable sized
> type 'struct ib_uverbs_create_cq_resp' not at the end of a struct or class is
> a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> struct ib_uverbs_create_cq_resp base;
> ^
> include/rdma/ib_user_verbs.h:644:34: error: field 'base' with variable sized
> type 'struct ib_uverbs_create_qp_resp' not at the end of a struct or class is
> a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> struct ib_uverbs_create_qp_resp base;
This looks very similar, a struct of unknown size is being treated as a
header struct:
struct ib_uverbs_create_cq_resp {
__u32 cq_handle;
__u32 cqe;
__aligned_u64 driver_data[0];
};
struct ib_uverbs_ex_create_cq_resp {
struct ib_uverbs_create_cq_resp base;
__u32 comp_mask;
__u32 response_length;
};
And it only gets used here:
DECLARE_UVERBS_WRITE(IB_USER_VERBS_CMD_CREATE_CQ,
ib_uverbs_create_cq,
UAPI_DEF_WRITE_UDATA_IO(
struct ib_uverbs_create_cq,
struct ib_uverbs_create_cq_resp),
^^^
UAPI_DEF_METHOD_NEEDS_FN(create_cq)),
which must also be assuming it's a header. So probably better to just
drop the driver_data field? I don't see anything using it (that I can
find) besides as a sanity-check that the field exists and is at the end
of the struct.
--
Kees Cook
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
On Tue, Jun 28, 2022 at 04:21:29AM +0200, Gustavo A. R. Silva wrote: > > > Though maybe we could just switch off > > > -Wgnu-variable-sized-type-not-at-end during configuration ? > We need to think in a different strategy. I think we will need to switch off the warning in userspace - this is doable for rdma-core. On the other hand, if the goal is to enable the array size check compiler warning I would suggest focusing only on those structs that actually hit that warning in the kernel. IIRC infiniband doesn't trigger it because it just pointer casts the flex array to some other struct. It isn't actually an array it is a placeholder for a trailing structure, so it is never indexed. This is also why we hit the warning because the convient way for userspace to compose the message is to squash the header and trailer structs together in a super struct on the stack, then invoke the ioctl. Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
Hi Gustavo,
Thanks for your patch!
On Mon, Jun 27, 2022 at 8:04 PM Gustavo A. R. Silva
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].
These rules apply to the kernel, but uapi is not considered part of the
kernel, so different rules apply. Uapi header files should work with
whatever compiler that can be used for compiling userspace.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
On Mon, Jun 27, 2022 at 08:27:37PM +0200, Daniel Borkmann wrote:
> On 6/27/22 8:04 PM, Gustavo A. R. Silva 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].
> >
> > This code was transformed with the help of Coccinelle:
> > (linux-5.19-rc2$ spatch --jobs $(getconf _NPROCESSORS_ONLN) --sp-file
> > script.cocci --include-headers --dir . > output.patch)
> >
> > @@
> > identifier S, member, array;
> > type T1, T2;
> > @@
> >
> > struct S {
> >...
> >T1 member;
> >T2 array[
> > - 0
> >];
> > };
> >
> > -fstrict-flex-arrays=3 is coming and we need to land these changes
> > to prevent issues like these in the short future:
> >
> > ../fs/minix/dir.c:337:3: warning: 'strcpy' will always overflow;
> > destination buffer has size 0,
> > but the source string has length 2 (including NUL byte) [-Wfortify-source]
> > strcpy(de3->name, ".");
> > ^
> >
> > Since these are all [0] to [] changes, the risk to UAPI is nearly zero. If
> > this breaks anything, we can use a union with a new member name.
> >
> > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > [2]
> > https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> >
> > Link: https://github.com/KSPP/linux/issues/78
> > Build-tested-by:
> > https://lore.kernel.org/lkml/62b675ec.wkx6aoz6cbe71vtf%25...@intel.com/
> > Signed-off-by: Gustavo A. R. Silva
> > ---
> > Hi all!
> >
> > JFYI: I'm adding this to my -next tree. :)
>
> Fyi, this breaks BPF CI:
>
> https://github.com/kernel-patches/bpf/runs/7078719372?check_suite_focus=true
>
> [...]
> progs/map_ptr_kern.c:314:26: error: field 'trie_key' with variable sized
> type 'struct bpf_lpm_trie_key' not at the end of a struct or class is a GNU
> extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> struct bpf_lpm_trie_key trie_key;
> ^
This will break the rdma-core userspace as well, with a similar
error:
/usr/bin/clang-13 -DVERBS_DEBUG -Dibverbs_EXPORTS -Iinclude
-I/usr/include/libnl3 -I/usr/include/drm -g -O2 -fdebug-prefix-map=/__w/1/s=.
-fstack-protector-strong -Wformat -Werror=format-security -Wdate-time
-D_FORTIFY_SOURCE=2 -Wmissing-prototypes -Wmissing-declarations -Wwrite-strings
-Wformat=2 -Wcast-function-type -Wformat-nonliteral -Wdate-time
-Wnested-externs -Wshadow -Wstrict-prototypes -Wold-style-definition -Werror
-Wredundant-decls -g -fPIC -std=gnu11 -MD -MT
libibverbs/CMakeFiles/ibverbs.dir/cmd_flow.c.o -MF
libibverbs/CMakeFiles/ibverbs.dir/cmd_flow.c.o.d -o
libibverbs/CMakeFiles/ibverbs.dir/cmd_flow.c.o -c ../libibverbs/cmd_flow.c
In file included from ../libibverbs/cmd_flow.c:33:
In file included from include/infiniband/cmd_write.h:36:
In file included from include/infiniband/cmd_ioctl.h:41:
In file included from include/infiniband/verbs.h:48:
In file included from include/infiniband/verbs_api.h:66:
In file included from include/infiniband/ib_user_ioctl_verbs.h:38:
include/rdma/ib_user_verbs.h:436:34: error: field 'base' with variable sized
type 'struct ib_uverbs_create_cq_resp' not at the end of a struct or class is a
GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct ib_uverbs_create_cq_resp base;
^
include/rdma/ib_user_verbs.h:644:34: error: field 'base' with variable sized
type 'struct ib_uverbs_create_qp_resp' not at the end of a struct or class is a
GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct ib_uverbs_create_qp_resp base;
Which is why I gave up trying to change these..
Though maybe we could just switch off -Wgnu-variable-sized-type-not-at-end
during configuration ?
Jason
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
Gustavo A. R. Silva 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].
>
> This code was transformed with the help of Coccinelle:
> (linux-5.19-rc2$ spatch --jobs $(getconf _NPROCESSORS_ONLN) --sp-file
> script.cocci --include-headers --dir . > output.patch)
>
> @@
> identifier S, member, array;
> type T1, T2;
> @@
>
> struct S {
> ...
> T1 member;
> T2 array[
> - 0
> ];
> };
>
> -fstrict-flex-arrays=3 is coming and we need to land these changes
> to prevent issues like these in the short future:
>
> ../fs/minix/dir.c:337:3: warning: 'strcpy' will always overflow; destination
> buffer has size 0,
> but the source string has length 2 (including NUL byte) [-Wfortify-source]
> strcpy(de3->name, ".");
> ^
>
> Since these are all [0] to [] changes, the risk to UAPI is nearly zero. If
> this breaks anything, we can use a union with a new member name.
>
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2]
> https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
>
> Link: https://github.com/KSPP/linux/issues/78
> Build-tested-by:
> https://lore.kernel.org/lkml/62b675ec.wkx6aoz6cbe71vtf%25...@intel.com/
> Signed-off-by: Gustavo A. R. Silva
> ---
> Hi all!
>
> JFYI: I'm adding this to my -next tree. :)
>
[..]
> include/uapi/linux/ndctl.h| 10 +--
For ndctl.h
Acked-by: Dan Williams
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][next] treewide: uapi: Replace zero-length arrays with flexible-array members
On 6/27/22 8:04 PM, Gustavo A. R. Silva 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].
This code was transformed with the help of Coccinelle:
(linux-5.19-rc2$ spatch --jobs $(getconf _NPROCESSORS_ONLN) --sp-file script.cocci
--include-headers --dir . > output.patch)
@@
identifier S, member, array;
type T1, T2;
@@
struct S {
...
T1 member;
T2 array[
- 0
];
};
-fstrict-flex-arrays=3 is coming and we need to land these changes
to prevent issues like these in the short future:
../fs/minix/dir.c:337:3: warning: 'strcpy' will always overflow; destination
buffer has size 0,
but the source string has length 2 (including NUL byte) [-Wfortify-source]
strcpy(de3->name, ".");
^
Since these are all [0] to [] changes, the risk to UAPI is nearly zero. If
this breaks anything, we can use a union with a new member name.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/78
Build-tested-by:
https://lore.kernel.org/lkml/62b675ec.wkx6aoz6cbe71vtf%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
Hi all!
JFYI: I'm adding this to my -next tree. :)
Fyi, this breaks BPF CI:
https://github.com/kernel-patches/bpf/runs/7078719372?check_suite_focus=true
[...]
progs/map_ptr_kern.c:314:26: error: field 'trie_key' with variable sized type
'struct bpf_lpm_trie_key' not at the end of a struct or class is a GNU
extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct bpf_lpm_trie_key trie_key;
^
1 error generated.
make: *** [Makefile:519:
/tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/map_ptr_kern.o] Error 1
make: *** Waiting for unfinished jobs
Error: Process completed with exit code 2.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
