[GIT PULL] percpu fix for v5.9-rc6

[GIT PULL] percpu fix for v5.9-rc6

[Dennis Zhou]

Hi Linus,

This is a fix for the first chunk size calculation where the variable
length array incorrectly used # of longs instead of bytes of longs. This
came in as a code fix and not a bug report, so I don't think it was
widely problematic. I believe it worked out due to it being memblock
memory and alignment requirements working in our favor.

Thanks,
Dennis

The following changes since commit f75aef392f869018f78cfedf3c320a6b3fcfda6b:

  Linux 5.9-rc3 (2020-08-30 16:01:54 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu.git for-5.9-fixes

for you to fetch changes up to b3b33d3c43bbe0177d70653f4e889c78cc37f097:

  percpu: fix first chunk size calculation for populated bitmap (2020-09-17 17:34:39 +0000)

----------------------------------------------------------------
Sunghyun Jin (1):
      percpu: fix first chunk size calculation for populated bitmap

 mm/percpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index f4709629e6de..1ed1a349eab8 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1316,7 +1316,7 @@ static struct pcpu_chunk * __init pcpu_alloc_first_chunk(unsigned long tmp_addr,
 
 	/* allocate chunk */
 	alloc_size = sizeof(struct pcpu_chunk) +
-		BITS_TO_LONGS(region_size >> PAGE_SHIFT);
+		BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(unsigned long);
 	chunk = memblock_alloc(alloc_size, SMP_CACHE_BYTES);
 	if (!chunk)
 		panic("%s: Failed to allocate %zu bytes\n", __func__,

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Thu, Sep 17, 2020 at 1:45 PM Dennis Zhou <dennis@kernel.org> wrote:
>
>
> diff --git a/mm/percpu.c b/mm/percpu.c
> index f4709629e6de..1ed1a349eab8 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -1316,7 +1316,7 @@ static struct pcpu_chunk * __init pcpu_alloc_first_chunk(unsigned long tmp_addr,
>
>         /* allocate chunk */
>         alloc_size = sizeof(struct pcpu_chunk) +
> -               BITS_TO_LONGS(region_size >> PAGE_SHIFT);
> +               BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(unsigned long);

Hmm.

Wouldn't this be cleaner as

        alloc_size =struct_size(chunk, populated,
                 BITS_TO_LONGS(region_size >> PAGE_SHIFT) );

and looking at this, I realize that I thought we enabled warnings for
'sizeof()' of flexible array structures to avoid these kinds of
mistakes, but that must clearly have happened only in a dream of mine.

Anyway, pulled.

            Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Gustavo A. R. Silva]

On Thu, Sep 17, 2020 at 06:05:13PM -0700, Linus Torvalds wrote:
> On Thu, Sep 17, 2020 at 1:45 PM Dennis Zhou <dennis@kernel.org> wrote:
> >
> >
> > diff --git a/mm/percpu.c b/mm/percpu.c
> > index f4709629e6de..1ed1a349eab8 100644
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> > @@ -1316,7 +1316,7 @@ static struct pcpu_chunk * __init pcpu_alloc_first_chunk(unsigned long tmp_addr,
> >
> >         /* allocate chunk */
> >         alloc_size = sizeof(struct pcpu_chunk) +
> > -               BITS_TO_LONGS(region_size >> PAGE_SHIFT);
> > +               BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(unsigned long);
> 
> Hmm.
> 
> Wouldn't this be cleaner as
> 
>         alloc_size =struct_size(chunk, populated,
>                  BITS_TO_LONGS(region_size >> PAGE_SHIFT) );

Yeah; the above is much better. Please, use that helper.

> and looking at this, I realize that I thought we enabled warnings for
> 'sizeof()' of flexible array structures to avoid these kinds of
> mistakes, but that must clearly have happened only in a dream of mine.

If you were to try to apply the sizeof() operator to the flexible-array member
alone: sizeof(chunk->populated); you would get a warning because such arrays
have incomplete type, see below:

mm/percpu.c: In function ‘pcpu_alloc_first_chunk’:
mm/percpu.c:1320:52: error: invalid application of ‘sizeof’ to incomplete type ‘long unsigned int[]’
 1320 |   BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(chunk->populated);
      |                                                    ^

However, in this case, sizeof() is being applied to the object type, which doesn't
cause a warning, but still is an error-prone coding practice. For instance, this
is the bugfix[1], for a 4-year old bug introduced by the combination of weak code
and this commit[2]. This bug could have been prevented by either adopting better
coding practices or through the use[3] of the recent struct_size() helper.

So please, whenever you can use it, do so. :)

Thanks
--
Gustavo

[1] https://git.kernel.org/linus/cffaaf0c816238c45cd2d06913476c83eb50f682
[2] https://git.kernel.org/linus/57384592c43375d2c9a14d82aebbdc95fdda9e9d
[3] https://git.kernel.org/linus/553d66cb1e8667aadb57e3804775c5ce1724a49b

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 9:17 AM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> This bug could have been prevented by either adopting better
> coding practices or through the use[3] of the recent struct_size() helper.

Well, my unspoken point was that coding practices are just
theoretical. Coding practices don't help - actual *checking* of them
helps.

I realize that structures with flexible-array member are allowed to
use sizeof() in standard C, but if we want to make sure this doesn't
happen, we would need to have a stricter model than that. But a quick
google didn't find any flag to enable such a stricter mode.

I guess a sparse warning would work, but sparse already has too many
warnings and as a result most people don't care - even if they were to
run sparse in the first place.

Is there some gcc option that I didn't find to help find any questionable cases?

Because if we have a coding practice that you should use
'struct_size()', then we should also have a way to _verify_ that.

The whole - and really ONLY - point of using flexible arrays was that
it would protect against these things. And as things are now, it
simply doesn't. It's not an actual improvement over just using a
zero-sized array.

(Slightly related: copying a struct has the exact same issue. A
flexible array is no better than a zero-sized array, and generates the
same code and the same lack of any warnings, afaik).

               Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Gustavo A. R. Silva]

On Fri, Sep 18, 2020 at 10:23:54AM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 9:17 AM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > This bug could have been prevented by either adopting better
> > coding practices or through the use[3] of the recent struct_size() helper.
> 
> Well, my unspoken point was that coding practices are just
> theoretical. Coding practices don't help - actual *checking* of them
> helps.

Yep; I agree that the best approach is to find a way to enforce such
practices. :)

> I realize that structures with flexible-array member are allowed to
> use sizeof() in standard C, but if we want to make sure this doesn't
> happen, we would need to have a stricter model than that. But a quick
> google didn't find any flag to enable such a stricter mode.
> 

OK. It seems that we are talking about two different things here. One thing
is to apply sizeof() to a structure that contains a flexible-array member.
And the other thing is to apply sizeof() to a flexible array. The former
is allowed, the latter is wrong and we already get a build error when that
occurs.

Applying sizeof() to a structure containing a flex-array member is allowed,
and the result is the size of the structure excluding the size of the
flexible-array member type. In this regard, using a zero-length array has
the same effect as using a flexible-array member.

Now, if you are trying to make the case for not allowing the application
of sizeof() to a structure that contains a flexible-array member because
that could be prone to error, then this is new and we need to think about
the implications and see if we can move in that direction.

> I guess a sparse warning would work, but sparse already has too many
> warnings and as a result most people don't care - even if they were to
> run sparse in the first place.

There are ongoing efforts to warn about the use of zero-length and one-element
arrays through the use of Coccinelle. But again, you might be talking
about a different thing...

> Is there some gcc option that I didn't find to help find any questionable cases?

If the questionable case is the application of sizeof() to a flex-array
member or a flex-array member not occuring last in the containing structure,
then yes, GCC already generates a build error for both cases. And that's
what we want, see at the bottom...

> Because if we have a coding practice that you should use
> 'struct_size()', then we should also have a way to _verify_ that.

struct_size() should be used to defend against these sorts of bugs[1] as I
explained in the last email[2], also to prevent integer overflows. It's meant
to replace the following sorts of idioms:

sizeof(struct foo) + sizeof(struct boo) * COUNT
sizeof(*foo) + sizeof(*boo) * COUNT
sizeof(*foo) + sizeof(*foo->array) * COUNT
sizeof(*foo) + sizeof(foo->array[0]) * COUNT

and all the many variations...

But struct_size() by itself is not meant to help identify any kind of array
misuses in structures. struct_size() can be used with structures that contain
any kind of arrays (zero-length/one-element/flexible array).

> The whole - and really ONLY - point of using flexible arrays was that
> it would protect against these things. And as things are now, it
> simply doesn't. It's not an actual improvement over just using a
> zero-sized array.

It protects against the cases I have explained at the top and that I elaborate
below...

> (Slightly related: copying a struct has the exact same issue. A
> flexible array is no better than a zero-sized array, and generates the
> same code and the same lack of any warnings, afaik).

An important difference is that the use of a flexible-array member allows
the compiler to generate errors when the flexible array does not occur last
in the structure, which helps to prevent some kind of undefined behavior bugs[3]
from being inadvertently introduced to the codebase. It also allows the compiler
to correctly analyze array sizes (via sizeof(), CONFIG_FORTIFY_SOURCE, and
CONFIG_UBSAN_BOUNDS). For instance, there is no mechanism that warns us that
the following application of the sizeof() operator to a zero-length array always
results in zero:

struct something {
	size_t count;
	struct foo items[0];
};

struct something *instance;

instance = kmalloc(struct_size(instance, items, count), GFP_KERNEL);
instance->count = count;

size = sizeof(instance->items) * instance->count;
memcpy(instance->items, source, size);

At the last line of code above, size turns out to be zero, when one might have
thought it represents the total size in bytes of the dynamic memory recently
allocated for the trailing array items. Here are a couple examples of this
issue[4][5]. Instead, flexible array members have incomplete type, and so the
sizeof() operator may not be applied, so any misuse of such operators will be
immediately noticed at build time.

We have documented all the above and more here[6], and there is an update waiting
to land in mainline here[7].

But again, you might be more concerned about applying sizeof() to a structure
containing a flexible-array member in the first place, rather than applying it
to the member alone...?

Thanks
--
Gustavo

[1] https://git.kernel.org/linus/cffaaf0c816238c45cd2d06913476c83eb50f682
[2] https://lore.kernel.org/lkml/20200918162305.GB25599@embeddedor/
[3] https://git.kernel.org/linus/76497732932f15e7323dc805e8ea8dc11bb587cf
[4] https://git.kernel.org/linus/f2cd32a443da694ac4e28fbf4ac6f9d5cc63a539
[5] https://git.kernel.org/linus/ab91c2a89f86be2898cee208d492816ec238b2cf
[6] https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
[7] https://lore.kernel.org/lkml/20200901010949.GA21398@embeddedor/

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 12:28 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> OK. It seems that we are talking about two different things here. One thing
> is to apply sizeof() to a structure that contains a flexible-array member.
> And the other thing is to apply sizeof() to a flexible array. The former
> is allowed, the latter is wrong and we already get a build error when that
> occurs.

The latter I'm not even interested in, it's such a pointless thing to do.

> Applying sizeof() to a structure containing a flex-array member is allowed,

Yes, and that's wrong and inconsistent, but what else is new about the
C standard. It's what allows these kinds of bugs to slip through.

I sent Luc a couple of examples in the hope that maybe sparse could do
better, but..

> > Is there some gcc option that I didn't find to help find any questionable cases?
>
> If the questionable case is the application of sizeof() to a flex-array
> member or a flex-array member not occuring last in the containing structure,
> then yes, GCC already generates a build error for both cases. And that's
> what we want, see at the bottom...

No.

The questionable thing is to do "sizeof(struct-with-flex-array)".

The point is, it's returning the same thing as if it was just a
zero-sized array, which makes the whole flex array entirely pointless
from a type safety standpoint.

The *only* thing it protects against is the "must be at the end" case,
which is almost entirely pointless and uninteresting.

Yeah, we've had that bug too, but that doesn't make it very interesting.

              Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Matthew Wilcox]

