[RFC] Introduce uniptr_t as a generic "universal" pointer

Commit Message

Takashi Iwai Aug. 9, 2023, 2:35 p.m. UTC

Although sockptr_t is used already in several places as a "universal"
pointer, it's still too confusing to use it in other subsystems, since
people see it always as if it were a network-related stuff.

This patch defines a more generic type, uniptr_t, that does exactly as
same as sockptr_t for a wider use.  As of now, it's almost 1:1 copy
with renames (just with comprehensive header file inclusions).

Signed-off-by: Takashi Iwai <[e-mail]>
---

This is a RFC patch, or rather a material for bikeshedding.

Initially the discussion started from the use of sockptr_t for the
sound driver in Andy's patch:
  https://lore.kernel.org/r/[e-mail]
followed by a bigger series of patches by me:
  https://lore.kernel.org/r/[e-mail]

The first reaction to the patches (including my own) were
"why sockptr_t?"  Yes, it's just confusing.  So, here it is, a
proposal of defining the new type for the very purpose as sockptr_t.

The name of uniptr_t is nothing but my random pick up, and we can
endlessly discuss for a better name (genptr_t or whatever).
I'm totally open for the name.

After this introduction, sockptr_t can be alias of uniptr_t,
e.g. simply override with "#define sockptr_t uniptr_t" or such.
How can it be is another open question.

Also, we can clean up the macro implementation along with it;
there seem a few (rather minor) issues as suggested by Andy:
  https://lore.kernel.org/r/[e-mail]

Honestly speaking, I don't mind to keep using sockptr_t generically
despite of the name, if people agree.  The rename might make sense,
though, if it's more widely used in other subsystems in future.


Takashi

===

 include/linux/uniptr.h | 121 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 include/linux/uniptr.h

Comments

Christoph Hellwig Aug. 9, 2023, 2:38 p.m. UTC | #1

On Wed, Aug 09, 2023 at 04:35:47PM +0200, Takashi Iwai wrote:
> Although sockptr_t is used already in several places as a "universal"
> pointer, it's still too confusing to use it in other subsystems, since
> people see it always as if it were a network-related stuff.
> 
> This patch defines a more generic type, uniptr_t, that does exactly as
> same as sockptr_t for a wider use.  As of now, it's almost 1:1 copy
> with renames (just with comprehensive header file inclusions).

The original set_fs removal series did that as uptr_t, and Linus
hated it with passion.  I somehow doubt he's going to like it more now.

> Signed-off-by: Takashi Iwai <[e-mail]>
> ---
> 
> This is a RFC patch, or rather a material for bikeshedding.
> 
> Initially the discussion started from the use of sockptr_t for the
> sound driver in Andy's patch:
>   https://lore.kernel.org/r/[e-mail]
> followed by a bigger series of patches by me:
>   https://lore.kernel.org/r/[e-mail]
> 
> The first reaction to the patches (including my own) were
> "why sockptr_t?"  Yes, it's just confusing.  So, here it is, a
> proposal of defining the new type for the very purpose as sockptr_t.
> 
> The name of uniptr_t is nothing but my random pick up, and we can
> endlessly discuss for a better name (genptr_t or whatever).
> I'm totally open for the name.
> 
> After this introduction, sockptr_t can be alias of uniptr_t,
> e.g. simply override with "#define sockptr_t uniptr_t" or such.
> How can it be is another open question.
> 
> Also, we can clean up the macro implementation along with it;
> there seem a few (rather minor) issues as suggested by Andy:
>   https://lore.kernel.org/r/[e-mail]
> 
> Honestly speaking, I don't mind to keep using sockptr_t generically
> despite of the name, if people agree.  The rename might make sense,
> though, if it's more widely used in other subsystems in future.
> 
> 
> Takashi
> 
> ===
> 
>  include/linux/uniptr.h | 121 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 include/linux/uniptr.h
> 
> diff --git a/include/linux/uniptr.h b/include/linux/uniptr.h
> new file mode 100644
> index 000000000000..f7994d3a45eb
> --- /dev/null
> +++ b/include/linux/uniptr.h
> @@ -0,0 +1,121 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Support for "universal" pointers that can point to either kernel or userspace
> + * memory.
> + *
> + * Original code from sockptr.h
> + *    Copyright (c) 2020 Christoph Hellwig
> + */
> +#ifndef _LINUX_UNIPTR_H
> +#define _LINUX_UNIPTR_H
> +
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.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 */
> -- 
> 2.35.3
---end quoted text---

