Re: [PATCH v2 1/4] lib: introduce copy_struct_from_user() helper
On Fri, Sep 27, 2019 at 11:07:36AM +1000, Aleksa Sarai wrote:
> On 2019-09-26, Christian Brauner wrote:
> > On Thu, Sep 26, 2019 at 01:03:29AM +0200, Aleksa Sarai wrote:
> > > +int is_zeroed_user(const void __user *from, size_t size)
> > > +{
> > > + unsigned long val;
> > > + uintptr_t align = (uintptr_t) from % sizeof(unsigned long);
> > > +
> > > + if (unlikely(!size))
> > > + return true;
> >
> > You're returning "true" and another implicit boolean with (val == 0)
> > down below but -EFAULT in other places. But that function is int
> > is_zeroed_user() Would probably be good if you either switch to bool
> > is_zeroed_user() as the name suggests or rename the function and have
> > it return an int everywhere.
>
> I just checked, and in C11 (and presumably in older specs) it is
> guaranteed that "true" and "false" from have the values 1
> and 0 (respectively) [§7.18]. So this is perfectly well-defined.
>
If you declare a function as returning an int, return ints and don't mix
returning ints and "proper" C boolean types. This:
static int foo()
{
if (bla)
return true;
return -1;
}
is just messy.
>
> Personally, I think it's more readable to have:
>
> if (unlikely(size == 0))
> return true;
> /* ... */
> return (val == 0);
>
> compared to:
>
> if (unlikely(size == 0))
> return 1;
> /* ... */
> return val ? 0 : 1;
Just do:
if (unlikely(size == 0))
return 1;
/* ... */
return (val == 0);
You don't need to change the last return.
Also, as I said in a previous mail: Please wait for rc1 (that's just two
days) to be out so you can base your patchset on that as there are
changes in mainline that cause a merge conflict with your changes.
Thanks!
Christian
Re: [PATCH v2 1/4] lib: introduce copy_struct_from_user() helper
On 2019-09-26, Christian Brauner wrote:
> On Thu, Sep 26, 2019 at 01:03:29AM +0200, Aleksa Sarai wrote:
> > +int is_zeroed_user(const void __user *from, size_t size)
> > +{
> > + unsigned long val;
> > + uintptr_t align = (uintptr_t) from % sizeof(unsigned long);
> > +
> > + if (unlikely(!size))
> > + return true;
>
> You're returning "true" and another implicit boolean with (val == 0)
> down below but -EFAULT in other places. But that function is int
> is_zeroed_user() Would probably be good if you either switch to bool
> is_zeroed_user() as the name suggests or rename the function and have
> it return an int everywhere.
I just checked, and in C11 (and presumably in older specs) it is
guaranteed that "true" and "false" from have the values 1
and 0 (respectively) [§7.18]. So this is perfectly well-defined.
Personally, I think it's more readable to have:
if (unlikely(size == 0))
return true;
/* ... */
return (val == 0);
compared to:
if (unlikely(size == 0))
return 1;
/* ... */
return val ? 0 : 1;
But I will change the function name (to check_zeroed_user) to make it
clearer that it isn't returning a boolean and that you need to check for
negative returns.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
Re: [PATCH v2 1/4] lib: introduce copy_struct_from_user() helper
On Thu, Sep 26, 2019 at 01:03:29AM +0200, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases).
>
> While this interface exists for communication in both directions, only
> one interface is straightforward to have reasonable semantics for
> (userspace passing a struct to the kernel). For kernel returns to
> userspace, what the correct semantics are (whether there should be an
> error if userspace is unaware of a new extension) is very
> syscall-dependent and thus probably cannot be unified between syscalls
> (a good example of this problem is [1]).
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[2]). Future
> patches replace common uses of this pattern to make use of
> copy_struct_from_user().
>
> Some in-kernel selftests that insure that the handling of alignment and
> various byte patterns are all handled identically to memchr_inv() usage.
>
> [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
> robustify sched_read_attr() ABI logic and code")
>
> [2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes
> Signed-off-by: Aleksa Sarai
> ---
> include/linux/bitops.h | 7 +++
> include/linux/uaccess.h | 4 ++
> lib/strnlen_user.c | 8 +--
> lib/test_user_copy.c| 59 ++---
> lib/usercopy.c | 115
> 5 files changed, 180 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index cf074bce3eb3..a23f4c054768 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -4,6 +4,13 @@
> #include
> #include
>
> +/* Set bits in the first 'n' bytes when loaded from memory */
> +#ifdef __LITTLE_ENDIAN
> +# define aligned_byte_mask(n) ((1ul << 8*(n))-1)
> +#else
> +# define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
> +#endif
Nti: The style in bitops.h suggestes this should be:
+/* Set bits in the first 'n' bytes when loaded from memory */
+#ifdef __LITTLE_ENDIAN
+# define aligned_byte_mask(n) ((1UL << 8*(n))-1)
+#else
+# define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
+#endif
Using UL also makes 0xffUL clearer.
> +
> #define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
> #define BITS_TO_LONGS(nr)DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 34a038563d97..824569e309e4 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -230,6 +230,10 @@ static inline unsigned long
> __copy_from_user_inatomic_nocache(void *to,
>
> #endif /* ARCH_HAS_NOCACHE_UACCESS */
>
> +extern int is_zeroed_user(const void __user *from, size_t count);
> +extern int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize);
> +
> /*
> * probe_kernel_read(): safely attempt to read from a location
> * @dst: pointer to the buffer that shall take the data
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..39d588aaa8cd 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -2,16 +2,10 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> -/* Set bits in the first 'n' bytes when loaded from memory */
> -#ifdef __LITTLE_ENDIAN
> -# define aligned_byte_mask(n) ((1ul << 8*(n))-1)
> -#else
> -# define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
> -#endif
> -
> /*
> * Do a strnlen, return length of string *with* final '\0'.
> * 'count' is the user-supplied count, while 'max' is the
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index 67bcd5dfd847..f7cde3845ccc 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -31,14 +31,58 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg) \
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("%s\n", msg); \
> - cond; \
> +#define test(condition, msg, ...)\
> +({ \
> + int cond = (condition); \
> + if (cond) \
> + pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> +
Re: [PATCH v2 1/4] lib: introduce copy_struct_from_user() helper
Hi Aleksa,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-071752
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:15,
from include/asm-generic/bug.h:18,
from arch/sh/include/asm/bug.h:112,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from include/linux/mman.h:5,
from lib/test_user_copy.c:13:
lib/test_user_copy.c: In function 'test_is_zeroed_user':
include/linux/kern_levels.h:5:18: warning: format '%lu' expects argument of
type 'long unsigned int', but argument 5 has type 'size_t {aka unsigned int}'
[-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
#define KERN_WARNING KERN_SOH "4" /* warning conditions */
^~~~
include/linux/printk.h:306:9: note: in expansion of macro 'KERN_WARNING'
printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
^~~~
include/linux/printk.h:307:17: note: in expansion of macro 'pr_warning'
#define pr_warn pr_warning
^~
>> lib/test_user_copy.c:38:3: note: in expansion of macro 'pr_warn'
pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
^~~
>> lib/test_user_copy.c:77:11: note: in expansion of macro 'test'
ret |= test(retval != expected,
^~~~
include/linux/kern_levels.h:5:18: warning: format '%lu' expects argument of
type 'long unsigned int', but argument 6 has type 'size_t {aka unsigned int}'
[-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
#define KERN_WARNING KERN_SOH "4" /* warning conditions */
^~~~
include/linux/printk.h:306:9: note: in expansion of macro 'KERN_WARNING'
printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
^~~~
include/linux/printk.h:307:17: note: in expansion of macro 'pr_warning'
#define pr_warn pr_warning
^~
>> lib/test_user_copy.c:38:3: note: in expansion of macro 'pr_warn'
pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
^~~
>> lib/test_user_copy.c:77:11: note: in expansion of macro 'test'
ret |= test(retval != expected,
^~~~
vim +/pr_warn +38 lib/test_user_copy.c
33
34 #define test(condition, msg, ...)
\
35 ({
\
36 int cond = (condition);
\
37 if (cond)
\
> 38 pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__);
\
39 cond;
\
40 })
41
42 static int test_is_zeroed_user(char *kmem, char __user *umem, size_t
size)
43 {
44 int ret = 0;
45 size_t start, end, i;
46 size_t zero_start = size / 4;
47 size_t zero_end = size - zero_start;
48
49 /*
50 * We conduct a series of is_zeroed_user() tests on a block of
memory
51 * with the following byte-pattern (trying every possible
[start,end]
52 * pair):
53 *
54 * [ 00 ff 00 ff ... 00 00 00 00 ... ff 00 ff 00 ]
55 *
56 * And we verify that is_zeroed_user() acts identically to
memchr_inv().
57 */
58
59 for (i = 0; i < zero_start; i += 2)
60 kmem[i] = 0x00;
61 for (i = 1; i < zero_start; i += 2)
62 kmem[i] = 0xff;
63
64 for (i = zero_end; i < size; i += 2)
65
Re: [PATCH v2 1/4] lib: introduce copy_struct_from_user() helper
On Thu, Sep 26, 2019 at 01:03:29AM +0200, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases).
>
> While this interface exists for communication in both directions, only
> one interface is straightforward to have reasonable semantics for
> (userspace passing a struct to the kernel). For kernel returns to
> userspace, what the correct semantics are (whether there should be an
> error if userspace is unaware of a new extension) is very
> syscall-dependent and thus probably cannot be unified between syscalls
> (a good example of this problem is [1]).
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[2]). Future
> patches replace common uses of this pattern to make use of
> copy_struct_from_user().
>
> Some in-kernel selftests that insure that the handling of alignment and
> various byte patterns are all handled identically to memchr_inv() usage.
>
> [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
> robustify sched_read_attr() ABI logic and code")
>
> [2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes
> Signed-off-by: Aleksa Sarai
> ---
> include/linux/bitops.h | 7 +++
> include/linux/uaccess.h | 4 ++
> lib/strnlen_user.c | 8 +--
> lib/test_user_copy.c| 59 ++---
> lib/usercopy.c | 115
> 5 files changed, 180 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index cf074bce3eb3..a23f4c054768 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -4,6 +4,13 @@
> #include
> #include
>
> +/* Set bits in the first 'n' bytes when loaded from memory */
> +#ifdef __LITTLE_ENDIAN
> +# define aligned_byte_mask(n) ((1ul << 8*(n))-1)
> +#else
> +# define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
> +#endif
> +
> #define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
> #define BITS_TO_LONGS(nr)DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 34a038563d97..824569e309e4 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -230,6 +230,10 @@ static inline unsigned long
> __copy_from_user_inatomic_nocache(void *to,
>
> #endif /* ARCH_HAS_NOCACHE_UACCESS */
>
> +extern int is_zeroed_user(const void __user *from, size_t count);
> +extern int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize);
> +
> /*
> * probe_kernel_read(): safely attempt to read from a location
> * @dst: pointer to the buffer that shall take the data
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..39d588aaa8cd 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -2,16 +2,10 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> -/* Set bits in the first 'n' bytes when loaded from memory */
> -#ifdef __LITTLE_ENDIAN
> -# define aligned_byte_mask(n) ((1ul << 8*(n))-1)
> -#else
> -# define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
> -#endif
> -
> /*
> * Do a strnlen, return length of string *with* final '\0'.
> * 'count' is the user-supplied count, while 'max' is the
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index 67bcd5dfd847..f7cde3845ccc 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -31,14 +31,58 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg) \
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("%s\n", msg); \
> - cond; \
> +#define test(condition, msg, ...)\
> +({ \
> + int cond = (condition); \
> + if (cond) \
> + pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> + cond; \
> })
>
> +static int test_is_zeroed_user(char *kmem, char __user *umem, size_t size)
> +{
> + int ret = 0;
> + size_t start, end, i;
> + size_t zero_start = size / 4;
> + size_t zero_end = size - zero_start;
> +
> + /*
> + *
Re: [PATCH v2 1/4] lib: introduce copy_struct_from_user() helper
(Damn, I forgot to add Kees to Cc.)
On 2019-09-26, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases).
>
> While this interface exists for communication in both directions, only
> one interface is straightforward to have reasonable semantics for
> (userspace passing a struct to the kernel). For kernel returns to
> userspace, what the correct semantics are (whether there should be an
> error if userspace is unaware of a new extension) is very
> syscall-dependent and thus probably cannot be unified between syscalls
> (a good example of this problem is [1]).
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[2]). Future
> patches replace common uses of this pattern to make use of
> copy_struct_from_user().
>
> Some in-kernel selftests that insure that the handling of alignment and
> various byte patterns are all handled identically to memchr_inv() usage.
>
> [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
> robustify sched_read_attr() ABI logic and code")
>
> [2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes
> Signed-off-by: Aleksa Sarai
> ---
> include/linux/bitops.h | 7 +++
> include/linux/uaccess.h | 4 ++
> lib/strnlen_user.c | 8 +--
> lib/test_user_copy.c| 59 ++---
> lib/usercopy.c | 115
> 5 files changed, 180 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index cf074bce3eb3..a23f4c054768 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -4,6 +4,13 @@
> #include
> #include
>
> +/* Set bits in the first 'n' bytes when loaded from memory */
> +#ifdef __LITTLE_ENDIAN
> +# define aligned_byte_mask(n) ((1ul << 8*(n))-1)
> +#else
> +# define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
> +#endif
> +
> #define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
> #define BITS_TO_LONGS(nr)DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 34a038563d97..824569e309e4 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -230,6 +230,10 @@ static inline unsigned long
> __copy_from_user_inatomic_nocache(void *to,
>
> #endif /* ARCH_HAS_NOCACHE_UACCESS */
>
> +extern int is_zeroed_user(const void __user *from, size_t count);
> +extern int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize);
> +
> /*
> * probe_kernel_read(): safely attempt to read from a location
> * @dst: pointer to the buffer that shall take the data
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..39d588aaa8cd 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -2,16 +2,10 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> -/* Set bits in the first 'n' bytes when loaded from memory */
> -#ifdef __LITTLE_ENDIAN
> -# define aligned_byte_mask(n) ((1ul << 8*(n))-1)
> -#else
> -# define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
> -#endif
> -
> /*
> * Do a strnlen, return length of string *with* final '\0'.
> * 'count' is the user-supplied count, while 'max' is the
> diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
> index 67bcd5dfd847..f7cde3845ccc 100644
> --- a/lib/test_user_copy.c
> +++ b/lib/test_user_copy.c
> @@ -31,14 +31,58 @@
> # define TEST_U64
> #endif
>
> -#define test(condition, msg) \
> -({ \
> - int cond = (condition); \
> - if (cond) \
> - pr_warn("%s\n", msg); \
> - cond; \
> +#define test(condition, msg, ...)\
> +({ \
> + int cond = (condition); \
> + if (cond) \
> + pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__); \
> + cond; \
> })
>
> +static int test_is_zeroed_user(char *kmem, char __user *umem, size_t size)
> +{
> + int ret = 0;
> + size_t start, end, i;
> + size_t zero_start = size / 4;
> + size_t zero_end = size - zero_start;
> +
> + /*
