[PATCH] m68k: mm: Remove size argument when calling strscpy()

[PATCH] m68k: mm: Remove size argument when calling strscpy()

[Thorsten Blum]

The size parameter of strscpy() is optional and specifying the size of
the destination buffer is unnecessary. Remove it to simplify the code.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 arch/m68k/kernel/setup_mm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index 48ce67947678..0fba32552836 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -147,8 +147,7 @@ static void __init m68k_parse_bootinfo(const struct bi_record *record)
 			break;
 
 		case BI_COMMAND_LINE:
-			strscpy(m68k_command_line, data,
-				sizeof(m68k_command_line));
+			strscpy(m68k_command_line, data);
 			break;
 
 		case BI_RNG_SEED: {
-- 
2.48.1

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Jean-Michel Hautbois]

Hi Thorsten,

Thanks for the patch !

On 03/03/2025 00:05, Thorsten Blum wrote:
> The size parameter of strscpy() is optional and specifying the size of
> the destination buffer is unnecessary. Remove it to simplify the code.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>

> ---
>   arch/m68k/kernel/setup_mm.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
> index 48ce67947678..0fba32552836 100644
> --- a/arch/m68k/kernel/setup_mm.c
> +++ b/arch/m68k/kernel/setup_mm.c
> @@ -147,8 +147,7 @@ static void __init m68k_parse_bootinfo(const struct bi_record *record)
>   			break;
>   
>   		case BI_COMMAND_LINE:
> -			strscpy(m68k_command_line, data,
> -				sizeof(m68k_command_line));
> +			strscpy(m68k_command_line, data);
>   			break;
>   
>   		case BI_RNG_SEED: {

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Geert Uytterhoeven]

On Mon, 3 Mar 2025 at 00:07, Thorsten Blum <thorsten.blum@linux.dev> wrote:
> The size parameter of strscpy() is optional and specifying the size of
> the destination buffer is unnecessary. Remove it to simplify the code.
>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
i.e. will queue in the m68k tree for v6.15.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Finn Thain]

On Thu, 6 Mar 2025, Geert Uytterhoeven wrote:

> On Mon, 3 Mar 2025 at 00:07, Thorsten Blum <thorsten.blum@linux.dev> wrote:
> > The size parameter of strscpy() is optional and specifying the size of
> > the destination buffer is unnecessary. Remove it to simplify the code.
> >
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> i.e. will queue in the m68k tree for v6.15.
> 

The commit message says "simplify the code" which is only true if you 
never scratch the surface (i.e. it's simple code if the reader is simple 
too...)

Commit 30035e45753b ("string: provide strscpy()") was a good idea. It was 
easily auditable. But that's not what we have now.

Patches like this one (which appear across the whole tree) need reviewers 
(lots of them) that know what kind of a bounds check you end up with when 
you ask an arbitary compiler to evaluate this:

sizeof(dst) + __must_be_array(dst) + __must_be_cstr(dst) + __must_be_cstr(src)

Frankly, I can't be sure. But it's a serious question, and not what I'd 
call a "simple" one.

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Geert Uytterhoeven]

Hi Finn,

CC Kees

On Fri, 7 Mar 2025 at 00:24, Finn Thain <fthain@linux-m68k.org> wrote:
> On Thu, 6 Mar 2025, Geert Uytterhoeven wrote:
> > On Mon, 3 Mar 2025 at 00:07, Thorsten Blum <thorsten.blum@linux.dev> wrote:
> > > The size parameter of strscpy() is optional and specifying the size of
> > > the destination buffer is unnecessary. Remove it to simplify the code.
> > >
> > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> >
> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > i.e. will queue in the m68k tree for v6.15.
>
> The commit message says "simplify the code" which is only true if you
> never scratch the surface (i.e. it's simple code if the reader is simple
> too...)

The code is simpler in the sense that the API is simpler to use,
and harder to abuse (i.e. to get it wrong).