Takashi Iwai Aug. 9, 2023, 2:44 p.m. UTC | #2

On Wed, 09 Aug 2023 16:38:01 +0200,
Christoph Hellwig wrote:
> 
> On Wed, Aug 09, 2023 at 04:35:47PM +0200, Takashi Iwai wrote:
> > Although sockptr_t is used already in several places as a "universal"
> > pointer, it's still too confusing to use it in other subsystems, since
> > people see it always as if it were a network-related stuff.
> > 
> > This patch defines a more generic type, uniptr_t, that does exactly as
> > same as sockptr_t for a wider use.  As of now, it's almost 1:1 copy
> > with renames (just with comprehensive header file inclusions).
> 
> The original set_fs removal series did that as uptr_t, and Linus
> hated it with passion.  I somehow doubt he's going to like it more now.

Ah, good to know!

The remaining question is whether the use of sockptr_t for other
subsystems as a generic pointer is a recommended / acceptable move...


Takashi

> > Signed-off-by: Takashi Iwai <[e-mail]>
> > ---
> > 
> > This is a RFC patch, or rather a material for bikeshedding.
> > 
> > Initially the discussion started from the use of sockptr_t for the
> > sound driver in Andy's patch:
> >   https://lore.kernel.org/r/[e-mail]
> > followed by a bigger series of patches by me:
> >   https://lore.kernel.org/r/[e-mail]
> > 
> > The first reaction to the patches (including my own) were
> > "why sockptr_t?"  Yes, it's just confusing.  So, here it is, a
> > proposal of defining the new type for the very purpose as sockptr_t.
> > 
> > The name of uniptr_t is nothing but my random pick up, and we can
> > endlessly discuss for a better name (genptr_t or whatever).
> > I'm totally open for the name.
> > 
> > After this introduction, sockptr_t can be alias of uniptr_t,
> > e.g. simply override with "#define sockptr_t uniptr_t" or such.
> > How can it be is another open question.
> > 
> > Also, we can clean up the macro implementation along with it;
> > there seem a few (rather minor) issues as suggested by Andy:
> >   https://lore.kernel.org/r/[e-mail]
> > 
> > Honestly speaking, I don't mind to keep using sockptr_t generically
> > despite of the name, if people agree.  The rename might make sense,
> > though, if it's more widely used in other subsystems in future.
> > 
> > 
> > Takashi
> > 
> > ===
> > 
> >  include/linux/uniptr.h | 121 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >  create mode 100644 include/linux/uniptr.h
> > 
> > diff --git a/include/linux/uniptr.h b/include/linux/uniptr.h
> > new file mode 100644
> > index 000000000000..f7994d3a45eb
> > --- /dev/null
> > +++ b/include/linux/uniptr.h
> > @@ -0,0 +1,121 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Support for "universal" pointers that can point to either kernel or userspace
> > + * memory.
> > + *
> > + * Original code from sockptr.h
> > + *    Copyright (c) 2020 Christoph Hellwig
> > + */
> > +#ifndef _LINUX_UNIPTR_H
> > +#define _LINUX_UNIPTR_H
> > +
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.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 */
> > -- 
> > 2.35.3
> ---end quoted text---
>

Linus Torvalds Aug. 9, 2023, 3:48 p.m. UTC | #3

On Wed, 9 Aug 2023 at 07:38, Christoph Hellwig <[e-mail]> wrote:
>
> The original set_fs removal series did that as uptr_t, and Linus
> hated it with passion.  I somehow doubt he's going to like it more now.

Christoph is right. I do hate this. The whole "pass a pointer that is
either user or kernel" concept is wrong.

Now, if it was some kind of extended pointer that also included the
length of the area and had a way to deal with updating the pointer
sanely, maybe that would be a different thing.

And it should guarantee that in the case of a user pointer it had gone
through access_ok().

And it also allowed the other common cases like having a raw page
array, along with a unified interface to copy and update this kind of
pointer either as a source or a destination, that would be a different
thing.