On Fri, Sep 18, 2020 at 12:37:48PM -0700, Linus Torvalds wrote:
> > Applying sizeof() to a structure containing a flex-array member is allowed,
> 
> Yes, and that's wrong and inconsistent, but what else is new about the
> C standard. It's what allows these kinds of bugs to slip through.

Hmm.  We actually do that in our implementation of struct_size()

#define struct_size(p, member, count)                                   \
        __ab_c_size(count,                                              \
                    sizeof(*(p)->member) + __must_be_array((p)->member),\
                    sizeof(*(p)))

I suppose it's not really necessary, we could do offsetof here, right?

#define struct_size(p, member, count)                                   \
        __ab_c_size(count,                                              \
                    sizeof(*(p)->member) + __must_be_array((p)->member),\
                    offsetof(typeof(*(p)), member))

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 1:02 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> I suppose it's not really necessary, we could do offsetof here, right?

Yup, that would make a lot more sense.

But right now, the sizeof() obviously silently works.

As do a number of other fairly nonsensical things, like assigning a struct etc.

And yes, I realize we may well do that too. But I think that's a
dangerous pattern too, ie doing

   *a = *b;

silently works, and copies everything but the final array.

And yes - none of this is _worse_ than using zero-sized arrays, but
the point is that it isn't better either.

            Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Arvind Sankar]

