Re: [PATCH 1/5] string.h: add array-wrappers for (v)memdup_user()

[pstanner@redhat.com]

On Wed, 2023-08-30 at 17:29 +0300, Andy Shevchenko wrote:
> On Wed, Aug 30, 2023 at 5:19 PM <pstanner@redhat.com> wrote:
> > On Wed, 2023-08-30 at 17:11 +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 30, 2023 at 4:46 PM Philipp Stanner
> > > <pstanner@redhat.com>
> > > wrote:
> 
> > > > --- a/include/linux/string.h
> > > > +++ b/include/linux/string.h
> > > 
> > > I'm wondering if this has no side-effects as string.h/string.c
> > > IIRC
> > > is
> > > used also for early stages where some of the APIs are not
> > > available.
> > > 
> > > > @@ -6,6 +6,8 @@
> > > >  #include <linux/types.h>       /* for size_t */
> > > >  #include <linux/stddef.h>      /* for NULL */
> > > >  #include <linux/errno.h>       /* for E2BIG */
> > > > +#include <linux/overflow.h>    /* for check_mul_overflow() */
> > > > +#include <linux/err.h>         /* for ERR_PTR() */
> > > 
> > > Can we preserve order (to some extent)?
> > 
> > Sure. I just put it there so the comments build a congruent block.
> > Which order would you prefer?
> 
> Alphabetical.
> 
> compiler.h
> err.h
> overflow.h
> ...the rest that is a bit unordered...
> 
> > > >  #include <linux/stdarg.h>
> > > >  #include <uapi/linux/string.h>
> 
> ...

I mean I could include my own in a sorted manner – but the existing
ones are not sorted:

/* SPDX-License-Identifier: GPL-2.0 */
#ifndef _LINUX_STRING_H_
#define _LINUX_STRING_H_

#include <linux/compiler.h>	/* for inline */
#include <linux/types.h>	/* for size_t */
#include <linux/stddef.h>	/* for NULL */
#include <linux/errno.h>	/* for E2BIG */
#include <linux/stdarg.h>
#include <uapi/linux/string.h>

extern char *strndup_user(const char __user *, long);

We could sort them all, but I'd prefer to do that in a separate patch
so that this commit does not make the impression of doing anything else
than including the two new headers

Such a separate patch could also unify the docstring style, see below

> 
> > > > +/**
> > > > + * memdup_array_user - duplicate array from user space
> > > 
> > > > + *
> > > 
> > > Do we need this blank line?
> > 
> > I more or less directly copied the docstring format from the
> > original
> > functions (v)memdup_user() in mm/util.c
> > I guess this is common style?
> 
> I think it's not. But you may grep kernel source tree and tell which
> one occurs more often with or without this (unneeded) blank line.


It seems to be very much mixed. string.h itself is mixed.
When you look at the bottom of string.h, you'll find functions such as
kbasename() that have the extra line.

That's not really a super decisive point for me, though. We can remove
the line I guess


P.


> 
> > > > + * @src: source address in user space
> > > > + * @n: number of array members to copy
> > > > + * @size: size of one array member
> > > > + *
> > > > + * Return: an ERR_PTR() on failure.  Result is physically
> > > > + * contiguous, to be freed by kfree().
> > > > + */
> 
> ...
> 
> > > > +/**
> > > > + * vmemdup_array_user - duplicate array from user space
> > > 
> > > > + *
> > > 
> > > Redundant?
> > 
> > No, there are two functions:
> >  * memdup_array_user()
> >  * vmemdup_array_user()
> > 
> > On the deeper layers they utilize kmalloc() or kvmalloc(),
> > respectively.
> 
> I guess you misunderstood my comment. I was talking about kernel doc
> (as in the previous function).
> 
> > > > + * @src: source address in user space
> > > > + * @n: number of array members to copy
> > > > + * @size: size of one array member
> > > > + *
> > > > + * Return: an ERR_PTR() on failure.  Result may be not
> > > > + * physically contiguous.  Use kvfree() to free.
> > > > + */
> 
>