Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 21:40:20 +0200,
> Mark Brown wrote:
> > On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > 
> > > > It really feels like we ought to rename, or add an alias for, the type
> > > > if we're going to start using it more widely - it's not helping to make
> > > > the code clearer.
> > 
> > > That was my very first impression, too, but I changed my mind after
> > > seeing the already used code.  An alias might work, either typedef or
> > > define genptr_t or such as sockptr_t.  But we'll need to copy the
> > > bunch of helper functions, too...
> > 
> > I would predict that if the type becomes more widely used that'll happen
> > eventually and the longer it's left the more work it'll be.
> 
> That's true.  The question is how more widely it'll be used, then.
> 
> Is something like below what you had in mind, too?

I agree with your proposal (uniptr_t also works for me), but see below.

...

> +#include <linux/slab.h>
> +#include <linux/uaccess.h>

But let make the list of the headers right this time.

It seems to me that

err.h
minmax.h // maybe not, see a remark at the bottom
string.h
types.h

are missing.

More below.

...

> +     void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
> +
> +     if (!p)
> +             return ERR_PTR(-ENOMEM);

This can use cleanup.h.

> +     if (copy_from_uniptr(p, src, len)) {
> +             kfree(p);
> +             return ERR_PTR(-EFAULT);
> +     }
> +     return p;
> +}
> +
> +static inline void *memdup_uniptr_nul(uniptr_t src, size_t len)
> +{
> +     char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);

Ditto.

> +     if (!p)
> +             return ERR_PTR(-ENOMEM);
> +     if (copy_from_uniptr(p, src, len)) {
> +             kfree(p);
> +             return ERR_PTR(-EFAULT);
> +     }
> +     p[len] = '\0';
> +     return p;
> +}

...

> +static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count)
> +{
> +     if (uniptr_is_kernel(src)) {
> +             size_t len = min(strnlen(src.kernel, count - 1) + 1, count);

I didn't get why do we need min()? To check the count == 0 case?

Wouldn't

                size_t len;

                len = strnlen(src.kernel, count);
                if (len == 0)
                        return 0;

                /* Copy a trailing NUL if found */
                if (len < count)
                        len++;

be a good equivalent?

> +             memcpy(dst, src.kernel, len);
> +             return len;
> +     }
> +     return strncpy_from_user(dst, src.user, count);
> +}

...

> +static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t 
> size)
> +{
> +     if (!uniptr_is_kernel(src))

Why not to align all the functions to use same conditional (either always
positive or negative)?

> +             return check_zeroed_user(src.user + offset, size);
> +     return memchr_inv(src.kernel + offset, 0, size) == NULL;
> +}

...

Taking all remarks into account I would rather go with sockptr.h being
untouched for now, just a big

/* DO NOT USE, it's obsolete, use uniptr.h instead! */

to be added.

-- 
With Best Regards,
Andy Shevchenko