On Fri, Sep 18, 2020 at 01:14:54PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 1:02 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > I suppose it's not really necessary, we could do offsetof here, right?
> 
> Yup, that would make a lot more sense.
> 
> But right now, the sizeof() obviously silently works.

In general (i.e. outside the implementation of the macro itself), what
is the preferred way of getting the size of just the header?
  1) offsetof(typeof(s),flex)
  2) struct_size(s, flex, 0)
  3) sizeof(s)
  4) new macro that's easier to read than 1 or 2, but makes it clear
     what you're doing?

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 1:29 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> In general (i.e. outside the implementation of the macro itself), what
> is the preferred way of getting the size of just the header?
>   1) offsetof(typeof(s),flex)
>   2) struct_size(s, flex, 0)

I think those two should end up being equivalent.

>   3) sizeof(s)

This works right now, but exactly *because* it works, we're not seeing
the questionable cases.

Of course, _also_ exactly because it just silently works, I also don't
know if there may be thousands of perfectly fine uses where people
really do want the header, and a "sizeof()" is simpler than
alternatives 1-2.

It's possible that there really are a lot of "I want to know just the
header size" cases. It sounds odd, but I could _imagine_ situations
like that, even though no actual case comes to mind.

>   4) new macro that's easier to read than 1 or 2, but makes it clear
>      what you're doing?

