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.
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)
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]
$ 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 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 <[email protected]>
---
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);
if (!ls)
goto out;
memcpy(ls->ls_name, name, namelen);
--
2.37.3
On October 8, 2022 4:17:37 PM PDT, Paulo Miguel Almeida <[email protected]> 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 <[email protected]>
>---
>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
--
Kees Cook
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
--
Kees Cook
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?
--
Kees Cook
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.
One-element arrays are deprecated. So, replace one-element array with
fixed size array member in struct dlm_ls, and refactor the rest of the
code, accordingly.
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
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Paulo Miguel Almeida <[email protected]>
---
Changelog:
v4: resend patch using the right version number. Req: Gustavo Silva
v3: replace one-element array with a fixed size array. Req: Kees Cook
v2: patch resent as I had an issue with a <CRLF> char in my mail client
v1: https://lore.kernel.org/lkml/[email protected]/
---
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..94fadb619ba0 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[DLM_LOCKSPACE_LEN + 1];
};
/*
diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
index bae050df7abf..23de0d47cbc1 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), GFP_NOFS);
if (!ls)
goto out;
memcpy(ls->ls_name, name, namelen);
--
2.37.3