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

[Andy Shevchenko]

On Wed, Aug 30, 2023 at 5:19 PM <pstanner@xxxxxxxxxx> 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@xxxxxxxxxx>
> > 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>

...

> > > +/**
> > > + * 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.

> > > + * @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.
> > > + */


-- 
With Best Regards,
Andy Shevchenko