I don't think this would have any real advantage, would it?

Now what might be good is if we can make "struct_size()" also actually
verify that the member that is passed in is that last non-sized
member. I'm not sure how to do that.

I know how to check that it's *not* that last unsized member (just do
"sizeof(s->flex)", and it should error), but I don't see how to assert
the reverse of that).

Because that kind of "yes, we actually pass in the right member" check
would be good to have too.

              Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Arvind Sankar]

On Fri, Sep 18, 2020 at 01:40:44PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 1:29 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > In general (i.e. outside the implementation of the macro itself), what
> > is the preferred way of getting the size of just the header?
> >   1) offsetof(typeof(s),flex)
> >   2) struct_size(s, flex, 0)
> 
> I think those two should end up being equivalent.

Yeah, but it would be good to standardize on one of them.

> 
> >   3) sizeof(s)
> 
> This works right now, but exactly *because* it works, we're not seeing
> the questionable cases.
> 
> Of course, _also_ exactly because it just silently works, I also don't
> know if there may be thousands of perfectly fine uses where people
> really do want the header, and a "sizeof()" is simpler than
> alternatives 1-2.
> 
> It's possible that there really are a lot of "I want to know just the
> header size" cases. It sounds odd, but I could _imagine_ situations
> like that, even though no actual case comes to mind.

I'm asking because I just added an instance of (3) and want to know if I
should change it :)

The case was when you have a function that got passed a pointer and a
size, and wants to verify that the size covers the structure before
accessing its fields. If the function only needs the "fixed" fields, it
feels a little unnatural to use (1) or (2) when the flex member is
otherwise not going be accessed at all.

> 
> >   4) new macro that's easier to read than 1 or 2, but makes it clear
> >      what you're doing?
> 
> I don't think this would have any real advantage, would it?

The advantage is documenting that you do mean the header size, i.e.
something like struct_header_size(s).

> 
> Now what might be good is if we can make "struct_size()" also actually
> verify that the member that is passed in is that last non-sized
> member. I'm not sure how to do that.
> 
> I know how to check that it's *not* that last unsized member (just do
> "sizeof(s->flex)", and it should error), but I don't see how to assert
> the reverse of that).
> 
> Because that kind of "yes, we actually pass in the right member" check
> would be good to have too.
> 
>               Linus

You could just assert that offsetof(typeof(s),flex) == sizeof(s), no? It
would also make sure that someone doesn't try to use struct_size() with
a 1-sized array member.

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 2:00 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> You could just assert that offsetof(typeof(s),flex) == sizeof(s), no?

No, because the whole point is that I want that "sizeof(s)" to *WARN*.

It's a nonsensical thing to do. That 's' has no statically known size.

