[PATCH] cgroup: reorder flexible array members of struct cgroup_root

[PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Nick Desaulniers]

When compiling arch/x86/boot/compressed/eboot.c with HOSTCC=clang, the
following warning is observed:

./include/linux/cgroup-defs.h:391:16: warning: field 'cgrp' with
variable sized type 'struct cgroup' not at the end of a struct or class
is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct cgroup cgrp;
                      ^
Flexible array members are a C99 feature, but must be the last member of
a struct. Structs with flexible members composed in other structs must
also be the final members, unless using GNU C extensions.

struct cgroup_root's member cgrp is a struct cgroup, struct cgroup's
member ancestor_ids is a flexible member.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
Alternatively, we could:
* Not use flexible array members. The flexible array members and allocation
  strategy, added in commit b11cfb5807e30 mentions the hot path, so this is
  likely not an option?
* Disable this warning for HOSTCC==clang.  I'd rather not, since there's
  nothing really requiring the use of this particular GNU C extension here,
  or specific location of the member cgrp within struct cgroup_root AFAICT.

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

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index ade4a78a54c2..2ef256932bf3 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -387,9 +387,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] */
 	int cgrp_ancestor_id_storage;
 
@@ -410,6 +407,9 @@ struct cgroup_root {
 
 	/* The name for this hierarchy - may be empty */
 	char name[MAX_CGROUP_ROOT_NAMELEN];
+
+	/* The root cgroup.  Root is destroyed on its release. */
+	struct cgroup cgrp;
 };
 
 /*
-- 
2.11.0

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Nick Desaulniers]

On Mon, Oct 16, 2017 at 11:33:21PM -0700, Nick Desaulniers wrote:
> When compiling arch/x86/boot/compressed/eboot.c with HOSTCC=clang, the

Actually, not sure this is because HOSTCC was specifically clang, I
think that could be reworded to `When compiling ... with Clang, the
...`.

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Nick Desaulniers]

On Mon, Oct 16, 2017 at 11:40 PM, Nick Desaulniers
<nick.desaulniers@gmail.com> wrote:
> On Mon, Oct 16, 2017 at 11:33:21PM -0700, Nick Desaulniers wrote:
>> When compiling arch/x86/boot/compressed/eboot.c with HOSTCC=clang, the
>
> Actually, not sure this is because HOSTCC was specifically clang, I
> think that could be reworded to `When compiling ... with Clang, the
> ...`.

arch/x86/boot/compressed/Makefile:28 indeed overwrites KBUILD_CFLAGS,
so wording should be changed to as-suggested in my previous post,
since that means it's CC=clang, not HOSTCC=clang as the source of the
warning.

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Tejun Heo]

Hello,

On Mon, Oct 16, 2017 at 11:33:21PM -0700, Nick Desaulniers wrote:
> When compiling arch/x86/boot/compressed/eboot.c with HOSTCC=clang, the
> following warning is observed:
> 
> ./include/linux/cgroup-defs.h:391:16: warning: field 'cgrp' with
> variable sized type 'struct cgroup' not at the end of a struct or class
> is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>         struct cgroup cgrp;
>                       ^
> Flexible array members are a C99 feature, but must be the last member of
> a struct. Structs with flexible members composed in other structs must
> also be the final members, unless using GNU C extensions.
> 
> struct cgroup_root's member cgrp is a struct cgroup, struct cgroup's
> member ancestor_ids is a flexible member.

This is silly tho.  We know the the root group embedded there won't
have any ancestor_ids.  Also, in general, nothing prevents us from
doing something like the following.

	struct outer_struct {
		blah blah;
		struct inner_struct_with_flexible_array_member inner;
		unsigned long storage_for_flexible_array[NR_ENTRIES];
		blah blah;
	};

I think we should just silence the bogus warning.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Nick Desaulniers]

On Wed, Oct 18, 2017 at 6:30 AM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Oct 16, 2017 at 11:33:21PM -0700, Nick Desaulniers wrote:
>> When compiling arch/x86/boot/compressed/eboot.c with HOSTCC=clang, the
>> following warning is observed:
>>
>> ./include/linux/cgroup-defs.h:391:16: warning: field 'cgrp' with
>> variable sized type 'struct cgroup' not at the end of a struct or class
>> is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>>         struct cgroup cgrp;
>>                       ^
>> Flexible array members are a C99 feature, but must be the last member of
>> a struct. Structs with flexible members composed in other structs must
>> also be the final members, unless using GNU C extensions.
>>
>> struct cgroup_root's member cgrp is a struct cgroup, struct cgroup's
>> member ancestor_ids is a flexible member.
>
> This is silly tho.  We know the the root group embedded there won't
> have any ancestor_ids.

Sure, but struct cgroup_root is still declared as having a struct
cgroup not declared as the final member.

> Also, in general, nothing prevents us from
> doing something like the following.
>
>         struct outer_struct {
>                 blah blah;
>                 struct inner_struct_with_flexible_array_member inner;
>                 unsigned long storage_for_flexible_array[NR_ENTRIES];
>                 blah blah;
>         };

At that point, then why have the struct with flexible array member in
the first place?

>> or specific location of the member cgrp within struct cgroup_root AFAICT.
> I think we should just silence the bogus warning.

Is the order of the members actually important?  Otherwise it seems
that we're taking advantage of a GNU C extension for no real reason,
which is what I'm trying to avoid.  Please reconsider.

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Tejun Heo]

Hello, Nick.

On Fri, Oct 20, 2017 at 12:15:55AM -0700, Nick Desaulniers wrote:
> > This is silly tho.  We know the the root group embedded there won't
> > have any ancestor_ids.
> 
> Sure, but struct cgroup_root is still declared as having a struct
> cgroup not declared as the final member.

Why is that a problem tho?  We know that it doesn't have any flexible
array member so there's no storage allocated to it.

> > Also, in general, nothing prevents us from
> > doing something like the following.
> >
> >         struct outer_struct {
> >                 blah blah;
> >                 struct inner_struct_with_flexible_array_member inner;
> >                 unsigned long storage_for_flexible_array[NR_ENTRIES];
> >                 blah blah;
> >         };
> 
> At that point, then why have the struct with flexible array member in
> the first place?

Because there are different ways to use the struct?

> >> or specific location of the member cgrp within struct cgroup_root AFAICT.
> > I think we should just silence the bogus warning.
> 
> Is the order of the members actually important?  Otherwise it seems
> that we're taking advantage of a GNU C extension for no real reason,
> which is what I'm trying to avoid.  Please reconsider.

Here, not necessarily but I don't want to move it for a bogus reason.
Why would we disallow embedding structs with flexible members in the
middle when it can be done and is useful?  If we want to discuss
whether we want to avoid such usages in the kernel (but why?), sure,
let's have that discussion but we can't decide that on "clang warns on
it by default".

Thanks.

-- 
tejun

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Aleksa Sarai]

> Here, not necessarily but I don't want to move it for a bogus reason.
> Why would we disallow embedding structs with flexible members in the
> middle when it can be done and is useful?  If we want to discuss
> whether we want to avoid such usages in the kernel (but why?), sure,
> let's have that discussion but we can't decide that on "clang warns on
> it by default".

There was a talk a few years ago by the clang folks[1] saying that while 
trying to build a kernel with clang, they discovered that several places 
in the kernel uses "VLAIS" (variable Length Arrays In Structs") and 
argued that this is a violation of the C specification, despite it being 
a GNU extension. They also submitted several patches that removed this 
code (even working around a user-space visible usage of VLAIS).

[1]: 
https://www.linuxplumbersconf.org/2013/ocw/system/presentations/1221/original/VLAIS.pdf

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Tejun Heo]

On Sun, Oct 22, 2017 at 02:59:33AM +1100, Aleksa Sarai wrote:
> >Here, not necessarily but I don't want to move it for a bogus reason.
> >Why would we disallow embedding structs with flexible members in the
> >middle when it can be done and is useful?  If we want to discuss
> >whether we want to avoid such usages in the kernel (but why?), sure,
> >let's have that discussion but we can't decide that on "clang warns on
> >it by default".
> 
> There was a talk a few years ago by the clang folks[1] saying that
> while trying to build a kernel with clang, they discovered that
> several places in the kernel uses "VLAIS" (variable Length Arrays In
> Structs") and argued that this is a violation of the C
> specification, despite it being a GNU extension. They also submitted
> several patches that removed this code (even working around a
> user-space visible usage of VLAIS).

The kernel is explicitly using GNU extended version of C and has
always from the beginning, so not-std-c isn't a valid argument.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Nick Desaulniers]

On Sat, Oct 21, 2017 at 9:03 AM, Tejun Heo <tj@kernel.org> wrote:
> The kernel is explicitly using GNU extended version of C and has
> always from the beginning, so not-std-c isn't a valid argument.

Ok, how about that it looks like gcc7.1 starts treating this as an error?

https://godbolt.org/g/ivzPHZ

gcc6.3 looks like it starts trying to warn about this.

I don't have a local build of gcc7.1 to verify, but it seems like
setting -std=gnu99 doesn't help.

With Clang, it seems to be a warning that can be ignored via command
line flags, but doesn't look like that's possible for newer versions
of gcc, so this will have to be addressed at some point.

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Matthias Kaehlcke]

Hi Tejun,

El Sat, Oct 21, 2017 at 08:32:53AM -0700 Tejun Heo ha dit:

> Hello, Nick.
> 
> On Fri, Oct 20, 2017 at 12:15:55AM -0700, Nick Desaulniers wrote:
> > > This is silly tho.  We know the the root group embedded there won't
> > > have any ancestor_ids.
> > 
> > Sure, but struct cgroup_root is still declared as having a struct
> > cgroup not declared as the final member.
> 
> Why is that a problem tho?  We know that it doesn't have any flexible
> array member so there's no storage allocated to it.
> 
> > > Also, in general, nothing prevents us from
> > > doing something like the following.
> > >
> > >         struct outer_struct {
> > >                 blah blah;
> > >                 struct inner_struct_with_flexible_array_member inner;
> > >                 unsigned long storage_for_flexible_array[NR_ENTRIES];
> > >                 blah blah;
> > >         };
> > 
> > At that point, then why have the struct with flexible array member in
> > the first place?
> 
> Because there are different ways to use the struct?
> 
> > >> or specific location of the member cgrp within struct cgroup_root AFAICT.
> > > I think we should just silence the bogus warning.
> > 
> > Is the order of the members actually important?  Otherwise it seems
> > that we're taking advantage of a GNU C extension for no real reason,
> > which is what I'm trying to avoid.  Please reconsider.
> 
> Here, not necessarily but I don't want to move it for a bogus reason.
> Why would we disallow embedding structs with flexible members in the
> middle when it can be done and is useful?  If we want to discuss
> whether we want to avoid such usages in the kernel (but why?), sure,
> let's have that discussion but we can't decide that on "clang warns on
> it by default".

>From your earlier comment I understand that there is no problem in
this case because we know that cgroup_root->cgrp will always be
empty.

However in other instances the warning could point out actual errors
in the code, so I think it is good to have this warning generally
enabled. If cgroup_root was defined in a .c file we could consider to
disable the warning locally, but since the definition is in a header
that is widely included (indirectly through linux/cgroup.h and
net/sock.h) this doesn't seem to be an option.

Is there a good reason for the current position of cgrp within
cgroup_root? If there are no drawbacks in moving it to the end of
the struct I think Nick's patch is a reasonable solution.

Re: [PATCH] cgroup: reorder flexible array members of struct cgroup_root

[Tejun Heo]

Hello,

On Wed, Oct 25, 2017 at 02:54:23PM -0700, Matthias Kaehlcke wrote:
> From your earlier comment I understand that there is no problem in
> this case because we know that cgroup_root->cgrp will always be
> empty.
> 
> However in other instances the warning could point out actual errors
> in the code, so I think it is good to have this warning generally
> enabled. If cgroup_root was defined in a .c file we could consider to
> disable the warning locally, but since the definition is in a header
> that is widely included (indirectly through linux/cgroup.h and
> net/sock.h) this doesn't seem to be an option.
> 
> Is there a good reason for the current position of cgrp within
> cgroup_root? If there are no drawbacks in moving it to the end of
> the struct I think Nick's patch is a reasonable solution.

This all sounds really bogus to me.  Let's say we have something like
the following.

  struct flex_struct {
  	int array[];
  };

And the following two usages.

1.

  struct flex_struct *fs =
	kmalloc(sizeof(struct flex_struct) + N * sizeof(int));

2.

  struct enclosing_struct es {
	struct flex_struct fs;
	int fs_array_storage[N];
  };

  struct enclosing_struct *es =
	kmalloc(sizeof(struct enclosing_struct));

So, you're saying #1 is okay but #2 is not, which is just silly.  The
compiler can't warn correctly about flex array members whether they're
embedded or not.  Nothing prevents somebody accessing beyond N in #1
either.

This effort seems really pointless to me.  Let's please not waste any
more bandwidth on this.

Thanks.

-- 
tejun