In the mainline kernel, there is no quick mechanism to get the virtual
memory size of the current process from userspace.
Despite the current state of affairs, this information is available to the
user through several means, one being a linear search of the entire address
space. This is an inefficient use of cpu cycles.
A component of the libhugetlb kernel test does exactly this, and as
systems' address spaces increase beyond 32-bits, this method becomes
exceedingly tedious.
For example, on a ppc64le system with a 47-bit address space, the linear
search causes the test to hang for some unknown amount of time. I
couldn't give you an exact number because I just ran it for about 10-20
minutes and went to go do something else, probably to get coffee or
something, and when I came back, I just killed the test and patched it
to use this new mechanism. I re-ran my new version of the test using a
kernel with this patch, and of course it passed through the previously
bottlenecking codepath nearly instantaneously.
As such, I propose that the prctl syscall be extended to include the
option to retrieve TASK_SIZE from the kernel.
This patch will allow us to upgrade an O(n) codepath to O(1) in an
architecture-independent manner, and provide a mechanism for others
to do the same.
Joel Savitz(2):
sys/prctl: add PR_GET_TASK_SIZE option to prctl(2)
prctl.2: Document the new PR_GET_TASK_SIZE option
include/uapi/linux/prctl.h | 3 +++
kernel/sys.c | 10 ++++++++++
2 files changed, 13 insertions(+)
man2/prctl.2 | 9 +++++++++
1 file changed, 9 insertions(+)
--
2.18.1
When PR_GET_TASK_SIZE is passed to prctl, the kernel will attempt to
copy the value of TASK_SIZE to the userspace address in arg2.
Suggested-by: Alexey Dobriyan <[email protected]>
Signed-off-by: Joel Savitz <[email protected]>
---
include/uapi/linux/prctl.h | 3 +++
kernel/sys.c | 10 ++++++++++
2 files changed, 13 insertions(+)
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 094bb03b9cc2..2335fe0a8db8 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -229,4 +229,7 @@ struct prctl_mm_map {
# define PR_PAC_APDBKEY (1UL << 3)
# define PR_PAC_APGAKEY (1UL << 4)
+/* Get the process virtual memory size */
+#define PR_GET_TASK_SIZE 55
+
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 12df0e5434b8..7ced7dbd035d 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2252,6 +2252,13 @@ static int propagate_has_child_subreaper(struct task_struct *p, void *data)
return 1;
}
+static int prctl_get_tasksize(void __user * uaddr)
+{
+ unsigned long task_size = TASK_SIZE;
+ return copy_to_user(uaddr, &task_size, sizeof(unsigned long))
+ ? -EFAULT : 0;
+}
+
int __weak arch_prctl_spec_ctrl_get(struct task_struct *t, unsigned long which)
{
return -EINVAL;
@@ -2486,6 +2493,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
return -EINVAL;
error = PAC_RESET_KEYS(me, arg2);
break;
+ case PR_GET_TASK_SIZE:
+ error = prctl_get_tasksize((void *)arg2) ;
+ break;
default:
error = -EINVAL;
break;
--
2.18.1
Add a short explanation of the new PR_GET_TASK_SIZE option for the benefit
of future generations.
Signed-off-by: Joel Savitz <[email protected]>
---
man2/prctl.2 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/man2/prctl.2 b/man2/prctl.2
index 06d8e13c7..35a6a3919 100644
--- a/man2/prctl.2
+++ b/man2/prctl.2
@@ -49,6 +49,7 @@
.\" 2013-01-10 Kees Cook, document PR_SET_PTRACER
.\" 2012-02-04 Michael Kerrisk, document PR_{SET,GET}_CHILD_SUBREAPER
.\" 2014-11-10 Dave Hansen, document PR_MPX_{EN,DIS}ABLE_MANAGEMENT
+.\" 2019-05-02 Joel Savitz, document PR_GET_TASK_SIZE
.\"
.\"
.TH PRCTL 2 2019-03-06 "Linux" "Linux Programmer's Manual"
@@ -1375,6 +1376,14 @@ system call on Tru64).
for information on versions and architectures)
Return unaligned access control bits, in the location pointed to by
.IR "(unsigned int\ *) arg2" .
+.TP
+.B PR_GET_TASK_SIZE
+Copy the value of TASK_SIZE to the userspace address in
+.IR "(unsigned long\ *) arg2" .
+Return
+.B EFAULT
+if this operation fails.
+
.SH RETURN VALUE
On success,
.BR PR_GET_DUMPABLE ,
--
2.18.1
On 5/2/19 4:52 PM, Joel Savitz wrote:
> In the mainline kernel, there is no quick mechanism to get the virtual
> memory size of the current process from userspace.
>
> Despite the current state of affairs, this information is available to the
> user through several means, one being a linear search of the entire address
> space. This is an inefficient use of cpu cycles.
>
> A component of the libhugetlb kernel test does exactly this, and as
> systems' address spaces increase beyond 32-bits, this method becomes
> exceedingly tedious.
>
> For example, on a ppc64le system with a 47-bit address space, the linear
> search causes the test to hang for some unknown amount of time. I
> couldn't give you an exact number because I just ran it for about 10-20
> minutes and went to go do something else, probably to get coffee or
> something, and when I came back, I just killed the test and patched it
> to use this new mechanism. I re-ran my new version of the test using a
> kernel with this patch, and of course it passed through the previously
> bottlenecking codepath nearly instantaneously.
>
> As such, I propose that the prctl syscall be extended to include the
> option to retrieve TASK_SIZE from the kernel.
>
> This patch will allow us to upgrade an O(n) codepath to O(1) in an
> architecture-independent manner, and provide a mechanism for others
> to do the same.
>
> Joel Savitz(2):
> sys/prctl: add PR_GET_TASK_SIZE option to prctl(2)
> prctl.2: Document the new PR_GET_TASK_SIZE option
>
> include/uapi/linux/prctl.h | 3 +++
> kernel/sys.c | 10 ++++++++++
> 2 files changed, 13 insertions(+)
>
> man2/prctl.2 | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> --
> 2.18.1
What did you change in v2 versus v1?
Cheers,
Longman
On Thu, May 02, 2019 at 05:01:38PM -0400, Waiman Long wrote:
>
> What did you change in v2 versus v1?
Seems unsigned long long has been changed to unsigned long.
On Thu, May 02, 2019 at 04:52:21PM -0400, Joel Savitz wrote:
>
> +static int prctl_get_tasksize(void __user * uaddr)
> +{
> + unsigned long task_size = TASK_SIZE;
> + return copy_to_user(uaddr, &task_size, sizeof(unsigned long))
> + ? -EFAULT : 0;
> +}
Won't be possible to use put_user here? Something like
static int prctl_get_tasksize(unsigned long __user *uaddr)
{
return put_user(TASK_SIZE, uaddr) ? -EFAULT : 0;
}
Yes, this the change, thanks to the suggestion of Yury Norov.
I also now explicitly mention the expected userspace destination type
in the manpage patch.
Best,
Joel Savitz
On Thu, May 2, 2019 at 5:10 PM Cyrill Gorcunov <[email protected]> wrote:
>
> On Thu, May 02, 2019 at 05:01:38PM -0400, Waiman Long wrote:
> >
> > What did you change in v2 versus v1?
>
> Seems unsigned long long has been changed to unsigned long.
On Thu, May 02, 2019 at 04:52:21PM -0400, Joel Savitz wrote:
> When PR_GET_TASK_SIZE is passed to prctl, the kernel will attempt to
> copy the value of TASK_SIZE to the userspace address in arg2.
>
> Suggested-by: Alexey Dobriyan <[email protected]>
> Signed-off-by: Joel Savitz <[email protected]>
> ---
> include/uapi/linux/prctl.h | 3 +++
> kernel/sys.c | 10 ++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 094bb03b9cc2..2335fe0a8db8 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -229,4 +229,7 @@ struct prctl_mm_map {
> # define PR_PAC_APDBKEY (1UL << 3)
> # define PR_PAC_APGAKEY (1UL << 4)
>
> +/* Get the process virtual memory size */
> +#define PR_GET_TASK_SIZE 55
> +
> #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 12df0e5434b8..7ced7dbd035d 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2252,6 +2252,13 @@ static int propagate_has_child_subreaper(struct task_struct *p, void *data)
> return 1;
> }
>
> +static int prctl_get_tasksize(void __user * uaddr)
> +{
> + unsigned long task_size = TASK_SIZE;
> + return copy_to_user(uaddr, &task_size, sizeof(unsigned long))
> + ? -EFAULT : 0;
Minor pick: I would keep the all of the ternary statement above in the
same line, to help on improving readability (even though it bursts a
little longer than 80 columns of text)
> +}
> +
> int __weak arch_prctl_spec_ctrl_get(struct task_struct *t, unsigned long which)
> {
> return -EINVAL;
> @@ -2486,6 +2493,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> return -EINVAL;
> error = PAC_RESET_KEYS(me, arg2);
> break;
> + case PR_GET_TASK_SIZE:
> + error = prctl_get_tasksize((void *)arg2) ;
> + break;
> default:
> error = -EINVAL;
> break;
> --
> 2.18.1
>
On Thu, May 02, 2019 at 04:52:20PM -0400, Joel Savitz wrote:
> In the mainline kernel, there is no quick mechanism to get the virtual
> memory size of the current process from userspace.
>
> Despite the current state of affairs, this information is available to the
> user through several means, one being a linear search of the entire address
> space. This is an inefficient use of cpu cycles.
>
> A component of the libhugetlb kernel test does exactly this, and as
> systems' address spaces increase beyond 32-bits, this method becomes
> exceedingly tedious.
>
> For example, on a ppc64le system with a 47-bit address space, the linear
> search causes the test to hang for some unknown amount of time. I
> couldn't give you an exact number because I just ran it for about 10-20
> minutes and went to go do something else, probably to get coffee or
> something, and when I came back, I just killed the test and patched it
> to use this new mechanism. I re-ran my new version of the test using a
> kernel with this patch, and of course it passed through the previously
> bottlenecking codepath nearly instantaneously.
>
> As such, I propose that the prctl syscall be extended to include the
> option to retrieve TASK_SIZE from the kernel.
>
> This patch will allow us to upgrade an O(n) codepath to O(1) in an
> architecture-independent manner, and provide a mechanism for others
> to do the same.
>
> Joel Savitz(2):
> sys/prctl: add PR_GET_TASK_SIZE option to prctl(2)
> prctl.2: Document the new PR_GET_TASK_SIZE option
>
> include/uapi/linux/prctl.h | 3 +++
> kernel/sys.c | 10 ++++++++++
> 2 files changed, 13 insertions(+)
>
> man2/prctl.2 | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> --
> 2.18.1
>
If you decide to repost patch 1/2 to sort out the minor nit I
pointed out, you can keep my ack.
Acked-by: Rafael Aquini <[email protected]>
чт, 2 мая 2019 г. в 14:23, Joel Savitz <[email protected]>:
>
> Yes, this the change, thanks to the suggestion of Yury Norov.
Joel, could you please stop top-posting?
> I also now explicitly mention the expected userspace destination type
> in the manpage patch.
>
> Best,
> Joel Savitz
>
>
> On Thu, May 2, 2019 at 5:10 PM Cyrill Gorcunov <[email protected]> wrote:
> >
> > On Thu, May 02, 2019 at 05:01:38PM -0400, Waiman Long wrote:
> > >
> > > What did you change in v2 versus v1?
> >
> > Seems unsigned long long has been changed to unsigned long.
Sorry guys, I replied to Joel, but accidentally dropped the folks.
Find discussion below.
чт, 2 мая 2019 г. в 13:50, Joel Savitz <[email protected]>:
>
> While I disagree that kernel memory is exposed, as the 8-byte
> (unsigned long long) value of task_size is initialized by the
> assignment of TASK_SIZE, I agree with your suggestion, as the current
> code may corrupt the userspace stack of the caller unless provided
> with the address of an unsigned long long, an unusual type to store a
> value of word size.
>
> As such, I have adopted your suggestion and added type information to
> my manpage patch. Expect the v2 to be posted shortly.
>
> Thank you for your review.
>
> Best,
> Joel Savitz
>
> On Thu, May 2, 2019 at 3:41 PM Yury Norov <[email protected]> wrote:
> >
> > чт, 2 мая 2019 г. в 12:15, Joel Savitz <[email protected]>:
> > >
> > > When PR_GET_TASK_SIZE is passed to prctl, the kernel will attempt to
> > > copy the value of TASK_SIZE to the userspace address in arg2.
> >
> > but you copy the value of task_size.
> >
> > > Suggested-by: Alexey Dobriyan <[email protected]>
> > > Signed-off-by: Joel Savitz <[email protected]>
> > > ---
> > > include/uapi/linux/prctl.h | 3 +++
> > > kernel/sys.c | 10 ++++++++++
> > > 2 files changed, 13 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> > > index 094bb03b9cc2..2335fe0a8db8 100644
> > > --- a/include/uapi/linux/prctl.h
> > > +++ b/include/uapi/linux/prctl.h
> > > @@ -229,4 +229,7 @@ struct prctl_mm_map {
> > > # define PR_PAC_APDBKEY (1UL << 3)
> > > # define PR_PAC_APGAKEY (1UL << 4)
> > >
> > > +/* Get the process virtual memory size */
> > > +#define PR_GET_TASK_SIZE 55
> > > +
> > > #endif /* _LINUX_PRCTL_H */
> > > diff --git a/kernel/sys.c b/kernel/sys.c
> > > index 12df0e5434b8..7ced7dbd035d 100644
> > > --- a/kernel/sys.c
> > > +++ b/kernel/sys.c
> > > @@ -2252,6 +2252,13 @@ static int propagate_has_child_subreaper(struct task_struct *p, void *data)
> > > return 1;
> > > }
> > >
> > > +static int prctl_get_tasksize(void __user * uaddr)
> > > +{
> > > + unsigned long long task_size = TASK_SIZE;
> > > + return copy_to_user(uaddr, &task_size, sizeof(unsigned long long))
> > > + ? -EFAULT : 0;
> > > +}
> > > +
> >
> > task_size is unsigned long. On 32-bit systems you will end up exposing 4 bytes
> > of kernel memory. You should switch to sizeof(unsigned long).
> >
> > Your code is broken for compat arches. Take a look at the definition
> > of TASK_SIZE
> > for arm64, for example.
> >
> > Thanks,
> > Yury
> Won't be possible to use put_user here? Something like
>
> static int prctl_get_tasksize(unsigned long __user *uaddr)
> {
> return put_user(TASK_SIZE, uaddr) ? -EFAULT : 0;
> }
What would be the benefit of using put_user() over copy_to_user() in
this context?
чт, 2 мая 2019 г. в 13:52, Joel Savitz <[email protected]>:
>
> When PR_GET_TASK_SIZE is passed to prctl, the kernel will attempt to
> copy the value of TASK_SIZE to the userspace address in arg2.
>
> Suggested-by: Alexey Dobriyan <[email protected]>
> Signed-off-by: Joel Savitz <[email protected]>
> ---
> include/uapi/linux/prctl.h | 3 +++
> kernel/sys.c | 10 ++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 094bb03b9cc2..2335fe0a8db8 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -229,4 +229,7 @@ struct prctl_mm_map {
> # define PR_PAC_APDBKEY (1UL << 3)
> # define PR_PAC_APGAKEY (1UL << 4)
>
> +/* Get the process virtual memory size */
> +#define PR_GET_TASK_SIZE 55
> +
> #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 12df0e5434b8..7ced7dbd035d 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2252,6 +2252,13 @@ static int propagate_has_child_subreaper(struct task_struct *p, void *data)
> return 1;
> }
>
> +static int prctl_get_tasksize(void __user * uaddr)
> +{
> + unsigned long task_size = TASK_SIZE;
> + return copy_to_user(uaddr, &task_size, sizeof(unsigned long))
> + ? -EFAULT : 0;
> +}
> +
Joel, you missed my point from the comment to v1.
This is still broken for compat architectures. On 64 bit machines
compat userspace
has unsigned long as u32, and therefore you corrupt user data.
> int __weak arch_prctl_spec_ctrl_get(struct task_struct *t, unsigned long which)
> {
> return -EINVAL;
> @@ -2486,6 +2493,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> return -EINVAL;
> error = PAC_RESET_KEYS(me, arg2);
> break;
> + case PR_GET_TASK_SIZE:
> + error = prctl_get_tasksize((void *)arg2) ;
> + break;
> default:
> error = -EINVAL;
> break;
> --
> 2.18.1
>
чт, 2 мая 2019 г. в 13:52, Joel Savitz <[email protected]>:
>
> Add a short explanation of the new PR_GET_TASK_SIZE option for the benefit
> of future generations.
>
> Signed-off-by: Joel Savitz <[email protected]>
> ---
> man2/prctl.2 | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/man2/prctl.2 b/man2/prctl.2
> index 06d8e13c7..35a6a3919 100644
> --- a/man2/prctl.2
> +++ b/man2/prctl.2
> @@ -49,6 +49,7 @@
> .\" 2013-01-10 Kees Cook, document PR_SET_PTRACER
> .\" 2012-02-04 Michael Kerrisk, document PR_{SET,GET}_CHILD_SUBREAPER
> .\" 2014-11-10 Dave Hansen, document PR_MPX_{EN,DIS}ABLE_MANAGEMENT
> +.\" 2019-05-02 Joel Savitz, document PR_GET_TASK_SIZE
> .\"
> .\"
> .TH PRCTL 2 2019-03-06 "Linux" "Linux Programmer's Manual"
> @@ -1375,6 +1376,14 @@ system call on Tru64).
> for information on versions and architectures)
> Return unaligned access control bits, in the location pointed to by
> .IR "(unsigned int\ *) arg2" .
> +.TP
> +.B PR_GET_TASK_SIZE
> +Copy the value of TASK_SIZE to the userspace address in
> +.IR "(unsigned long\ *) arg2" .
This is a bad idea to use pointers to size-undefined types in interface because
that way you have to introduce compat versions of interface functions.
I'd recommend you to use u64 or unsigned long long here.
The comment not clear for reader not familiar with kernel internals.
Can you rephrase
TASK_SIZE like 'the (next after) highest possible userspace address',
or similar?
For the updated version could you please CC to [email protected]?
> +Return
> +.B EFAULT
> +if this operation fails.
> +
> .SH RETURN VALUE
> On success,
> .BR PR_GET_DUMPABLE ,
> --
> 2.18.1
>
On Thu, May 02, 2019 at 03:23:12PM -0700, Yury Norov wrote:
> чт, 2 мая 2019 г. в 13:52, Joel Savitz <[email protected]>:
> >
> > Add a short explanation of the new PR_GET_TASK_SIZE option for the benefit
> > of future generations.
> >
> > Signed-off-by: Joel Savitz <[email protected]>
> > ---
> > man2/prctl.2 | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/man2/prctl.2 b/man2/prctl.2
> > index 06d8e13c7..35a6a3919 100644
> > --- a/man2/prctl.2
> > +++ b/man2/prctl.2
> > @@ -49,6 +49,7 @@
> > .\" 2013-01-10 Kees Cook, document PR_SET_PTRACER
> > .\" 2012-02-04 Michael Kerrisk, document PR_{SET,GET}_CHILD_SUBREAPER
> > .\" 2014-11-10 Dave Hansen, document PR_MPX_{EN,DIS}ABLE_MANAGEMENT
> > +.\" 2019-05-02 Joel Savitz, document PR_GET_TASK_SIZE
> > .\"
> > .\"
> > .TH PRCTL 2 2019-03-06 "Linux" "Linux Programmer's Manual"
> > @@ -1375,6 +1376,14 @@ system call on Tru64).
> > for information on versions and architectures)
> > Return unaligned access control bits, in the location pointed to by
> > .IR "(unsigned int\ *) arg2" .
> > +.TP
> > +.B PR_GET_TASK_SIZE
> > +Copy the value of TASK_SIZE to the userspace address in
> > +.IR "(unsigned long\ *) arg2" .
>
> This is a bad idea to use pointers to size-undefined types in interface because
> that way you have to introduce compat versions of interface functions.
> I'd recommend you to use u64 or unsigned long long here.
>
unsigned long long seems to make little sense too as prctl(2) extra arguments
are of unsigned long type (good for passing the pointer address, in this case).
a pointer to an unsigned long var is OK for native builds, and for the
compat mode issue you correctly pointed out, the storage size needs to be
dealt with at the kernel side, by checking test_thread_flag(TIF_ADDR32),
before proceeding with copy_to_user().
> The comment not clear for reader not familiar with kernel internals.
> Can you rephrase
> TASK_SIZE like 'the (next after) highest possible userspace address',
> or similar?
>
> For the updated version could you please CC to [email protected]?
>
> > +Return
> > +.B EFAULT
> > +if this operation fails.
> > +
> > .SH RETURN VALUE
> > On success,
> > .BR PR_GET_DUMPABLE ,
> > --
> > 2.18.1
> >
On Thu, May 02, 2019 at 05:46:08PM -0400, Joel Savitz wrote:
> > Won't be possible to use put_user here? Something like
> >
> > static int prctl_get_tasksize(unsigned long __user *uaddr)
> > {
> > return put_user(TASK_SIZE, uaddr) ? -EFAULT : 0;
> > }
>
> What would be the benefit of using put_user() over copy_to_user() in
> this context?
It is a common pattern to use put_user with native types, where
copy_to_user more biased for composed types transfer.
From: Cyrill Gorcunov
> Sent: 03 May 2019 09:32
> On Thu, May 02, 2019 at 05:46:08PM -0400, Joel Savitz wrote:
> > > Won't be possible to use put_user here? Something like
> > >
> > > static int prctl_get_tasksize(unsigned long __user *uaddr)
> > > {
> > > return put_user(TASK_SIZE, uaddr) ? -EFAULT : 0;
> > > }
> >
> > What would be the benefit of using put_user() over copy_to_user() in
> > this context?
>
> It is a common pattern to use put_user with native types, where
> copy_to_user more biased for composed types transfer.
It also removes all the crappy code that checks whether the
kernel buffer is valid.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)