-Wgnu-variable-sized-type-not-at-end in include/linux/cgroup-defs.h

[alviroiskandar]

Hello @nickdesaulniers, @nathanchance

Could you review this patch? Not submitted yet, what do you think?

From e7b1c9380ded1e5386c4003d27c73f9de4b72953 Mon Sep 17 00:00:00 2001
From: Alviro Iskandar Setiawan <alviro.iskandar@gmail.com>
Date: Tue, 8 Feb 2022 20:47:30 +0700
Subject: [PATCH] cgroup: Fix -Wgnu-variable-sized-type-not-at-end error in
 struct cgroup_root

Clang says:

  ./include/linux/cgroup-defs.h:508:16: error: field 'cgrp' with \
  variable sized type 'struct cgroup' not at the end of a struct \
  or class is a GNU extension

This is because `struct cgroup cgrp` is a variable-sized struct, but
it is placed in the middle of `struct cgroup_root`. A variable sized
struct should be placed at the end of struct.

In 2017, Nick tried to fix this, but Tejun was unhappy about that [1].

While the usage for `struct cgroup_root` is valid and it will not
clobber any unintended field, it can be painful for other places that
want to check their code with -Wgnu-variable-sized-type-not-at-end
enabled since cgroup-defs.h is included from many places.

If `struct cgroup_root` were private to a C file, it's nice to silence
this warning locally to that C file. Unfortunately, it is not.

Let's allow the compiler to generate error when the flexible array
does not occur last in the structure, which helps to prevent some
kind of undefined behavior bugs from being inadvertently introduced
to the codebase (explained by "Gustavo A. R. Silva" at [2]).

To keep the changes minimal, wrap extra space at the end of struct
with an anonymous union. This way we don't need to change any code
that allocates or uses `struct cgroup_root`.

No functional change is intended.

[1]: https://lore.kernel.org/all/20171017063322.11455-1-nick.desaulniers@gmail.com/T/
[2]: https://github.com/KSPP/linux/issues/79

Fixes: 743210386c0354a2f8ef3d697353c7d8477fa81d ("cgroup: use cgrp->kn->id as the cgroup ID")
Fixes: b11cfb5807e30333b36c02701382b820b7dcf0d5 ("cgroup: record ancestor IDs and reimplement cgroup_is_descendant() using it")
Cc: llvm@lists.linux.dev
Cc: Tejun Heo <tj@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alviro Iskandar Setiawan <alviro.iskandar@gmail.com>
---

Hello Tejun,

Sorry for bringing up an old discussion. Please reconsider this small
change, again...

Thx
 -- Viro

 include/linux/cgroup-defs.h | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 1bfcfb1af352..71cc244a1c0d 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -504,12 +504,6 @@ struct cgroup_root {
    /* Unique id for this hierarchy. */
    int hierarchy_id;

-   /* The root cgroup.  Root is destroyed on its release. */
-   struct cgroup cgrp;
-
-   /* for cgrp->ancestor_ids[0] */
-   u64 cgrp_ancestor_id_storage;
-
    /* Number of cgroups in the hierarchy, used only for /proc/cgroups */
    atomic_t nr_cgrps;

@@ -524,6 +518,25 @@ struct cgroup_root {

    /* The name for this hierarchy - may be empty */
    char name[MAX_CGROUP_ROOT_NAMELEN];
+
+   union {
+       /* The root cgroup.  Root is destroyed on its release. */
+       struct cgroup cgrp;
+
+       struct {
+           /* Preserve a space for cgrp. */
+           char __cgrp[sizeof(struct cgroup)];
+
+           /* Preserve a space for cgrp->ancestor_ids[0]. */
+           u64 cgrp_ancestor_id_storage;
+       };
+   };
+
+   /*
+    * Warning: struct cgroup contains a variable-sized array.
+    *
+    * Do not put anything below here!
+    */
 };

 /*

base-commit: 2bdfd2825c9662463371e6691b1a794e97fa36b4
-- 
2.32.0

[nathanchance]

Hi @alviroiskandar, thank you for the patch!

I am curious, under what conditions do you see this warning? It comes from -Wgnu, which we explicitly disable in the main Makefile because we build with -std=gnu89, which means we are going to take advantage of GNU extensions. While fixing this one warning is fine, we want to make sure that every part of the kernel has the same set of warnings disabled.

@kees and @GustavoARSilva might have some thoughts on the patch, as they have been trying to clean up the usage of flexible arrays throughout the kernel. One small comment off the bat, that comment at the end of the structure should probably be above the union, as that is a more standard location.

[alviroiskandar]

On Tue, Feb 8, 2022 at 11:57 PM Nathan Chancellor wrote:

Hi @alviroiskandar, thank you for the patch!

I am curious, under what conditions do you see this warning? It comes from -Wgnu, which we explicitly disable in the main Makefile because we build with -std=gnu89, which means we are going to take advantage of GNU extensions. While fixing this one warning is fine, we want to make sure that every part of the kernel has the same set of warnings disabled.

Ahh right, sorry.

The build is actually not relevant with the config provided from upstream, not sure if my patch is acceptable under that situation. If it is not, let's close this.

After reconsidering the condition, I think it seems it is not really reasonable since the upstream tree builds just fine. So my excuse "clang warns this" is not valid as we intentionally disable that warning.

@kees and @GustavoARSilva might have some thoughts on the patch, as they have been trying to clean up the usage of flexible arrays throughout the kernel. One small comment off the bat, that comment at the end of the structure should probably be above the union, as that is a more standard location.

I actually copied the comment style and sentence from sched.h.

https://github.com/torvalds/linux/blob/e6251ab4551f51fa4cee03523e08051898c3ce82/include/linux/sched.h#L1501-L1510

But I agree to have them be above the union, though. If this patch looks reasonable, I will spin the v2 with that change folded in.

Thx

-- Viro

[nickdesaulniers]

I think this issue is stale, but I'm not sure precisely that this has been fixed, or when. Looks a bit like #113 and #1101 which are both closed.

[nathanchance]

-Wgnu is disabled unconditionally in Makefile so it seems likely that this may have been caused by a cc-option failure.

[alviroiskandar]

Closing this as it's not reproducible anymore... It seems it was just a bad setup from my end.