But this kind of "if (uniptr_is_kernel(src))" special case thing is
just garbage and *not* acceptable.

And oh, btw, we already *have* that extended kind of unipointer thing.

It's called "struct iov_iter".

And yes, it's a very complicated thing, exactly because it handles way
more cases than that uniptr_t. It's a *real* unified pointer of many
different types.

Those iov_iter things are often so complicated that you really don't
want to use them, but if you really want a uniptr, that is what you
should do. It comes with a real cost, but it does come with real
advantages, one of which is "this is extensively tested
nfrastructure".

                  Linus

Linus Torvalds Aug. 9, 2023, 3:59 p.m. UTC | #4

On Wed, 9 Aug 2023 at 07:44, Takashi Iwai <[e-mail]> wrote:
>
> The remaining question is whether the use of sockptr_t for other
> subsystems as a generic pointer is a recommended / acceptable move...

Very much not recommended. sockptr_t is horrible too, but it was (part
of) what made it possible to fix an even worse horrible historical
mistake (ie getting rid of set_fs()).

So I detest sockptr_t. It's garbage. At the very minimum it should
have had the length associated with it, not passed separately.

But it's garbage exactly because it allowed for conversion of some
much much horrid legacy code with fairly minimal impact.

New code does *not* have that excuse.

DO NOT MIX USER AND KERNEL POINTERS. And don't add interfaces that
think such mixing is ok. Pointers should be statically clearly of one
type or the other, and never lied about.

Or you go all the way, and do that whole iterator thing, and make it
very clear that you're doing something truly generic that can be
passed fairly widely along across subsystem boundaries.

             Linus

Takashi Iwai Aug. 9, 2023, 4:05 p.m. UTC | #5

On Wed, 09 Aug 2023 17:48:11 +0200,
Linus Torvalds wrote:
> 
> On Wed, 9 Aug 2023 at 07:38, Christoph Hellwig <[e-mail]> wrote:
> >
> > The original set_fs removal series did that as uptr_t, and Linus
> > hated it with passion.  I somehow doubt he's going to like it more now.
> 
> Christoph is right. I do hate this. The whole "pass a pointer that is
> either user or kernel" concept is wrong.
> 
> Now, if it was some kind of extended pointer that also included the
> length of the area and had a way to deal with updating the pointer
> sanely, maybe that would be a different thing.
> 
> And it should guarantee that in the case of a user pointer it had gone
> through access_ok().
> 
> And it also allowed the other common cases like having a raw page
> array, along with a unified interface to copy and update this kind of
> pointer either as a source or a destination, that would be a different
> thing.
> 
> But this kind of "if (uniptr_is_kernel(src))" special case thing is
> just garbage and *not* acceptable.
> 
> And oh, btw, we already *have* that extended kind of unipointer thing.
> 
> It's called "struct iov_iter".
> 
> And yes, it's a very complicated thing, exactly because it handles way
> more cases than that uniptr_t. It's a *real* unified pointer of many
> different types.
> 
> Those iov_iter things are often so complicated that you really don't
> want to use them, but if you really want a uniptr, that is what you
> should do. It comes with a real cost, but it does come with real
> advantages, one of which is "this is extensively tested
> nfrastructure".

Hmm.  In one side, I tend to agree that it can go wrong easily.

OTOH, it simplifies the code well for us; as of now, we have two
callbacks for copying PCM memory from/to the device, distinct for
kernel and user pointers.  It's basically either copy_from_user() or
memcpy() of the given size depending on the caller.  The sockptr_t or
its variant would allow us to unify those to a single callback.

Of course, we can create yet another local type that is just for the
specific code -- or just (ab)use sockptr_t as is.  The fact is that it
can simplify the code well, which in turn make more maintainable.

Though, I have no strong opinion about this topic.  If you believe
this kind of code is way too dangerous, fine, we can go with the
current code.  OTOH, if the limited use is acceptable (as already seen
with sockptr_t), we can either re-use it or have own one.

(And yeah, iov_iter is there, but it's definitely overkill for the
purpose.)


Takashi

Takashi Iwai Aug. 9, 2023, 4:08 p.m. UTC | #6