The C standard is being very confused here, in that it tries to claim
that the flexible arrays are somehow fundamentally different from a
zero-sized one. But then it acts as if they are exactly the same wrt
sizeof() and structure copies.

It should warn, exactly because right now it causes potential bugs
like the one that started this thread.

You can't have both "zero-sized arrays are bad and shouldn't be used"
and "flexible arrays are good, and work exactly like zero-sized
arrays".

Either zero-sized arrays are bad or they aren't. And if they are bad,
then flexible arrays shouldn't work *exactly* like them apart from
some UBSAN warnings.

See my point?

             Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Arvind Sankar]

On Fri, Sep 18, 2020 at 02:18:20PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 2:00 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > You could just assert that offsetof(typeof(s),flex) == sizeof(s), no?
> 
> No, because the whole point is that I want that "sizeof(s)" to *WARN*.
> 
> It's a nonsensical thing to do. That 's' has no statically known size.
> 
> The C standard is being very confused here, in that it tries to claim
> that the flexible arrays are somehow fundamentally different from a
> zero-sized one. But then it acts as if they are exactly the same wrt
> sizeof() and structure copies.
> 
> It should warn, exactly because right now it causes potential bugs
> like the one that started this thread.
> 
> You can't have both "zero-sized arrays are bad and shouldn't be used"
> and "flexible arrays are good, and work exactly like zero-sized
> arrays".
> 
> Either zero-sized arrays are bad or they aren't. And if they are bad,
> then flexible arrays shouldn't work *exactly* like them apart from
> some UBSAN warnings.
> 
> See my point?
> 
>              Linus

Ouch, offsetof() and sizeof() will give different results in the
presence of alignment padding.

https://godbolt.org/z/rqnxTK

I think, grepping at random, that at least struct scsi_vpd is like this,
size is 24 but data[] starts at offset 20.

	struct scsi_vpd {
		struct rcu_head	rcu;
		int		len;
		unsigned char	data[];
	};

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 3:40 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Ouch, offsetof() and sizeof() will give different results in the
> presence of alignment padding.

Indeed. But from an allocation standpoint, the offsetof()+size is I
think the correct size. The padding at the end makes very little sense
for something like "struct_size()".

Padding at the end is required for sizeof() for a very simple reason:
arrays.  The "sizeof()" needs to be aligned to the alignment of the
entry, because if it isn't, then the standard C array traversal
doesn't work.

But you cannot sanely have arrays of these structures of variable size
entries either - even if standard C cheerfully allows you to declare
them (again: it will not behave like a variable sized array, it will
behave like a zero-sized one).

That was in fact one of the test-cases that I submitted to the sparse
list - the insanity of allowing arrays of structures that have a
flexible array at the end is just the C standard being confused. The C
standard may allow it, but I don't think we should allow it in the
kernel.

Oh, I can see why somebody would want to have an array of those things
- exactly because they want to have some "initializer _without_ the
flexible array part", and they actually don't want that variably-sized
array at all for that case.

But I'm pretty sure we really really don't want that kind of oddities
in the kernel. If we really want a separate "struct head_struct", then
I think we should do so explicitly, and have something like

    struct real_struct {
        // Unnamed head struct here
        struct head_struct {
                    ,,,,
        };
        unsigned int variable_array[];
    };

and if you want the part without the flexible array at the end, then
you use that "struct head_struct".

Instead of depending on the imho broken model of the C standard that
says "in lots of cases, we'll just silently make that flexible array
be a zero-sized one".

            Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Arvind Sankar]

On Fri, Sep 18, 2020 at 06:28:30PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 3:40 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Ouch, offsetof() and sizeof() will give different results in the
> > presence of alignment padding.
> 
> Indeed. But from an allocation standpoint, the offsetof()+size is I
> think the correct size. The padding at the end makes very little sense
> for something like "struct_size()".

I just meant that my suggestion doesn't actually work to assert that you
passed in the flexible array member to struct_size(), even outside of
any future warnings on sizeof().

And that it's another source of subtle bugs, although you'll err towards
over-allocating memory rather than under-allocating by using sizeof().

Is it ever necessary to allocate _at least_ sizeof() even if
offsetof()+size is smaller?

> 
> Padding at the end is required for sizeof() for a very simple reason:
> arrays.  The "sizeof()" needs to be aligned to the alignment of the
> entry, because if it isn't, then the standard C array traversal
> doesn't work.
> 
> But you cannot sanely have arrays of these structures of variable size
> entries either - even if standard C cheerfully allows you to declare
> them (again: it will not behave like a variable sized array, it will
> behave like a zero-sized one).