> Commit 30035e45753b ("string: provide strscpy()") was a good idea. It was
> easily auditable. But that's not what we have now.
>
> Patches like this one (which appear across the whole tree) need reviewers
> (lots of them) that know what kind of a bounds check you end up with when
> you ask an arbitary compiler to evaluate this:
>
> sizeof(dst) + __must_be_array(dst) + __must_be_cstr(dst) + __must_be_cstr(src)
>
> Frankly, I can't be sure. But it's a serious question, and not what I'd
> call a "simple" one.

All the __must_be_*() macros evaluate to zero when true, and cause a
build failure when false.

BTW, Linux does not support being built by an "arbitrary compiler":
only gcc and clang are supported.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Finn Thain]

On Fri, 7 Mar 2025, Geert Uytterhoeven wrote:

> On Fri, 7 Mar 2025 at 00:24, Finn Thain <fthain@linux-m68k.org> wrote:
> > On Thu, 6 Mar 2025, Geert Uytterhoeven wrote:
> > > On Mon, 3 Mar 2025 at 00:07, Thorsten Blum <thorsten.blum@linux.dev> wrote:
> > > > The size parameter of strscpy() is optional and specifying the 
> > > > size of the destination buffer is unnecessary. Remove it to 
> > > > simplify the code.
> > > >
> > > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> i.e. will 
> > > queue in the m68k tree for v6.15.
> >
> > The commit message says "simplify the code" which is only true if you 
> > never scratch the surface (i.e. it's simple code if the reader is 
> > simple too...)
> 
> The code is simpler in the sense that the API is simpler to use, and 
> harder to abuse (i.e. to get it wrong).
> 
> > Commit 30035e45753b ("string: provide strscpy()") was a good idea. It 
> > was easily auditable. But that's not what we have now.
> >
> > Patches like this one (which appear across the whole tree) need 
> > reviewers (lots of them) that know what kind of a bounds check you end 
> > up with when you ask an arbitary compiler to evaluate this:
> >
> > sizeof(dst) + __must_be_array(dst) + __must_be_cstr(dst) + 
> > __must_be_cstr(src)
> >
> > Frankly, I can't be sure. But it's a serious question, and not what 
> > I'd call a "simple" one.
> 
> All the __must_be_*() macros evaluate to zero when true, and cause a 
> build failure when false.
> 

It seems to me that the code review problem could be solved either by not 
churning the whole tree, or if we must have the churn, by short-circuiting 
the recursive search by reviewers for macro definitions.

Can we do something like this?

sizeof(dst) * !!__must_be_array(dst) * !!__must_be_cstr(dst) * !!__must_be_cstr(src)

At first glance multiplication appears to be safe (unlike all the addition 
terms that we have) because the limit of the string copy is either 
unchanged or zeroed.

Yes, I know you said "zero when true". That looks like another design flaw 
to me. But maybe I'm missing something that's more important than 
readability and ease of review.

> BTW, Linux does not support being built by an "arbitrary compiler": only 
> gcc and clang are supported.
> 

So only gcc and clang must agree about all of the details...

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Thorsten Blum]

Hi Finn,

On 7. Mar 2025, at 00:24, Finn Thain wrote:
> On Thu, 6 Mar 2025, Geert Uytterhoeven wrote:
>> On Mon, 3 Mar 2025 at 00:07, Thorsten Blum wrote:
>>> The size parameter of strscpy() is optional and specifying the size of
>>> the destination buffer is unnecessary. Remove it to simplify the code.
>>> 
>>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>> 
>> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> i.e. will queue in the m68k tree for v6.15.
>> 
> The commit message says "simplify the code" which is only true if you 
> never scratch the surface (i.e. it's simple code if the reader is simple 
> too...)

strscpy() automatically determines the length of the destination buffer
using sizeof() if the size argument is omitted. This makes the explicit
sizeof(m68k_command_line) unnecessary, so removing it shortens the code
without changing its behavior.

