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.