On Wed, 09 Aug 2023 17:59:20 +0200,
Linus Torvalds wrote:
> 
> On Wed, 9 Aug 2023 at 07:44, Takashi Iwai <[e-mail]> wrote:
> >
> > The remaining question is whether the use of sockptr_t for other
> > subsystems as a generic pointer is a recommended / acceptable move...
> 
> Very much not recommended. sockptr_t is horrible too, but it was (part
> of) what made it possible to fix an even worse horrible historical
> mistake (ie getting rid of set_fs()).
> 
> So I detest sockptr_t. It's garbage. At the very minimum it should
> have had the length associated with it, not passed separately.
> 
> But it's garbage exactly because it allowed for conversion of some
> much much horrid legacy code with fairly minimal impact.
> 
> New code does *not* have that excuse.
> 
> DO NOT MIX USER AND KERNEL POINTERS. And don't add interfaces that
> think such mixing is ok. Pointers should be statically clearly of one
> type or the other, and never lied about.
> 
> Or you go all the way, and do that whole iterator thing, and make it
> very clear that you're doing something truly generic that can be
> passed fairly widely along across subsystem boundaries.

OK, it looks like we need to scratch the idea...


Takashi

Linus Torvalds Aug. 9, 2023, 5:01 p.m. UTC | #7

On Wed, 9 Aug 2023 at 09:05, Takashi Iwai <[e-mail]> wrote:
>
> OTOH, it simplifies the code well for us; as of now, we have two
> callbacks for copying PCM memory from/to the device, distinct for
> kernel and user pointers.  It's basically either copy_from_user() or
> memcpy() of the given size depending on the caller.  The sockptr_t or
> its variant would allow us to unify those to a single callback.

I didn't see the follow-up patches that use this, but...

> (And yeah, iov_iter is there, but it's definitely overkill for the
> purpose.)

You can actually use a "simplified form" of iov_iter, and it's not all that bad.

If the actual copying operation is just a memcpy, you're all set: just
do copy_to/from_iter(), and it's a really nice interface, and you
don't have to carry "ptr+size" things around.

And we now have a simple way to generate simple iov_iter's, so
*creating* the iter is trivial too:

        struct iov_iter iter;
        int ret = import_ubuf(ITER_SRC/DEST, uptr, len, &iter);

        if (unlikely(ret < 0))
                return ret;

and you're all done. You can now pass '&iter' around, and it has a
nice user pointer and a range in it, and copying that thing is easy.

Perhaps somewhat strangely (*) we don't have the same for a simple
kernel buffer, but adding that wouldn't be hard. You either end up
using a 'kvec', or we could even add something like ITER_KBUF if it
really matters.

Right now the kernel buffer init is a *bit* more involved than the
above ubuf case:

        struct iov_iter iter;
        struct kvec kvec = { kptr, len};

        iov_iter_kvec(&iter, ITER_SRC/DEST, &kvec, 1, len);

and that's maybe a *bit* annoying, but we could maybe simplify this
with some helper macros even without ITER_KBUF.

So yes, iov_iter does have some abstraction overhead, but it really
isn't that bad. And it *does* allow you to do a lot of things, and can
actually simplify the users quite a bit, exactly because it allows you
to just pass that single iter pointer around, and you automatically
have not just the user/kernel distinction, you have the buffer size,
and you have a lot of helper functions to use it.

I really think that if you want a user-or-kernel buffer interface, you
should use these things.

Please? At least look into it.

                 Linus

(*) Well, not so strange - we've just never needed it.

Linus Torvalds Aug. 9, 2023, 5:21 p.m. UTC | #8

On Wed, 9 Aug 2023 at 10:01, Linus Torvalds
<[e-mail]> wrote:
>
> Right now the kernel buffer init is a *bit* more involved than the
> above ubuf case:
>
>         struct iov_iter iter;
>         struct kvec kvec = { kptr, len};
>
>         iov_iter_kvec(&iter, ITER_SRC/DEST, &kvec, 1, len);
>
> and that's maybe a *bit* annoying, but we could maybe simplify this
> with some helper macros even without ITER_KBUF.

Looking at the diff that Christoph quoted, you possibly also want

        strncpy_from_iov()

and honestly, that's a bit of an odd operation for the traditional
iov_iter use, but it certainly shouldn't be _hard_ to implement.

