| Message ID | 20200723060908.50081-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] bpfilter: fix up a sparse annotation | expand |
On Thursday 2020-07-23 08:08, Christoph Hellwig wrote: >+typedef struct { >+ union { >+ void *kernel; >+ void __user *user; >+ }; >+ bool is_kernel : 1; >+} sockptr_t; >+ >+static inline bool sockptr_is_null(sockptr_t sockptr) >+{ >+ return !sockptr.user && !sockptr.kernel; >+} """If the member used to access the contents of a union is not the same as the member last used to store a value, the object representation of the value that was stored is reinterpreted as an object representation of the new type (this is known as type punning). If the size of the new type is larger than the size of the last-written type, the contents of the excess bytes are unspecified (and may be a trap representation)""" As I am not too versed with the consequences of trap representations, I will just point out that a future revision of the C standard may introduce (proposal N2362) stronger C++-like requirements; as for union, that would imply a simple: """It's undefined behavior to read from the member of the union that wasn't most recently written.""" [cppreference.com] So, in the spirit of copy_from/to_sockptr, the is_null function should read { return sockptr.is_kernel ? !sockptr.user : !sockptr.kernel; }
On Wed, Jul 22, 2020 at 11:09 PM Christoph Hellwig <hch@lst.de> wrote: > > Add a uptr_t type that can hold a pointer to either a user or kernel > memory region, and simply helpers to copy to and from it. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/linux/sockptr.h | 104 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > create mode 100644 include/linux/sockptr.h > > diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h > new file mode 100644 > index 00000000000000..700856e13ea0c4 > --- /dev/null > +++ b/include/linux/sockptr.h > @@ -0,0 +1,104 @@ > +/* 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_SOCKPTR_H > +#define _LINUX_SOCKPTR_H > + > +#include <linux/slab.h> > +#include <linux/uaccess.h> > + > +typedef struct { > + union { > + void *kernel; > + void __user *user; > + }; > + bool is_kernel : 1; > +} sockptr_t; > I am not sure why you chose sockptr_t for something that really seems generic. Or is it really meant to be exclusive to setsockopt() and/or getsockopt() ? If the first user of this had been futex code, we would have used futexptr_t, I guess.
On Thu, Jul 23, 2020 at 09:40:27AM -0700, Eric Dumazet wrote: > I am not sure why you chose sockptr_t for something that really seems generic. > > Or is it really meant to be exclusive to setsockopt() and/or getsockopt() ? > > If the first user of this had been futex code, we would have used > futexptr_t, I guess. It was originally intended to be generic and called uptr_t, based on me misunderstanding that Linus wanted a file operation for it, which he absolutely didn't and hate with passion. So the plan is to only use it for setsockopt for now, although there are some arguments for also using it in sendmsg/recvmsg. There is no need to use it for getsockopt.
diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h new file mode 100644 index 00000000000000..700856e13ea0c4 --- /dev/null +++ b/include/linux/sockptr.h @@ -0,0 +1,104 @@ +/* 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_SOCKPTR_H +#define _LINUX_SOCKPTR_H + +#include <linux/slab.h> +#include <linux/uaccess.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) +{ + return !sockptr.user && !sockptr.kernel; +} + +static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size) +{ + if (!sockptr_is_kernel(src)) + return copy_from_user(dst, src.user, size); + memcpy(dst, src.kernel, size); + return 0; +} + +static inline int copy_to_sockptr(sockptr_t dst, const void *src, size_t size) +{ + if (!sockptr_is_kernel(dst)) + return copy_to_user(dst.user, src, size); + memcpy(dst.kernel, src, size); + return 0; +} + +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 void sockptr_advance(sockptr_t sockptr, size_t len) +{ + if (sockptr_is_kernel(sockptr)) + sockptr.kernel += len; + else + sockptr.user += len; +} + +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); +} + +#endif /* _LINUX_SOCKPTR_H */
Add a uptr_t type that can hold a pointer to either a user or kernel memory region, and simply helpers to copy to and from it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/sockptr.h | 104 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 include/linux/sockptr.h