I think you can't do this in standard C. It's a GCC extension.

	A structure containing a flexible array member, or a union
	containing such a structure (possibly recursively), may not be a
	member of a structure or an element of an array. (However, these
	uses are permitted by GCC as extensions.)

Re: [GIT PULL] percpu fix for v5.9-rc6

[Matthew Wilcox]

On Fri, Sep 18, 2020 at 10:53:36PM -0400, Arvind Sankar wrote:
> I think you can't do this in standard C. It's a GCC extension.
> 
> 	A structure containing a flexible array member, or a union
> 	containing such a structure (possibly recursively), may not be a
> 	member of a structure or an element of an array. (However, these
> 	uses are permitted by GCC as extensions.)

I actually have a patch in the works which wants to do this.

 struct pagevec {
-       unsigned char nr;
-       bool percpu_pvec_drained;
-       struct page *pages[PAGEVEC_SIZE];
+       union {
+               struct {
+                       unsigned char sz;
+                       unsigned char nr;
+                       bool percpu_pvec_drained;
+                       struct page *pages[];
+               };
+               void *__p[PAGEVEC_SIZE + 1];
+       };
 };

I don't think ANSI C permits this, but it's useful to be able to declare
a pagevec on the stack and be guaranteed to get enough memory to hold
a useful sized array of pointers (as well as be able to dynamically
allocate a larger pagevec for the cases which want such a thing).

We could certainly split pagevec into a variable length array version
and have a struct stack_pagevec which had the extra padding, but that
involves changing a lot more code.

Re: [GIT PULL] percpu fix for v5.9-rc6

[Linus Torvalds]

On Fri, Sep 18, 2020 at 7:53 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Is it ever necessary to allocate _at least_ sizeof() even if
> offsetof()+size is smaller?

Not that I can tell.

Obviously all allocators tend to have their own alignment concerns, so
they'll all align things up internally anyway.

But why would the alignment of the earlier members of the structure
have anything to do with the size of it? That's nonsensical outside of
the array situation, I feel.

Of course, maybe somebody has such a case: an "array of structures,
each with the same size of flexible array member". And then you _do_
want to align all those entries.

But honestly, once you start doing things like that, why would you
only have one single structure type, much less just one single size of
that flexible array? If you lay out these variably-sized things in
memory each after each other, maybe you lay out multiple different
_kinds_ of variably sized structures?

So there are lots of reasons to want alignment at the end, but why
would the alignment be the same as the beginning of that particular
type?

That said, in the kernel, this probably practically never really
matters. Because typically, our allocation alignment tends to be
bigger than any individual structure type alignment anyway.

So it's probably all moot. The difference between using "sizeof()" and
"offsetof()" is in the noise and not important per se.

No, the real reason I would advocate using 'offsetof()' is really just
that I'd rather have 'sizeof()' cause a warning.

> I think you can't do this in standard C. It's a GCC extension.
>
>         A structure containing a flexible array member, or a union
>         containing such a structure (possibly recursively), may not be a
>         member of a structure or an element of an array. (However, these
>         uses are permitted by GCC as extensions.)

Ahh.

But I'm pretty sure the 'sizeof()' thing is actually the standard, not gcc.

Arrays of those things is odd, and apparently the standard got that
right. Good (but I think it also means that allowing sizeof() makes
even less sense).

            Linus

Re: [GIT PULL] percpu fix for v5.9-rc6

[Matthew Wilcox]

On Fri, Sep 18, 2020 at 06:39:57PM -0400, Arvind Sankar wrote:
> On Fri, Sep 18, 2020 at 02:18:20PM -0700, Linus Torvalds wrote:
> > On Fri, Sep 18, 2020 at 2:00 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > You could just assert that offsetof(typeof(s),flex) == sizeof(s), no?
> > 
> > No, because the whole point is that I want that "sizeof(s)" to *WARN*.
> 
> Ouch, offsetof() and sizeof() will give different results in the
> presence of alignment padding.
> 
> https://godbolt.org/z/rqnxTK

We really should be using offsetof() then.  It's harmless because we're
currently overallocating, not underallocating.  The test case I did was:

struct s {
        int count;
        char *p[];
};

struct_size(&s, p, 5); (48 bytes)
struct_size2(&s, p, 5); (also 48 bytes)

