[PATCH v2][next] dlm: Replace one-element array with flexible-array member

[PATCH v2][next] dlm: Replace one-element array with flexible-array member

[Paulo Miguel Almeida]

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 <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);
 	if (!ls)
 		goto out;
 	memcpy(ls->ls_name, name, namelen);
-- 
2.37.3

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[Paulo Miguel Almeida]

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.

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[Kees Cook]

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

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[David Teigland]

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

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[Kees Cook]

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

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[David Teigland]

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

Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

[Paulo Miguel Almeida]

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.

[PATCH v4] [next] dlm: replace one-element array with fixed size array

[Paulo Miguel Almeida]

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/Y0W5jkiXUkpNl4ap@mail.google.com/

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
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/Y0ICbf8tCtXMn+W0@mail.google.com/
---
 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

Re: [PATCH v4] [next] dlm: replace one-element array with fixed size array

[Kees Cook]

On Wed, Oct 12, 2022 at 09:04:15AM +1300, Paulo Miguel Almeida wrote:
> 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/Y0W5jkiXUkpNl4ap@mail.google.com/
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> 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/Y0ICbf8tCtXMn+W0@mail.google.com/
> ---
>  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);
                     ^^^^^^^^^^^^^^^^^^^^
I think you forgot the suggestion Gustavo had here. :) Preferred style would
be:
	sizeof(*ls)

-- 
Kees Cook

Re: [PATCH v4] [next] dlm: replace one-element array with fixed size array

[Paulo Miguel Almeida]

On Tue, Oct 11, 2022 at 01:06:32PM -0700, Kees Cook wrote:
> On Wed, Oct 12, 2022 at 09:04:15AM +1300, Paulo Miguel Almeida wrote:
> >  	error = -ENOMEM;
> >  
> > -	ls = kzalloc(sizeof(struct dlm_ls) + namelen, GFP_NOFS);
> > +	ls = kzalloc(sizeof(struct dlm_ls), GFP_NOFS);
>                      ^^^^^^^^^^^^^^^^^^^^
> I think you forgot the suggestion Gustavo had here. :) Preferred style would
> be:
> 	sizeof(*ls)
> 
Ooops, I hadn't seen that one =O 

Alright, v5 in the making

Paulo A.

[PATCH v5] [next] dlm: replace one-element array with fixed size array

[Paulo Miguel Almeida]

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/Y0W5jkiXUkpNl4ap@mail.google.com/

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:

v5: use preferred sizeof style. Req: Gustavo Silva
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/Y0ICbf8tCtXMn+W0@mail.google.com/
---

 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..9479c8110979 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(*ls), GFP_NOFS);
 	if (!ls)
 		goto out;
 	memcpy(ls->ls_name, name, namelen);
-- 
2.37.3

Re: [PATCH v5] [next] dlm: replace one-element array with fixed size array

[Gustavo A. R. Silva]

On Wed, Oct 12, 2022 at 09:23:14AM +1300, Paulo Miguel Almeida wrote:
> 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/Y0W5jkiXUkpNl4ap@mail.google.com/
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
> Changelog:
> 
> v5: use preferred sizeof style. Req: Gustavo Silva
> 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/Y0ICbf8tCtXMn+W0@mail.google.com/
> ---
> 
>  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..9479c8110979 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(*ls), GFP_NOFS);
>  	if (!ls)
>  		goto out;
>  	memcpy(ls->ls_name, name, namelen);
> -- 
> 2.37.3
> 

Re: [PATCH v5] [next] dlm: replace one-element array with fixed size array

[Kees Cook]

On Wed, Oct 12, 2022 at 09:23:14AM +1300, Paulo Miguel Almeida wrote:
> 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/Y0W5jkiXUkpNl4ap@mail.google.com/
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v5] [next] dlm: replace one-element array with fixed size array

[Paulo Miguel Almeida]

On Wed, Oct 12, 2022 at 09:23:14AM +1300, Paulo Miguel Almeida wrote:
> 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/Y0W5jkiXUkpNl4ap@mail.google.com/
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Changelog:
> 
> v5: use preferred sizeof style. Req: Gustavo Silva
> 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/Y0ICbf8tCtXMn+W0@mail.google.com/
> ---
> 
>  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..9479c8110979 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(*ls), GFP_NOFS);
>  	if (!ls)
>  		goto out;
>  	memcpy(ls->ls_name, name, namelen);
> -- 
> 2.37.3
> 

Christine, David,

Just following up on this patch. Is there anything that either of you
want me change for this patch to be merged?

thanks!

- Paulo A.

Re: [Cluster-devel] [PATCH v5] [next] dlm: replace one-element array with fixed size array

[Alexander Aring]

Hi,

On Fri, Nov 4, 2022 at 1:00 AM Paulo Miguel Almeida
<paulo.miguel.almeida.rodenas@gmail.com> wrote:
>
> On Wed, Oct 12, 2022 at 09:23:14AM +1300, Paulo Miguel Almeida wrote:
> > 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/Y0W5jkiXUkpNl4ap@mail.google.com/
> >
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> > ---
> > Changelog:
> >
> > v5: use preferred sizeof style. Req: Gustavo Silva
> > 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/Y0ICbf8tCtXMn+W0@mail.google.com/
> > ---
> >
> >  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..9479c8110979 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(*ls), GFP_NOFS);
> >       if (!ls)
> >               goto out;
> >       memcpy(ls->ls_name, name, namelen);
> > --
> > 2.37.3
> >
>
> Christine, David,
>
> Just following up on this patch. Is there anything that either of you
> want me change for this patch to be merged?
>

I think it's fine. I am working on DLM and the current upstream
process is more per release where I resend a huge patch-series to get
it upstream into dlm/next based on a recent rc... It just takes time.
Then your patch will of course be applied on.

- Alex