| Message ID | 20230731154718.31048-1-tiwai@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: Generic PCM copy ops using sockptr_t | expand |
On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote: > this is a patch set to clean up the PCM copy ops using sockptr_t as a > "universal" pointer, inspired by the recent patch from Andy > Shevchenko: > https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com > Even though it sounds a bit weird, sockptr_t is a generic type that is > used already in wide ranges, and it can fit our purpose, too. With > sockptr_t, the former split of copy_user and copy_kernel PCM ops can > be unified again gracefully. 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.
On Mon, 31 Jul 2023 19:20:54 +0200, Mark Brown wrote: > > On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote: > > > this is a patch set to clean up the PCM copy ops using sockptr_t as a > > "universal" pointer, inspired by the recent patch from Andy > > Shevchenko: > > https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com > > > Even though it sounds a bit weird, sockptr_t is a generic type that is > > used already in wide ranges, and it can fit our purpose, too. With > > sockptr_t, the former split of copy_user and copy_kernel PCM ops can > > be unified again gracefully. > > 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... Takashi
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.
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? Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] Introduce uniptr_t as replacement of sockptr_t Although sockptr_t is used already in several places as a "universal" pointer, it's still too confusing as if were related with network stuff. Make a more generic type, uniptr_t, that does exactly as same as sockptr_t for a wider use. sockptr_t becomes now alias to uniptr_t. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- include/linux/sockptr.h | 124 +++++----------------------------------- include/linux/uniptr.h | 117 +++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 109 deletions(-) create mode 100644 include/linux/uniptr.h diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h index bae5e2369b4f..dc803989a4d6 100644 --- a/include/linux/sockptr.h +++ b/include/linux/sockptr.h @@ -1,118 +1,24 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* - * Copyright (c) 2020 Christoph Hellwig. - * - * Support for "universal" pointers that can point to either kernel or userspace - * memory. + * Aliases for the old sockptr_t and its helpers for the new uniptr_t */ #ifndef _LINUX_SOCKPTR_H #define _LINUX_SOCKPTR_H -#include <linux/slab.h> -#include <linux/uaccess.h> +#include <linux/uniptr.h> -typedef struct { - union { - void *kernel; - void __user *user; - }; - bool is_kernel : 1; -} sockptr_t; - -static inline bool sockptr_is_kernel(sockptr_t sockptr) -{ - return sockptr.is_kernel; -} - -static inline sockptr_t KERNEL_SOCKPTR(void *p) -{ - return (sockptr_t) { .kernel = p, .is_kernel = true }; -} - -static inline sockptr_t USER_SOCKPTR(void __user *p) -{ - return (sockptr_t) { .user = p }; -} - -static inline bool sockptr_is_null(sockptr_t sockptr) -{ - if (sockptr_is_kernel(sockptr)) - return !sockptr.kernel; - return !sockptr.user; -} - -static inline int copy_from_sockptr_offset(void *dst, sockptr_t src, - size_t offset, size_t size) -{ - if (!sockptr_is_kernel(src)) - return copy_from_user(dst, src.user + offset, size); - memcpy(dst, src.kernel + offset, size); - return 0; -} - -static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size) -{ - return copy_from_sockptr_offset(dst, src, 0, size); -} - -static inline int copy_to_sockptr_offset(sockptr_t dst, size_t offset, - const void *src, size_t size) -{ - if (!sockptr_is_kernel(dst)) - return copy_to_user(dst.user + offset, src, size); - memcpy(dst.kernel + offset, src, size); - return 0; -} - -static inline int copy_to_sockptr(sockptr_t dst, const void *src, size_t size) -{ - return copy_to_sockptr_offset(dst, 0, src, size); -} - -static inline void *memdup_sockptr(sockptr_t src, size_t len) -{ - void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN); - - if (!p) - return ERR_PTR(-ENOMEM); - if (copy_from_sockptr(p, src, len)) { - kfree(p); - return ERR_PTR(-EFAULT); - } - return p; -} - -static inline void *memdup_sockptr_nul(sockptr_t src, size_t len) -{ - char *p = kmalloc_track_caller(len + 1, GFP_KERNEL); - - if (!p) - return ERR_PTR(-ENOMEM); - if (copy_from_sockptr(p, src, len)) { - kfree(p); - return ERR_PTR(-EFAULT); - } - p[len] = '\0'; - return p; -} - -static inline long strncpy_from_sockptr(char *dst, sockptr_t src, size_t count) -{ - if (sockptr_is_kernel(src)) { - size_t len = min(strnlen(src.kernel, count - 1) + 1, count); - - memcpy(dst, src.kernel, len); - return len; - } - return strncpy_from_user(dst, src.user, count); -} - -static inline int check_zeroed_sockptr(sockptr_t src, size_t offset, - size_t size) -{ - if (!sockptr_is_kernel(src)) - return check_zeroed_user(src.user + offset, size); - return memchr_inv(src.kernel + offset, 0, size) == NULL; -} +#define sockptr_t uniptr_t +#define sockptr_is_kernel uniptr_is_kernel +#define KERNEL_SOCKPTR KERNEL_UNIPTR +#define USER_SOCKPTR USER_UNIPTR +#define sockptr_is_null uniptr_is_null +#define copy_from_sockptr_offset copy_from_uniptr_offset +#define copy_from_sockptr copy_from_uniptr +#define copy_to_sockptr_offset copy_to_uniptr_offset +#define copy_to_sockptr copy_to_uniptr +#define memdup_sockptr memdup_uniptr +#define memdup_sockptr_nul memdup_uniptr_nul +#define strncpy_from_sockptr strncpy_from_uniptr +#define check_zeroed_sockptr check_zeroed_uniptr #endif /* _LINUX_SOCKPTR_H */ diff --git a/include/linux/uniptr.h b/include/linux/uniptr.h new file mode 100644 index 000000000000..3ca9fc8eab4e --- /dev/null +++ b/include/linux/uniptr.h @@ -0,0 +1,117 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2020 Christoph Hellwig. + * + * Support for "universal" pointers that can point to either kernel or userspace + * memory. + */ +#ifndef _LINUX_UNIPTR_H +#define _LINUX_UNIPTR_H + +#include <linux/slab.h> +#include <linux/uaccess.h> + +typedef struct { + union { + void *kernel; + void __user *user; + }; + bool is_kernel : 1; +} uniptr_t; + +static inline bool uniptr_is_kernel(uniptr_t uniptr) +{ + return uniptr.is_kernel; +} + +static inline uniptr_t KERNEL_UNIPTR(void *p) +{ + return (uniptr_t) { .kernel = p, .is_kernel = true }; +} + +static inline uniptr_t USER_UNIPTR(void __user *p) +{ + return (uniptr_t) { .user = p }; +} + +static inline bool uniptr_is_null(uniptr_t uniptr) +{ + if (uniptr_is_kernel(uniptr)) + return !uniptr.kernel; + return !uniptr.user; +} + +static inline int copy_from_uniptr_offset(void *dst, uniptr_t src, + size_t offset, size_t size) +{ + if (!uniptr_is_kernel(src)) + return copy_from_user(dst, src.user + offset, size); + memcpy(dst, src.kernel + offset, size); + return 0; +} + +static inline int copy_from_uniptr(void *dst, uniptr_t src, size_t size) +{ + return copy_from_uniptr_offset(dst, src, 0, size); +} + +static inline int copy_to_uniptr_offset(uniptr_t dst, size_t offset, + const void *src, size_t size) +{ + if (!uniptr_is_kernel(dst)) + return copy_to_user(dst.user + offset, src, size); + memcpy(dst.kernel + offset, src, size); + return 0; +} + +static inline int copy_to_uniptr(uniptr_t dst, const void *src, size_t size) +{ + return copy_to_uniptr_offset(dst, 0, src, size); +} + +static inline void *memdup_uniptr(uniptr_t src, size_t len) +{ + void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN); + + if (!p) + return ERR_PTR(-ENOMEM); + 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); + + 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); + + 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)) + return check_zeroed_user(src.user + offset, size); + return memchr_inv(src.kernel + offset, 0, size) == NULL; +} + +#endif /* _LINUX_UNIPTR_H */
On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote: > On Mon, 31 Jul 2023 19:20:54 +0200, > Mark Brown wrote: > > > > On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote: > > > > > this is a patch set to clean up the PCM copy ops using sockptr_t as a > > > "universal" pointer, inspired by the recent patch from Andy > > > Shevchenko: > > > https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com > > > > > Even though it sounds a bit weird, sockptr_t is a generic type that is > > > used already in wide ranges, and it can fit our purpose, too. With > > > sockptr_t, the former split of copy_user and copy_kernel PCM ops can > > > be unified again gracefully. > > > > 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... Maybe we should define a genptr_t (in genptr.h) and convert sockptr infra to use it (in sockptr.h)? This will leave network and other existing users to convert to it step-by-step. Another approach is to simply copy sockptr.h to genptr.h with changed naming scheme and add a deprecation note to the former. Thank you, Takashi, for doing this!
On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote: > That's true. The question is how more widely it'll be used, then. Indeed. > Is something like below what you had in mind, too? Yes, or Andy's suggestion of just copying without trying to put a redirect in works for me too. I imagine there might be some bikeshedding of the name, your proposal seems fine to me.
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.
On Tue, 01 Aug 2023 19:51:39 +0200, Andy Shevchenko wrote: > > 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. OK, makes sense to add them. > > More below. > > ... > > > + void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN); > > + > > + if (!p) > > + return ERR_PTR(-ENOMEM); > > This can use cleanup.h. Hm, I don't think it can be replaced with that. There may be more use of cleanup.h, but it's no direct alternative for kmalloc_track_caller()... > > + 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? A good question. I rather wonder why it can't be simple strncpy(). > > + 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)? A different person, a different taste :) But it's trivial to fix. > > + 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. Possibly that's a safer choice. But the biggest question is whether we want a generic type or not. Let's try to ask it first. Interestingly, this file doesn't belong to any subsystem in MAINTAINERS, so I'm not sure who to be Cc'ed. Chirstoph as the original author and net dev, maybe. thanks, Takashi
On Mon, Aug 07, 2023 at 05:22:18PM +0200, Takashi Iwai wrote: > On Tue, 01 Aug 2023 19:51:39 +0200, Andy Shevchenko wrote: > > On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote: ... > I rather wonder why it can't be simple strncpy(). This is obvious. To avoid compiler warning about 0 (possible) truncation. ... > > 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. > > Possibly that's a safer choice. But the biggest question is whether > we want a generic type or not. Let's try to ask it first. > > Interestingly, this file doesn't belong to any subsystem in > MAINTAINERS, so I'm not sure who to be Cc'ed. Chirstoph as the > original author and net dev, maybe. Yes, but actually it's fine to just copy and change sockptr.h to say "that's blablabla for net subsystem" (maybe this is implied by the name?). In that case we just introduce our copy and can do whatever modifications we want (see previous reply).