Re: [PATCH v2 2/2] string: use offset_in_page() in sized_strscpy()

[Lorenzo Stoakes <ljs@kernel.org>]

NAK.

I guess I wasn't clear before :)

On Thu, May 21, 2026 at 03:47:25PM +0200, Thorsten Blum wrote:
> On Thu, May 21, 2026 at 12:31:25PM +0100, Lorenzo Stoakes wrote:
> > On Thu, May 21, 2026 at 11:06:58AM +0200, Thorsten Blum wrote:
> > > Replace the open-coded implementation with offset_in_page() to simplify
> > > sized_strscpy().
> > >
> > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> >
> > I feel this patch actually makes sized_strscpy() more difficult to understand
> > unfortunately, so not really in favour of us taking it.
> >
> > > ---
> > >  lib/string.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/string.c b/lib/string.c
> > > index 1f9297e9776a..7c72adc7377c 100644
> > > --- a/lib/string.c
> > > +++ b/lib/string.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/stddef.h>
> > >  #include <linux/string.h>
> > >  #include <linux/types.h>
> > > +#include <vdso/page.h>
> > >
> > >  #include <asm/page.h>
> > >  #include <asm/rwonce.h>
> > > @@ -127,7 +128,7 @@ ssize_t sized_strscpy(char *dest, const char *src, size_t count)
> > >  	 * since we don't know if the next page is mapped.
> > >  	 */
> > >  	if ((long)src & (sizeof(long) - 1))
> > > -		max = min(PAGE_SIZE - ((long)src & (PAGE_SIZE - 1)), max);
> > > +		max = min(PAGE_SIZE - offset_in_page(src), max);
> >
> > This isn't strictly the same code, offset_in_page() is:
> >
> > #define offset_in_page(p)	((unsigned long)(p) & ~PAGE_MASK)
> >
> > So this is now, effectively:
> >
> > -		max = min(PAGE_SIZE - ((long)src & (PAGE_SIZE - 1)), max);
> > +		max = min(PAGE_SIZE - ((unsigned long)src & (PAGE_SIZE -1)), max);
> >
> > So there could be some issues here with type conversions at least in theory.
>
> I think this should have used unsigned long from the start, as long
> seems a bit odd for a pointer, and PAGE_SIZE - offset_in_page() is a
> common kernel idiom for this exact calculation.
>
> Andrew also commented [1] on this line recently.

Yup, but it makes no sense as a singular change, it actively worsens the
code.

Hypotheticals about what should or should not be with you not changing
anything isn't useful.

>
> > But in any case you're now making the logic inconsistent (the next line uses
> > bitwise operations directly).
>
> Agreed, maybe there are other helpers we can use here too, but that
> should probably be a follow-up patch.

No, this patch is bad, we're not taking it.

>
> > So I'd rather we didn't make this change.
>
> FWIW, using offset_in_page() in lib/string.c (as suggested by Andy [2])
> was the initial motivation for moving it out of mm.h, so I'd prefer to
> keep the use-site in this series if possible.

Nope.

>
> Thanks,
> Thorsten
>
> [1] https://lore.kernel.org/lkml/20260519123740.458d905f958f39d41cb130fd@linux-foundation.org/
> [2] https://lore.kernel.org/lkml/CAHp75VfQNkqEYsO4Uup0c-uiYuVyAWit=tmCz2BsYLp-sjXsZw@mail.gmail.com/

Thanks, Lorenzo