struct_size2 uses offsetof instead of sizeof.

Your case is different because the chars fit in the padding at the end
of the struct.

Re: [GIT PULL] percpu fix for v5.9-rc6

[Arvind Sankar]

On Sat, Sep 19, 2020 at 03:45:56AM +0100, Matthew Wilcox wrote:
> On Fri, Sep 18, 2020 at 06:39:57PM -0400, Arvind Sankar wrote:
> > On Fri, Sep 18, 2020 at 02:18:20PM -0700, Linus Torvalds wrote:
> > > On Fri, Sep 18, 2020 at 2:00 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > You could just assert that offsetof(typeof(s),flex) == sizeof(s), no?
> > > 
> > > No, because the whole point is that I want that "sizeof(s)" to *WARN*.
> > 
> > Ouch, offsetof() and sizeof() will give different results in the
> > presence of alignment padding.
> > 
> > https://godbolt.org/z/rqnxTK
> 
> We really should be using offsetof() then.  It's harmless because we're
> currently overallocating, not underallocating.  The test case I did was:
> 

I wonder if there are cases where we know the total size, and are
working out the number of elements in the flexible array by doing
size - sizeof(s).

Would a macro to do the inverse of struct_size(), i.e. get the count
knowing the total size be useful?

RE: [GIT PULL] percpu fix for v5.9-rc6

[David Laight]

From: Arvind Sankar
> Sent: 18 September 2020 23:40
..
> Ouch, offsetof() and sizeof() will give different results in the
> presence of alignment padding.
> 
> https://godbolt.org/z/rqnxTK
> 
> I think, grepping at random, that at least struct scsi_vpd is like this,
> size is 24 but data[] starts at offset 20.
> 
> 	struct scsi_vpd {
> 		struct rcu_head	rcu;
> 		int		len;
> 		unsigned char	data[];
> 	};

For another standards 'brain-fart' consider:
	x = malloc(offsetof(struct scsi_vpd, data[count]));

Since offsetof() is defined to return a compile-time constant
(hi Microsoft) this is illegal unless 'count' is also a
compile-time constant.
(It ought to be defined to be constant if the field is constant.)

If count < 4 then *x = *y will also write past the end of x.
Such structure assignments should be compile-time errors.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [GIT PULL] percpu fix for v5.9-rc6

[Gustavo A. R. Silva]

On Fri, Sep 18, 2020 at 12:37:48PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 12:28 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > OK. It seems that we are talking about two different things here. One thing
> > is to apply sizeof() to a structure that contains a flexible-array member.
> > And the other thing is to apply sizeof() to a flexible array. The former
> > is allowed, the latter is wrong and we already get a build error when that
> > occurs.
> 
> The latter I'm not even interested in, it's such a pointless thing to do.
> 
> > Applying sizeof() to a structure containing a flex-array member is allowed,
> 
> Yes, and that's wrong and inconsistent, but what else is new about the
> C standard. It's what allows these kinds of bugs to slip through.
> 
> I sent Luc a couple of examples in the hope that maybe sparse could do
> better, but..
> 
> > > Is there some gcc option that I didn't find to help find any questionable cases?
> >
> > If the questionable case is the application of sizeof() to a flex-array
> > member or a flex-array member not occuring last in the containing structure,
> > then yes, GCC already generates a build error for both cases. And that's
> > what we want, see at the bottom...
> 
> No.
> 
> The questionable thing is to do "sizeof(struct-with-flex-array)".

I see now... 

> The point is, it's returning the same thing as if it was just a
> zero-sized array, which makes the whole flex array entirely pointless
> from a type safety standpoint.
> 
> The *only* thing it protects against is the "must be at the end" case,
> which is almost entirely pointless and uninteresting.
> 

But you are missing the point about CONFIG_UBSAN_BOUNDS, which doesn't
work with zero-lenght and one-element arrays. And we want to be able
to use that configuration. That's the main reason why we are replacing
those arrays with a flexible one. I should have made more emphasis on
that point in my last response.

Thanks
--
Gustavo

Re: [GIT PULL] percpu fix for v5.9-rc6

[pr-tracker-bot]

The pull request you sent on Thu, 17 Sep 2020 20:45:14 +0000:

> git://git.kernel.org/pub/scm/linux/kernel/git/dennis/percpu.git for-5.9-fixes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/10b82d5176488acee2820e5a2cf0f2ec5c3488b6

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html