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

[Thorsten Blum]

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@xxxxxxxxx>
> 
> 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.

> 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.

> 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.

Thanks,
Thorsten

[1] https://lore.kernel.org/lkml/20260519123740.458d905f958f39d41cb130fd@xxxxxxxxxxxxxxxxxxxx/
[2] https://lore.kernel.org/lkml/CAHp75VfQNkqEYsO4Uup0c-uiYuVyAWit=tmCz2BsYLp-sjXsZw@xxxxxxxxxxxxxx/