I'd probably initially implement it as a special case that only deals
with the "one single buffer" case (whether user space or kernel
space), since that would presumably be what you'd ever have, but
extending it to the generic case later if people actually need it
would not be problematic - those "iterate_and_advance()" macros in
lib/iovec.c are all about that horror.

               Linus

Takashi Iwai Aug. 9, 2023, 6:08 p.m. UTC | #9

On Wed, 09 Aug 2023 19:01:50 +0200,
Linus Torvalds wrote:
> 
> On Wed, 9 Aug 2023 at 09:05, Takashi Iwai <[e-mail]> wrote:
> >
> > OTOH, it simplifies the code well for us; as of now, we have two
> > callbacks for copying PCM memory from/to the device, distinct for
> > kernel and user pointers.  It's basically either copy_from_user() or
> > memcpy() of the given size depending on the caller.  The sockptr_t or
> > its variant would allow us to unify those to a single callback.
> 
> I didn't see the follow-up patches that use this, but...
> 
> > (And yeah, iov_iter is there, but it's definitely overkill for the
> > purpose.)
> 
> You can actually use a "simplified form" of iov_iter, and it's not all that bad.
> 
> If the actual copying operation is just a memcpy, you're all set: just
> do copy_to/from_iter(), and it's a really nice interface, and you
> don't have to carry "ptr+size" things around.
> 
> And we now have a simple way to generate simple iov_iter's, so
> *creating* the iter is trivial too:
> 
>         struct iov_iter iter;
>         int ret = import_ubuf(ITER_SRC/DEST, uptr, len, &iter);
> 
>         if (unlikely(ret < 0))
>                 return ret;
> 
> and you're all done. You can now pass '&iter' around, and it has a
> nice user pointer and a range in it, and copying that thing is easy.
> 
> Perhaps somewhat strangely (*) we don't have the same for a simple
> kernel buffer, but adding that wouldn't be hard. You either end up
> using a 'kvec', or we could even add something like ITER_KBUF if it
> really matters.
> 
> Right now the kernel buffer init is a *bit* more involved than the
> above ubuf case:
> 
>         struct iov_iter iter;
>         struct kvec kvec = { kptr, len};
> 
>         iov_iter_kvec(&iter, ITER_SRC/DEST, &kvec, 1, len);
> 
> and that's maybe a *bit* annoying, but we could maybe simplify this
> with some helper macros even without ITER_KBUF.
> 
> So yes, iov_iter does have some abstraction overhead, but it really
> isn't that bad. And it *does* allow you to do a lot of things, and can
> actually simplify the users quite a bit, exactly because it allows you
> to just pass that single iter pointer around, and you automatically
> have not just the user/kernel distinction, you have the buffer size,
> and you have a lot of helper functions to use it.
> 
> I really think that if you want a user-or-kernel buffer interface, you
> should use these things.
> 
> Please? At least look into it.

All sounds convincing, I'll take a look tomorrow.  Thanks!


Takashi

> 
>                  Linus
> 
> (*) Well, not so strange - we've just never needed it.
>

Andy Shevchenko Aug. 10, 2023, 2:48 p.m. UTC | #10

On Wed, Aug 09, 2023 at 08:08:30PM +0200, Takashi Iwai wrote:
> On Wed, 09 Aug 2023 19:01:50 +0200,
> Linus Torvalds wrote:
> > On Wed, 9 Aug 2023 at 09:05, Takashi Iwai <[e-mail]> wrote:

...

> > Please? At least look into it.
> 
> All sounds convincing, I'll take a look tomorrow.  Thanks!

Nice discussion happened while I was sleeping / busy with some personal stuff.
Thank you, Linus, for all insights, it's educational.

Takashi Iwai Aug. 11, 2023, 1:54 p.m. UTC | #11