Both macro calls expand to the same code, and I find the shorter version
simpler to read (this doesn't mean that strscpy() itself is simple).

> Commit 30035e45753b ("string: provide strscpy()") was a good idea. It was 
> easily auditable. But that's not what we have now.
> 
> Patches like this one (which appear across the whole tree) need reviewers 
> (lots of them) that know what kind of a bounds check you end up with when 
> you ask an arbitary compiler to evaluate this:
> 
> sizeof(dst) + __must_be_array(dst) + __must_be_cstr(dst) + __must_be_cstr(src)
> 
> Frankly, I can't be sure. But it's a serious question, and not what I'd 
> call a "simple" one.

I'm not sure I fully understand this part or how it's related to this
change. This patch doesn't change bounds checking, it just removes an
unnecessary macro argument.

Thanks,
Thorsten

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Geert Uytterhoeven]

Hi Finn,

On Fri, 7 Mar 2025 at 09:58, Finn Thain <fthain@linux-m68k.org> wrote:
> On Fri, 7 Mar 2025, Geert Uytterhoeven wrote:
> > On Fri, 7 Mar 2025 at 00:24, Finn Thain <fthain@linux-m68k.org> wrote:
> > > On Thu, 6 Mar 2025, Geert Uytterhoeven wrote:
> > > > On Mon, 3 Mar 2025 at 00:07, Thorsten Blum <thorsten.blum@linux.dev> wrote:
> > > > > The size parameter of strscpy() is optional and specifying the
> > > > > size of the destination buffer is unnecessary. Remove it to
> > > > > simplify the code.
> > > > >
> > > > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > > >
> > > > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> i.e. will
> > > > queue in the m68k tree for v6.15.
> > >
> > > The commit message says "simplify the code" which is only true if you
> > > never scratch the surface (i.e. it's simple code if the reader is
> > > simple too...)
> >
> > The code is simpler in the sense that the API is simpler to use, and
> > harder to abuse (i.e. to get it wrong).
> >
> > > Commit 30035e45753b ("string: provide strscpy()") was a good idea. It
> > > was easily auditable. But that's not what we have now.
> > >
> > > Patches like this one (which appear across the whole tree) need
> > > reviewers (lots of them) that know what kind of a bounds check you end
> > > up with when you ask an arbitary compiler to evaluate this:
> > >
> > > sizeof(dst) + __must_be_array(dst) + __must_be_cstr(dst) +
> > > __must_be_cstr(src)
> > >
> > > Frankly, I can't be sure. But it's a serious question, and not what
> > > I'd call a "simple" one.
> >
> > All the __must_be_*() macros evaluate to zero when true, and cause a
> > build failure when false.
>
> It seems to me that the code review problem could be solved either by not
> churning the whole tree, or if we must have the churn, by short-circuiting
> the recursive search by reviewers for macro definitions.
>
> Can we do something like this?
>
> sizeof(dst) * !!__must_be_array(dst) * !!__must_be_cstr(dst) * !!__must_be_cstr(src)

x * !!0 = 0

So either the above should be changed to

    sizeof(dst) * !__must_be_array(dst) * !__must_be_cstr(dst) *
!__must_be_cstr(src)

or all __must_be_*() macros should be changed to invert their return values...

> At first glance multiplication appears to be safe (unlike all the addition
> terms that we have) because the limit of the string copy is either
> unchanged or zeroed.
>
> Yes, I know you said "zero when true". That looks like another design flaw
> to me. But maybe I'm missing something that's more important than
> readability and ease of review.

We had

    #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))

for ages...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68k: mm: Remove size argument when calling strscpy()

[Finn Thain]

On Fri, 7 Mar 2025, Thorsten Blum wrote:

> it shortens the code without changing its behavior
> 

Is there some limit on the quantity of code in the kernel that could be 
made shorter by the addition of new layers of macros? The only limit I can 
see is the scarcity of skilled code reviewers.

As for code style, I guess I prefer shallow bugs but YMMV.