| Message ID | Y0IFEUjwXGZFf7bB@mail.google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,next] dlm: Replace one-element array with flexible-array member | expand |
On October 8, 2022 4:17:37 PM PDT, Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote: >One-element arrays are deprecated, and we are replacing them with >flexible array members instead. So, replace one-element array with >flexible-array member in struct dlm_ls, and refactor the rest of the >code, accordingly. Thanks for working on this! > >We strive to make changes that don't produce any before/after binary >output differeces as that makes it easier for the reviewer to accept the >patch. In this particular instance, it wasn't possible to achieve this >due to the fact that the ls_name[1] size, which is used as the >NUL-terminator space, was hidden within the struct's padding as shown >below. > >$ diff <(objdump -M intel -j .text -D dlm.old) <(objdump -M intel -j >.text -D dlm.new) I'd suggest different options here, this is harder to map back to the source line. See https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/ for lots of details. :) > >13778c13778 >< c693: 49 8d bc 24 c0 08 00 lea rdi,[r12+0x8c0] >--- >> c693: 49 8d bc 24 c1 08 00 lea rdi,[r12+0x8c1] This implies something unexpected changed. > >$ pahole dlm.old -C dlm_ls >... > int ls_namelen; /* 2232 4 */ > char ls_name[1]; /* 2236 1 */ > > /* size: 2240, cachelines: 35, members: 90 */ > /* sum members: 2166, holes: 17, sum holes: 71 */ > /* padding: 3 */ > /* paddings: 3, sum paddings: 17 */ > /* forced alignments: 1 */ >} __attribute__((__aligned__(8))); > >$ pahole dlm.new -C dlm_ls >... > int ls_namelen; /* 2232 4 */ > char ls_name[]; /* 2236 0 */ > > /* size: 2240, cachelines: 35, members: 90 */ > /* sum members: 2165, holes: 17, sum holes: 71 */ > /* padding: 4 */ > /* paddings: 3, sum paddings: 17 */ > /* forced alignments: 1 */ >} __attribute__((__aligned__(8))); This has trailing padding, so the struct size didn't actually change. >This helps with the ongoing efforts to tighten the FORTIFY_SOURCE >routines on memcpy() and help us make progress towards globally >enabling -fstrict-flex-arrays=3 [1]. > >Link: https://github.com/KSPP/linux/issues/79 >Link: https://github.com/KSPP/linux/issues/228 >Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > >Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> >--- >My apologies for v2, there was an accidental <Cr> I added on >the CC line which messed up the body of my previus email. > >This patch sets it right. > >--- > fs/dlm/dlm_internal.h | 2 +- > fs/dlm/lockspace.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > >diff --git a/fs/dlm/dlm_internal.h b/fs/dlm/dlm_internal.h >index e34c3d2639a5..ce2e32ed2ede 100644 >--- a/fs/dlm/dlm_internal.h >+++ b/fs/dlm/dlm_internal.h >@@ -670,7 +670,7 @@ struct dlm_ls { > void *ls_ops_arg; > > int ls_namelen; >- char ls_name[1]; >+ char ls_name[]; > }; > > /* >diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c >index bae050df7abf..c3a36f3197da 100644 >--- a/fs/dlm/lockspace.c >+++ b/fs/dlm/lockspace.c >@@ -473,7 +473,7 @@ static int new_lockspace(const char *name, const char *cluster, > > error = -ENOMEM; > >- ls = kzalloc(sizeof(struct dlm_ls) + namelen, GFP_NOFS); >+ ls = kzalloc(sizeof(struct dlm_ls) + namelen + 1, GFP_NOFS); This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. I would expect the correct allocation size to be: offsetof(typeof(*ls), ls_name) + namelen Question, though: is ls_name _expected_ to be %NUL terminated, and was the prior 3 bytes of extra allocation accidentally required? -Kees
On Sat, Oct 08, 2022 at 05:18:35PM -0700, Kees Cook wrote: > >$ diff <(objdump -M intel -j .text -D dlm.old) <(objdump -M intel -j > >.text -D dlm.new) > > I'd suggest different options here, this is harder to map back to the source line. > See https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/ > for lots of details. :) > Just read the blog entry, it's really interesting. I will be using it from now onwards :) > > > >13778c13778 > >< c693: 49 8d bc 24 c0 08 00 lea rdi,[r12+0x8c0] > >--- > >> c693: 49 8d bc 24 c1 08 00 lea rdi,[r12+0x8c1] > > This implies something unexpected changed. > I will add more details about this line at the other point you made below to avoid repeating myself. But to cut a long story, short.. this [reg + displacement + 1] difference is caused because I deliberately add the NUL-terminator space to the kzalloc() call. > This has trailing padding, so the struct size didn't actually change. > > >- ls = kzalloc(sizeof(struct dlm_ls) + namelen, GFP_NOFS); > >+ ls = kzalloc(sizeof(struct dlm_ls) + namelen + 1, GFP_NOFS); > > This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. That's true. I agree that leaving "+ 1" would work and produce a no-binary-changes patch due to the existing padding that the structure has. OTOH, I thought that relying on that space could bite us in the future if anyone tweaks the struct again...so my reaction was to ensure that the NUL-terminator space was always guaranteed to be there. Hence, the change on c693 (objdump above). What do you think? Should we keep or leave the above "+ 1" after the rationale above? > > I would expect the correct allocation size to be: > offsetof(typeof(*ls), ls_name) + namelen Fair point, I will make this change. > > Question, though: is ls_name _expected_ to be %NUL terminated Yes, it is. I tracked down ls_name's utilisations and it is passed down to a bunch of routines that expects it to be NUL-terminated such as snprintf and vsnprintf. >, and was the prior 3 bytes of extra allocation accidentally required? > I am assuming that you are refering to ls_namelen in the struct dlm_ls (please correct me if this isn't what you meant). ls_namelen member is only used within the routine which kzalloc the space for the struct (fs/dlm/lockspace.c:new_lockspace). There are no external references to this member outside of that method in the kernel. One could say that ls_namelen can be removed without side effects but I wouldn't suggest doing it in this patch, that's why I didn't touch it :) Paulo A.
On Sun, Oct 09, 2022 at 03:05:17PM +1300, Paulo Miguel Almeida wrote: > On Sat, Oct 08, 2022 at 05:18:35PM -0700, Kees Cook wrote: > > This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. > > That's true. I agree that leaving "+ 1" would work and produce a > no-binary-changes patch due to the existing padding that the structure > has. OTOH, I thought that relying on that space could bite us in the > future if anyone tweaks the struct again...so my reaction was to ensure > that the NUL-terminator space was always guaranteed to be there. > Hence, the change on c693 (objdump above). > > What do you think? Should we keep or leave the above > "+ 1" after the rationale above? I think it depends on what's expected from this allocation. Christine or David, can you speak to this? > > I would expect the correct allocation size to be: > > offsetof(typeof(*ls), ls_name) + namelen > > Fair point, I will make this change. Well, only do that if we don't depend on the padding nor a trailing %NUL. :) > > Question, though: is ls_name _expected_ to be %NUL terminated > > Yes, it is. I tracked down ls_name's utilisations and it is passed down to > a bunch of routines that expects it to be NUL-terminated such as > snprintf and vsnprintf. Agreed: I see the string functions it gets passed to. So, then the next question I have is does "namelen" take into account the %NUL, and is "name" %NUL terminated? Those answers appear to be "no" and "yes", respectively: static int new_lockspace(const char *name, ...) { ... int namelen = strlen(name); The comparisons for ls->ls_namelen are all done without the %NUL count: if (ls->ls_namelen != namelen) continue; if (memcmp(ls->ls_name, name, namelen)) continue; > >, and was the prior 3 bytes of extra allocation accidentally required? > > > > I am assuming that you are refering to ls_namelen in the struct dlm_ls > (please correct me if this isn't what you meant). No, I meant ls_name (the pahole output shows the trailing 3 bytes of padding before. And with your patch it becomes 4 bytes of trailing padding. So I think this is "accidentally correct", since it's so carefully using memcmp() and not strcmp(). Given the existing padding on the structure, through, it likely needs to keep a certain amount of minimum padding. original size was actually this, so you could use this for the new calculation to get the same values as before: offsetof(typeof(*ls), ls_name) + 4 + namelen; In reality, it may be possible to do this to get exactly what is needed, but no less than the struct itself: max(offsetof(typeof(*ls), ls_name) + 1 + namelen, sizeof(*ls)); -Kees
On Sat, Oct 08, 2022 at 09:03:28PM -0700, Kees Cook wrote: > On Sun, Oct 09, 2022 at 03:05:17PM +1300, Paulo Miguel Almeida wrote: > > On Sat, Oct 08, 2022 at 05:18:35PM -0700, Kees Cook wrote: > > > This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. > > > > That's true. I agree that leaving "+ 1" would work and produce a > > no-binary-changes patch due to the existing padding that the structure > > has. OTOH, I thought that relying on that space could bite us in the > > future if anyone tweaks the struct again...so my reaction was to ensure > > that the NUL-terminator space was always guaranteed to be there. > > Hence, the change on c693 (objdump above). > > > > What do you think? Should we keep or leave the above > > "+ 1" after the rationale above? > > I think it depends on what's expected from this allocation. Christine or > David, can you speak to this? Hi, thanks for picking through that. Most likely the intention was to allow up to 64 (DLM_LOCKSPACE_LEN) character names, and then use the ls_name[1] for the terminating byte. I'd be happy to take the patch replacing the one-element name. Or, if you'd like to drop it, then we'll eliminate it along with a cleanup of name/namelen more broadly. Dave
On Mon, Oct 10, 2022 at 04:00:39PM -0500, David Teigland wrote: > On Sat, Oct 08, 2022 at 09:03:28PM -0700, Kees Cook wrote: > > On Sun, Oct 09, 2022 at 03:05:17PM +1300, Paulo Miguel Almeida wrote: > > > On Sat, Oct 08, 2022 at 05:18:35PM -0700, Kees Cook wrote: > > > > This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. > > > > > > That's true. I agree that leaving "+ 1" would work and produce a > > > no-binary-changes patch due to the existing padding that the structure > > > has. OTOH, I thought that relying on that space could bite us in the > > > future if anyone tweaks the struct again...so my reaction was to ensure > > > that the NUL-terminator space was always guaranteed to be there. > > > Hence, the change on c693 (objdump above). > > > > > > What do you think? Should we keep or leave the above > > > "+ 1" after the rationale above? > > > > I think it depends on what's expected from this allocation. Christine or > > David, can you speak to this? > > Hi, thanks for picking through that. Most likely the intention was to > allow up to 64 (DLM_LOCKSPACE_LEN) character names, and then use the > ls_name[1] for the terminating byte. I'd be happy to take the patch Should this just use: char ls_name[DLM_LOCKSPACE_LEN + 1]; instead, or is the byte savings worth keeping it dynamically sized?
On Mon, Oct 10, 2022 at 03:35:24PM -0700, Kees Cook wrote: > On Mon, Oct 10, 2022 at 04:00:39PM -0500, David Teigland wrote: > > On Sat, Oct 08, 2022 at 09:03:28PM -0700, Kees Cook wrote: > > > On Sun, Oct 09, 2022 at 03:05:17PM +1300, Paulo Miguel Almeida wrote: > > > > On Sat, Oct 08, 2022 at 05:18:35PM -0700, Kees Cook wrote: > > > > > This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. > > > > > > > > That's true. I agree that leaving "+ 1" would work and produce a > > > > no-binary-changes patch due to the existing padding that the structure > > > > has. OTOH, I thought that relying on that space could bite us in the > > > > future if anyone tweaks the struct again...so my reaction was to ensure > > > > that the NUL-terminator space was always guaranteed to be there. > > > > Hence, the change on c693 (objdump above). > > > > > > > > What do you think? Should we keep or leave the above > > > > "+ 1" after the rationale above? > > > > > > I think it depends on what's expected from this allocation. Christine or > > > David, can you speak to this? > > > > Hi, thanks for picking through that. Most likely the intention was to > > allow up to 64 (DLM_LOCKSPACE_LEN) character names, and then use the > > ls_name[1] for the terminating byte. I'd be happy to take the patch > > Should this just use: > > char ls_name[DLM_LOCKSPACE_LEN + 1]; > > instead, or is the byte savings worth keeping it dynamically sized? Yes, I think that's the best option. Dave
On Tue, Oct 11, 2022 at 10:20:31AM -0500, David Teigland wrote: > On Mon, Oct 10, 2022 at 03:35:24PM -0700, Kees Cook wrote: > > On Mon, Oct 10, 2022 at 04:00:39PM -0500, David Teigland wrote: > > > On Sat, Oct 08, 2022 at 09:03:28PM -0700, Kees Cook wrote: > > > > On Sun, Oct 09, 2022 at 03:05:17PM +1300, Paulo Miguel Almeida wrote: > > > > > On Sat, Oct 08, 2022 at 05:18:35PM -0700, Kees Cook wrote: > > > > > > This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off. > > > > > > > > > > That's true. I agree that leaving "+ 1" would work and produce a > > > > > no-binary-changes patch due to the existing padding that the structure > > > > > has. OTOH, I thought that relying on that space could bite us in the > > > > > future if anyone tweaks the struct again...so my reaction was to ensure > > > > > that the NUL-terminator space was always guaranteed to be there. > > > > > Hence, the change on c693 (objdump above). > > > > > > > > > > What do you think? Should we keep or leave the above > > > > > "+ 1" after the rationale above? > > > > > > > > I think it depends on what's expected from this allocation. Christine or > > > > David, can you speak to this? > > > > > > Hi, thanks for picking through that. Most likely the intention was to > > > allow up to 64 (DLM_LOCKSPACE_LEN) character names, and then use the > > > ls_name[1] for the terminating byte. I'd be happy to take the patch > > > > Should this just use: > > > > char ls_name[DLM_LOCKSPACE_LEN + 1]; > > > > instead, or is the byte savings worth keeping it dynamically sized? > > Yes, I think that's the best option. > Dave > Thanks for the reply Dave; Thanks for the suggestion Kees; I'll send a new patch for it :) Paulo A.
diff --git a/fs/dlm/dlm_internal.h b/fs/dlm/dlm_internal.h index e34c3d2639a5..ce2e32ed2ede 100644 --- a/fs/dlm/dlm_internal.h +++ b/fs/dlm/dlm_internal.h @@ -670,7 +670,7 @@ struct dlm_ls { void *ls_ops_arg; int ls_namelen; - char ls_name[1]; + char ls_name[]; }; /* diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c index bae050df7abf..c3a36f3197da 100644 --- a/fs/dlm/lockspace.c +++ b/fs/dlm/lockspace.c @@ -473,7 +473,7 @@ static int new_lockspace(const char *name, const char *cluster, error = -ENOMEM; - ls = kzalloc(sizeof(struct dlm_ls) + namelen, GFP_NOFS); + ls = kzalloc(sizeof(struct dlm_ls) + namelen + 1, GFP_NOFS); if (!ls) goto out; memcpy(ls->ls_name, name, namelen);