On Wed, 09 Aug 2023 20:08:30 +0200,
Takashi Iwai wrote:
> 
> On Wed, 09 Aug 2023 19:01:50 +0200,
> Linus Torvalds wrote:
> > 
> > On Wed, 9 Aug 2023 at 09:05, Takashi Iwai <[e-mail]> wrote:
> > >
> > > OTOH, it simplifies the code well for us; as of now, we have two
> > > callbacks for copying PCM memory from/to the device, distinct for
> > > kernel and user pointers.  It's basically either copy_from_user() or
> > > memcpy() of the given size depending on the caller.  The sockptr_t or
> > > its variant would allow us to unify those to a single callback.
> > 
> > I didn't see the follow-up patches that use this, but...
> > 
> > > (And yeah, iov_iter is there, but it's definitely overkill for the
> > > purpose.)
> > 
> > You can actually use a "simplified form" of iov_iter, and it's not all that bad.
> > 
> > If the actual copying operation is just a memcpy, you're all set: just
> > do copy_to/from_iter(), and it's a really nice interface, and you
> > don't have to carry "ptr+size" things around.
> > 
> > And we now have a simple way to generate simple iov_iter's, so
> > *creating* the iter is trivial too:
> > 
> >         struct iov_iter iter;
> >         int ret = import_ubuf(ITER_SRC/DEST, uptr, len, &iter);
> > 
> >         if (unlikely(ret < 0))
> >                 return ret;
> > 
> > and you're all done. You can now pass '&iter' around, and it has a
> > nice user pointer and a range in it, and copying that thing is easy.
> > 
> > Perhaps somewhat strangely (*) we don't have the same for a simple
> > kernel buffer, but adding that wouldn't be hard. You either end up
> > using a 'kvec', or we could even add something like ITER_KBUF if it
> > really matters.
> > 
> > Right now the kernel buffer init is a *bit* more involved than the
> > above ubuf case:
> > 
> >         struct iov_iter iter;
> >         struct kvec kvec = { kptr, len};
> > 
> >         iov_iter_kvec(&iter, ITER_SRC/DEST, &kvec, 1, len);
> > 
> > and that's maybe a *bit* annoying, but we could maybe simplify this
> > with some helper macros even without ITER_KBUF.
> > 
> > So yes, iov_iter does have some abstraction overhead, but it really
> > isn't that bad. And it *does* allow you to do a lot of things, and can
> > actually simplify the users quite a bit, exactly because it allows you
> > to just pass that single iter pointer around, and you automatically
> > have not just the user/kernel distinction, you have the buffer size,
> > and you have a lot of helper functions to use it.
> > 
> > I really think that if you want a user-or-kernel buffer interface, you
> > should use these things.
> > 
> > Please? At least look into it.
> 
> All sounds convincing, I'll take a look tomorrow.  Thanks!

FYI, I rewrote and tested patches, and it looks promising.

The only missing piece in the upstream side was the export of
import_ubuf().


Takashi

David Laight Aug. 14, 2023, 4:06 p.m. UTC | #12

From: Linus Torvalds 
> Sent: 09 August 2023 16:59
> 
> On Wed, 9 Aug 2023 at 07:44, Takashi Iwai <[e-mail]> wrote:
> >
> > The remaining question is whether the use of sockptr_t for other
> > subsystems as a generic pointer is a recommended / acceptable move...
> 
> Very much not recommended. sockptr_t is horrible too, but it was (part
> of) what made it possible to fix an even worse horrible historical
> mistake (ie getting rid of set_fs()).
> 
> So I detest sockptr_t. It's garbage. At the very minimum it should
> have had the length associated with it, not passed separately.

FWIW I've thought you'd want something like:
struct ptr_arg {
	void          *kernel;
	void __user   *user;
	unsigned int  kernel_len;
	unsigned int  user_len;
};

Then [gs]etsockopt() could copy short user buffers into
kernel space (eg on stack) while allowing code that needs
very large buffers (eg some sctp options) to directly
access a userspace buffer.

There certainly used to be sockopt calls where the user
didn't pass the correct/any length.
They might all have been in decnet.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Patch

diff --git a/include/linux/uniptr.h b/include/linux/uniptr.h
new file mode 100644
index 000000000000..f7994d3a45eb
--- /dev/null
+++ b/include/linux/uniptr.h
@@ -0,0 +1,121 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Support for "universal" pointers that can point to either kernel or userspace
+ * memory.
+ *
+ * Original code from sockptr.h
+ *    Copyright (c) 2020 Christoph Hellwig
+ */
+#ifndef _LINUX_UNIPTR_H
+#define _LINUX_UNIPTR_H
+
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.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 */