| Message ID | 20200917204514.GA2880159@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] percpu fix for v5.9-rc6 | expand |
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
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!
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
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
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/
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
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))
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
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
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?
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
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.
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
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[]; };
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
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.
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.)
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.
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
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?
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)
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__,
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(-)