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
