* Re: Can we drop upstream Linux x32 support?
From: H.J. Lu @ 2018-12-11 3:14 UTC (permalink / raw)
To: Andy Lutomirski
Cc: the arch/x86 maintainers, LKML, Linux API, H. Peter Anvin,
Peter Zijlstra, Borislav Petkov, Florian Weimer, Mike Frysinger,
Rich Felker, x32, Arnd Bergmann, Will Deacon, Catalin Marinas,
Linus Torvalds
In-Reply-To: <CALCETrXoRAibsbWa9nfbDrt0iEuebMnCMhSFg-d9W-J2g8mDjw@mail.gmail.com>
On Mon, Dec 10, 2018 at 5:23 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> Hi all-
>
> I'm seriously considering sending a patch to remove x32 support from
> upstream Linux. Here are some problems with it:
>
> 1. It's not entirely clear that it has users. As far as I know, it's
> supported on Gentoo and Debian, and the Debian popcon graph for x32
> has been falling off dramatically. I don't think that any enterprise
> distro has ever supported x32.
I have been posting x32 GCC results for years:
https://gcc.gnu.org/ml/gcc-testresults/2018-12/msg01358.html
> 2. The way that system calls work is very strange. Most syscalls on
> x32 enter through their *native* (i.e. not COMPAT_SYSCALL_DEFINE)
> entry point, and this is intentional. For example, adjtimex() uses
> the native entry, not the compat entry, because x32's struct timex
> matches the x86_64 layout. But a handful of syscalls have separate
This becomes less an issue with 64-bit time_t.
> entry points -- these are the syscalls starting at 512. These enter
> throuh the COMPAT_SYSCALL_DEFINE entry points.
>
> The x32 syscalls that are *not* in the 512 range violate all semblance
> of kernel syscall convention. In the syscall handlers,
> in_compat_syscall() returns true, but the COMPAT_SYSCALL_DEFINE entry
> is not invoked. This is nutty and risks breaking things when people
> refactor their syscall implementations. And no one tests these
> things. Similarly, if someone calls any of the syscalls below 512 but
> sets bit 31 in RAX, then the native entry will be called with
> in_compat_set().
>
> Conversely, if you call a syscall in the 512 range with bit 31
> *clear*, then the compat entry is set with in_compat_syscall()
> *clear*. This is also nutty.
This is to share syscalls between LP64 and ILP32 (x32) in x86-64 kernel.
> Finally, the kernel has a weird distinction between CONFIG_X86_X32_ABI
> and and CONFIG_X86_X32, which I suspect results in incorrect builds if
> the host doesn't have an x32 toolchain installed.
X86-64 binutils and GCC can be used to build x86-64 kernel with x32 support.
> I propose that we make CONFIG_X86_X32 depend on BROKEN for a release
> or two and then remove all the code if no one complains. If anyone
> wants to re-add it, IMO they're welcome to do so, but they need to do
> it in a way that is maintainable.
--
H.J.
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: hpa @ 2018-12-11 2:22 UTC (permalink / raw)
To: Linus Torvalds, Andrew Lutomirski
Cc: the arch/x86 maintainers, Linux List Kernel Mailing, linux-api,
Peter Zijlstra, bp, fweimer, vapier, hjl.tools, dalias, x32,
Arnd Bergmann, Will Deacon, Catalin Marinas
In-Reply-To: <CAHk-=wi_Kp=3XmGDdzmadzFSPFvuL+aAJ6ZPAR=o4z=KwYT2vw@mail.gmail.com>
On December 10, 2018 5:40:33 PM PST, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Mon, Dec 10, 2018 at 5:23 PM Andy Lutomirski <luto@kernel.org>
>wrote:
>>
>> I'm seriously considering sending a patch to remove x32 support from
>> upstream Linux. Here are some problems with it:
>
>I talked to Arnd (I think - we were talking about all the crazy ABI's,
>but maybe it was with somebody else) about exactly this in Edinburgh.
>
>Apparently the main real use case is for extreme benchmarking. It's
>the only use-case where the complexity of maintaining a whole
>development environment and distro is worth it, it seems. Apparently a
>number of Spec submissions have been done with the x32 model.
>
>I'm not opposed to trying to sunset the support, but let's see who
>complains..
>
> Linus
The use case aside, I need to address the technical issues in this post; some of the behaviors that Andy is pointing out area quite intentional, even if they are perhaps somewhat confusing at first glance. That being said, some were due to tradeoffs that might have been wrong.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Linus Torvalds @ 2018-12-11 1:40 UTC (permalink / raw)
To: Andrew Lutomirski
Cc: the arch/x86 maintainers, Linux List Kernel Mailing, linux-api,
Peter Anvin, Peter Zijlstra, bp, fweimer, vapier, hjl.tools,
dalias, x32, Arnd Bergmann, Will Deacon, Catalin Marinas
In-Reply-To: <CALCETrXoRAibsbWa9nfbDrt0iEuebMnCMhSFg-d9W-J2g8mDjw@mail.gmail.com>
On Mon, Dec 10, 2018 at 5:23 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> I'm seriously considering sending a patch to remove x32 support from
> upstream Linux. Here are some problems with it:
I talked to Arnd (I think - we were talking about all the crazy ABI's,
but maybe it was with somebody else) about exactly this in Edinburgh.
Apparently the main real use case is for extreme benchmarking. It's
the only use-case where the complexity of maintaining a whole
development environment and distro is worth it, it seems. Apparently a
number of Spec submissions have been done with the x32 model.
I'm not opposed to trying to sunset the support, but let's see who complains..
Linus
^ permalink raw reply
* Can we drop upstream Linux x32 support?
From: Andy Lutomirski @ 2018-12-11 1:23 UTC (permalink / raw)
To: X86 ML, LKML, Linux API, H. Peter Anvin, Peter Zijlstra,
Borislav Petkov, Florian Weimer, Mike Frysinger, H. J. Lu,
Rich Felker, x32, Arnd Bergmann, Will Deacon, Catalin Marinas,
Linus Torvalds
Hi all-
I'm seriously considering sending a patch to remove x32 support from
upstream Linux. Here are some problems with it:
1. It's not entirely clear that it has users. As far as I know, it's
supported on Gentoo and Debian, and the Debian popcon graph for x32
has been falling off dramatically. I don't think that any enterprise
distro has ever supported x32.
2. The way that system calls work is very strange. Most syscalls on
x32 enter through their *native* (i.e. not COMPAT_SYSCALL_DEFINE)
entry point, and this is intentional. For example, adjtimex() uses
the native entry, not the compat entry, because x32's struct timex
matches the x86_64 layout. But a handful of syscalls have separate
entry points -- these are the syscalls starting at 512. These enter
through the COMPAT_SYSCALL_DEFINE entry points.
The x32 syscalls that are *not* in the 512 range violate all semblance
of kernel syscall convention. In the syscall handlers,
in_compat_syscall() returns true, but the COMPAT_SYSCALL_DEFINE entry
is not invoked. This is nutty and risks breaking things when people
refactor their syscall implementations. And no one tests these
things. Similarly, if someone calls any of the syscalls below 512 but
sets bit 31 in RAX, then the native entry will be called with
in_compat_set().
Conversely, if you call a syscall in the 512 range with bit 31
*clear*, then the compat entry is set with in_compat_syscall()
*clear*. This is also nutty.
Finally, the kernel has a weird distinction between CONFIG_X86_X32_ABI
and and CONFIG_X86_X32, which I suspect results in incorrect builds if
the host doesn't have an x32 toolchain installed.
I propose that we make CONFIG_X86_X32 depend on BROKEN for a release
or two and then remove all the code if no one complains. If anyone
wants to re-add it, IMO they're welcome to do so, but they need to do
it in a way that is maintainable.
--Andy
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Randy Dunlap @ 2018-12-10 23:32 UTC (permalink / raw)
To: Carlos O'Donell, Jonathan Corbet
Cc: Greg KH, Daniel Colascione, linux-kernel, Joel Fernandes,
Linux API
In-Reply-To: <f3db1a75-e6f2-06a8-5264-a6807b7b4538@redhat.com>
On 12/10/18 9:39 AM, Carlos O'Donell wrote:
> On 12/10/18 11:27 AM, Jonathan Corbet wrote:
>> On Sat, 8 Dec 2018 20:38:56 -0800
>> Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>>> On 11/12/18 8:08 AM, Jonathan Corbet wrote:
>>>> On Sun, 11 Nov 2018 18:36:30 -0800
>>>> Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>
>>>>> We should have a checklist. That's a great idea. Now to find someone
>>>>> to write it... :)
>>>>
>>>> Do we think the LPC session might have the right people to create such a
>>>> thing? If so, I can try to put together a coherent presentation of the
>>>> result.
>>>
>>> Hi,
>>> Did anything ever happen with this syscall checklist suggestion?
>>
>> No, we really didn't have the right people around to do that,
>> unfortunately.
>
> We already have Documentation/process/adding-syscalls.rst.
>
> The documentation there is quite thorough.
>
> It lists things that people commonly forget e.g. email linux-api@vger.kernel.org.
>
> Would it be acceptable to attempt to collate per-libc information
> into the adding-syscalls.rst under a new section called:
>
> "Integration with libc"
>
I think that updates to adding-syscalls.rst would be sufficient,
instead of having a new/separate syscalls-checklist file.
thanks,
--
~Randy
^ permalink raw reply
* [PATCH v5 5/5] cgroup: Document bypass mode
From: Waiman Long @ 2018-12-10 21:40 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Peter Zijlstra, Ingo Molnar, Jonathan Corbet,
Johannes Weiner
Cc: cgroups, linux-kernel, linux-doc, linux-api, Roman Gushchin,
Jens Axboe, Andrew Morton, Dennis Zhou, Shakeel Butt,
Michael Kerrisk, Waiman Long
In-Reply-To: <1544478058-24252-1-git-send-email-longman@redhat.com>
The cgroup-v2.rst file is updated to document the new bypass mode.
Signed-off-by: Waiman Long <longman@redhat.com>
---
Documentation/admin-guide/cgroup-v2.rst | 66 ++++++++++++++++++++++++---------
1 file changed, 48 insertions(+), 18 deletions(-)
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 07e0613..17e6584 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -372,9 +372,16 @@ disabled by writing to the "cgroup.subtree_control" file::
# echo "+cpu +memory -io" > cgroup.subtree_control
-Only controllers which are listed in "cgroup.controllers" can be
-enabled. When multiple operations are specified as above, either they
-all succeed or fail. If multiple operations on the same controller
+The prefixes '+', '-' and '#' are used to enable, disable or put
+a controller in the bypass mode respectively. In the bypass mode,
+a controller is disabled in a cgroup, but it can be enabled again in
+its child cgroups as it will still be listed in "cgroup.controllers".
+Bypass mode can only be used on bypassable controllers. Currently,
+only the cpu controller is bypassable.
+
+Only controllers which are listed in "cgroup.controllers" can be enabled
+or bypassed. When multiple operations are specified as above, either
+they all succeed or fail. If multiple operations on the same controller
are specified, the last one is effective.
Enabling a controller in a cgroup indicates that the distribution of
@@ -399,6 +406,20 @@ prefixed controller interface files from C and D. This means that the
controller interface files - anything which doesn't start with
"cgroup." are owned by the parent rather than the cgroup itself.
+Once a bypassable controller is put into bypass mode in
+"cgroup.subtree_control", that controller can optionally be enabled
+again in child cgroups by writing the controller name with the '+ prefix
+into "cgroup.controllers". Writing the controller name with the '#'
+prefix into "cgroup.controllers" resets the state back to bypass mode.
+The state of a bypassable controller cannot be changed anymore if it
+is enabled or bypassed in its "cgroup.subtree_control".
+
+The use of bypass mode thus allows a cgroup parent to have the ability
+to selectively enable a bypassable controller in a subset of its child
+cgroups instead of in either all or none of them. In other words, a
+bypassable can be enabled only on the cgroups that actually need it,
+if desired.
+
Top-down Constraint
~~~~~~~~~~~~~~~~~~~
@@ -406,10 +427,11 @@ Top-down Constraint
Resources are distributed top-down and a cgroup can further distribute
a resource only if the resource has been distributed to it from the
parent. This means that all non-root "cgroup.subtree_control" files
-can only contain controllers which are enabled in the parent's
-"cgroup.subtree_control" file. A controller can be enabled only if
-the parent has the controller enabled and a controller can't be
-disabled if one or more children have it enabled.
+can only contain controllers which are enabled or bypassed in the
+parent's "cgroup.subtree_control" file. A controller can be enabled
+or bypassed only if the parent has the controller enabled or bypassed
+and the state of a controller can't be changed if one or more children
+have it enabled or bypassed.
No Internal Process Constraint
@@ -834,11 +856,18 @@ All cgroup core files are prefixed with "cgroup."
should be granted along with the containing directory.
cgroup.controllers
- A read-only space separated values file which exists on all
+ A read-write space separated values file which exists on all
cgroups.
It shows space separated list of all controllers available to
- the cgroup. The controllers are not ordered.
+ the cgroup. Controller names with '#' prefix are in bypass mode.
+ The controllers are not ordered.
+
+ When a controller is set into bypass mode in its parent's
+ "cgroup.subtree_control", its name prefixed with '+' or '#'
+ can be written to enable it or reset it back to bypass mode
+ respectively. Controllers not in bypass mode are not allowed
+ to be written.
cgroup.subtree_control
A read-write space separated values file which exists on all
@@ -848,12 +877,12 @@ All cgroup core files are prefixed with "cgroup."
which are enabled to control resource distribution from the
cgroup to its children.
- Space separated list of controllers prefixed with '+' or '-'
- can be written to enable or disable controllers. A controller
- name prefixed with '+' enables the controller and '-'
- disables. If a controller appears more than once on the list,
- the last one is effective. When multiple enable and disable
- operations are specified, either all succeed or all fail.
+ Space separated list of controllers prefixed with '+', '-' or
+ '#' can be written to enable, disable or bypass controllers
+ respectively. If a controller appears more than once on
+ the list, the last one is effective. When multiple enable,
+ disable or bypass operations are specified, either all succeed
+ or all fail.
cgroup.events
A read-only flat-keyed file which exists on non-root cgroups.
@@ -904,17 +933,18 @@ Controllers
CPU
---
-The "cpu" controllers regulates distribution of CPU cycles. This
+The "cpu" controller regulates distribution of CPU cycles. This
controller implements weight and absolute bandwidth limit models for
normal scheduling policy and absolute bandwidth allocation model for
-realtime scheduling policy.
+realtime scheduling policy. The cpu controller is bypassable.
WARNING: cgroup2 doesn't yet support control of realtime processes and
the cpu controller can only be enabled when all RT processes are in
the root cgroup. Be aware that system management software may already
have placed RT processes into nonroot cgroups during the system boot
process, and these processes may need to be moved to the root cgroup
-before the cpu controller can be enabled.
+before the cpu controller can be enabled unless bypass mode is used
+in those non-root cgroups.
CPU Interface Files
--
1.8.3.1
^ permalink raw reply related
* [PATCH v5 4/5] sched/core: Make cpu cgroup controller bypassable
From: Waiman Long @ 2018-12-10 21:40 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Peter Zijlstra, Ingo Molnar, Jonathan Corbet,
Johannes Weiner
Cc: cgroups, linux-kernel, linux-doc, linux-api, Roman Gushchin,
Jens Axboe, Andrew Morton, Dennis Zhou, Shakeel Butt,
Michael Kerrisk, Waiman Long
In-Reply-To: <1544478058-24252-1-git-send-email-longman@redhat.com>
Make the cpu cgroup controller bypassable in the default hierarchy so
that cpu controller could be activated only in those cgroups that really
need it instead of all the cgroups down to the lowest level that need it.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/sched/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f12225f..ba4550c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7000,6 +7000,7 @@ struct cgroup_subsys cpu_cgrp_subsys = {
.dfl_cftypes = cpu_files,
.early_init = true,
.threaded = true,
+ .bypassable = true,
};
#endif /* CONFIG_CGROUP_SCHED */
--
1.8.3.1
^ permalink raw reply related
* [PATCH v5 3/5] cgroup: Make debug controller report new controller masks
From: Waiman Long @ 2018-12-10 21:40 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Peter Zijlstra, Ingo Molnar, Jonathan Corbet,
Johannes Weiner
Cc: cgroups, linux-kernel, linux-doc, linux-api, Roman Gushchin,
Jens Axboe, Andrew Morton, Dennis Zhou, Shakeel Butt,
Michael Kerrisk, Waiman Long
In-Reply-To: <1544478058-24252-1-git-send-email-longman@redhat.com>
The newly added cgroup controller masks (subtree_bypass and
enable_ss_mask) are now being reported in the debug.masks controller
file.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/cgroup/debug.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/cgroup/debug.c b/kernel/cgroup/debug.c
index 5f1b873..176bb9e 100644
--- a/kernel/cgroup/debug.c
+++ b/kernel/cgroup/debug.c
@@ -263,6 +263,8 @@ static int cgroup_masks_read(struct seq_file *seq, void *v)
cgroup_masks_read_one(seq, "subtree_control", cgrp->subtree_control);
cgroup_masks_read_one(seq, "subtree_ss_mask", cgrp->subtree_ss_mask);
+ cgroup_masks_read_one(seq, "subtree_bypass", cgrp->subtree_bypass);
+ cgroup_masks_read_one(seq, "enable_ss_mask", cgrp->enable_ss_mask);
cgroup_kn_unlock(of->kn);
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v5 2/5] cgroup: Allow reenabling of controller in bypass mode
From: Waiman Long @ 2018-12-10 21:40 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Peter Zijlstra, Ingo Molnar, Jonathan Corbet,
Johannes Weiner
Cc: cgroups, linux-kernel, linux-doc, linux-api, Roman Gushchin,
Jens Axboe, Andrew Morton, Dennis Zhou, Shakeel Butt,
Michael Kerrisk, Waiman Long
In-Reply-To: <1544478058-24252-1-git-send-email-longman@redhat.com>
Bypassable controllers set to bypass mode in the parent's
"cgroup.subtree_control" can now be optionally enabled by writing the
controller name with the '+' prefix to "cgroup.controllers". Using the
'#' prefix will reset it back to the bypass state.
This capability allows a cgroup parent to individually enable bypassable
controllers in a subset of its children instead of either all or none
of them. This increases the flexibility each controller has in shaping
the effective cgroup hierarchy to best suit its need.
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/linux/cgroup-defs.h | 7 +++
kernel/cgroup/cgroup.c | 109 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 113 insertions(+), 3 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 5bff798..ab1b355 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -388,6 +388,13 @@ struct cgroup {
u16 old_subtree_ss_mask;
u16 old_subtree_bypass;
+ /*
+ * The bitmask of subsystems that are set in its parent's
+ * ->subtree_bypass and explicitly enabled in this cgroup.
+ */
+ u16 enable_ss_mask;
+ u16 old_enable_ss_mask;
+
/* Private pointers for each registered subsystem */
struct cgroup_subsys_state __rcu *subsys[CGROUP_SUBSYS_COUNT];
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 8a729e4..7aa77b1 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -424,7 +424,7 @@ static u16 cgroup_control(struct cgroup *cgrp, bool show_bypass)
u16 root_ss_mask = cgrp->root->subsys_mask;
if (parent) {
- u16 ss_mask = parent->subtree_control;
+ u16 ss_mask = parent->subtree_control|cgrp->enable_ss_mask;
if (show_bypass)
ss_mask |= parent->subtree_bypass;
@@ -447,7 +447,7 @@ static u16 cgroup_ss_mask(struct cgroup *cgrp, bool show_bypass)
struct cgroup *parent = cgroup_parent(cgrp);
if (parent) {
- u16 ss_mask = parent->subtree_ss_mask;
+ u16 ss_mask = parent->subtree_ss_mask|cgrp->enable_ss_mask;
if (show_bypass)
@@ -2874,6 +2874,7 @@ static void cgroup_save_control(struct cgroup *cgrp)
dsct->old_subtree_control = dsct->subtree_control;
dsct->old_subtree_ss_mask = dsct->subtree_ss_mask;
dsct->old_subtree_bypass = dsct->subtree_bypass;
+ dsct->old_enable_ss_mask = dsct->enable_ss_mask;
dsct->old_dom_cgrp = dsct->dom_cgrp;
}
}
@@ -2919,6 +2920,7 @@ static void cgroup_restore_control(struct cgroup *cgrp)
dsct->subtree_control = dsct->old_subtree_control;
dsct->subtree_ss_mask = dsct->old_subtree_ss_mask;
dsct->subtree_bypass = dsct->old_subtree_bypass;
+ dsct->enable_ss_mask = dsct->old_enable_ss_mask;
dsct->dom_cgrp = dsct->old_dom_cgrp;
}
}
@@ -3197,7 +3199,8 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
}
cgroup_for_each_live_child(child, cgrp)
- child_enable |= child->subtree_control|child->subtree_bypass;
+ child_enable |= child->subtree_control|child->subtree_bypass|
+ child->enable_ss_mask;
/*
* Cannot change the state of a controller if enabled in children.
@@ -3230,6 +3233,105 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
return ret ?: nbytes;
}
+/*
+ * Change bypass status of controllers for a cgroup in the default hierarchy.
+ */
+static ssize_t cgroup_controllers_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes,
+ loff_t off)
+{
+ u16 enable = 0, bypass = 0;
+ struct cgroup *cgrp, *parent;
+ struct cgroup_subsys *ss;
+ char *tok;
+ int ssid, ret;
+
+ /*
+ * Parse input - space separated list of subsystem names prefixed
+ * with either + or #.
+ */
+ buf = strstrip(buf);
+ while ((tok = strsep(&buf, " "))) {
+ if (tok[0] == '\0')
+ continue;
+ do_each_subsys_mask(ss, ssid, ~cgrp_dfl_inhibit_ss_mask) {
+ if (!cgroup_ssid_enabled(ssid) ||
+ strcmp(tok + 1, ss->name))
+ continue;
+
+ if (*tok == '+') {
+ enable |= 1 << ssid;
+ bypass &= ~(1 << ssid);
+ } else if (*tok == '#') {
+ bypass |= 1 << ssid;
+ enable &= ~(1 << ssid);
+ } else {
+ return -EINVAL;
+ }
+ break;
+ } while_each_subsys_mask();
+ if (ssid == CGROUP_SUBSYS_COUNT)
+ return -EINVAL;
+ }
+
+ cgrp = cgroup_kn_lock_live(of->kn, true);
+ if (!cgrp)
+ return -ENODEV;
+
+ /*
+ * Write to root cgroup's controllers file is not allowed.
+ */
+ parent = cgroup_parent(cgrp);
+ if (!parent) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ /*
+ * Only controllers set into bypass mode in the parent cgroup
+ * can be specified here.
+ */
+ if (~parent->subtree_bypass & (enable|bypass)) {
+ ret = -ENOENT;
+ goto out_unlock;
+ }
+
+ /*
+ * Mask off irrelevant bits.
+ */
+ enable &= ~cgrp->enable_ss_mask;
+ bypass &= cgrp->enable_ss_mask;
+
+ if (!(enable|bypass)) {
+ ret = 0;
+ goto out_unlock;
+ }
+
+ /*
+ * We cannot change the bypass state of a controller that is enabled
+ * in subtree_control.
+ */
+ if ((cgrp->subtree_control|cgrp->subtree_bypass) & (enable|bypass)) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+ /* Save and update control masks and prepare csses */
+ cgroup_save_control(cgrp);
+
+ cgrp->enable_ss_mask |= enable;
+ cgrp->enable_ss_mask &= ~bypass;
+
+ ret = cgroup_apply_control(cgrp);
+ cgroup_finalize_control(cgrp, ret);
+ kernfs_activate(cgrp->kn);
+ ret = 0;
+
+out_unlock:
+ cgroup_kn_unlock(of->kn);
+ return ret ?: nbytes;
+}
+
/**
* cgroup_enable_threaded - make @cgrp threaded
* @cgrp: the target cgroup
@@ -4573,6 +4675,7 @@ static ssize_t cgroup_threads_write(struct kernfs_open_file *of,
{
.name = "cgroup.controllers",
.seq_show = cgroup_controllers_show,
+ .write = cgroup_controllers_write,
},
{
.name = "cgroup.subtree_control",
--
1.8.3.1
^ permalink raw reply related
* [PATCH v5 1/5] cgroup: subtree_control bypass mode for bypassable controllers
From: Waiman Long @ 2018-12-10 21:40 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Peter Zijlstra, Ingo Molnar, Jonathan Corbet,
Johannes Weiner
Cc: cgroups, linux-kernel, linux-doc, linux-api, Roman Gushchin,
Jens Axboe, Andrew Morton, Dennis Zhou, Shakeel Butt,
Michael Kerrisk, Waiman Long
In-Reply-To: <1544478058-24252-1-git-send-email-longman@redhat.com>
A controller in the default hierarchy is bypassable if the bypassable
flag is set in its cgroup_subsys structure. The special prefix '#'
attached to a bypassable controller name can now be written into the
cgroup.subtree_control file to set that controller in bypass mode in
all the child cgroups. The controller will show up in the children's
cgroup.controllers file, but the corresponding control knobs will be
absent. However, that controller can be enabled or bypassed in its
children by writing to their respective subtree_control files.
In term of resource control, a bypassed cgroup was lumped into its
nearest non-bypassed ancestor. So all the tasks in that ancestor as
well as those in its bypassed direct descendants are controlled by the
same set of control knobs.
This mode is useful to those bypassable controllers where there are
costs to each additional layer of hierarchy. This mode will also allow
more freedom in how each controller can shape its effective hierarchy
independent of the others.
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/linux/cgroup-defs.h | 19 ++++--
kernel/cgroup/cgroup.c | 151 ++++++++++++++++++++++++++++++--------------
2 files changed, 116 insertions(+), 54 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 8fcbae1..5bff798 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -375,16 +375,18 @@ struct cgroup {
struct cgroup_file events_file; /* handle for "cgroup.events" */
/*
- * The bitmask of subsystems enabled on the child cgroups.
- * ->subtree_control is the one configured through
- * "cgroup.subtree_control" while ->child_ss_mask is the effective
- * one which may have more subsystems enabled. Controller knobs
- * are made available iff it's enabled in ->subtree_control.
+ * The bitmask of subsystems enabled or bypassed on the child cgroups.
+ * ->subtree_control and ->subtree_bypass are the one configured
+ * through "cgroup.subtree_control" while ->subtree_ss_mask is the
+ * effective one which may have more subsystems enabled. Controller
+ * knobs are made available iff it's enabled in ->subtree_ss_mask.
*/
u16 subtree_control;
u16 subtree_ss_mask;
+ u16 subtree_bypass;
u16 old_subtree_control;
u16 old_subtree_ss_mask;
+ u16 old_subtree_bypass;
/* Private pointers for each registered subsystem */
struct cgroup_subsys_state __rcu *subsys[CGROUP_SUBSYS_COUNT];
@@ -647,6 +649,13 @@ struct cgroup_subsys {
bool broken_hierarchy:1;
bool warned_broken_hierarchy:1;
+ /*
+ * If %true, the controller, on the default hierarchy, can be
+ * bypassable. IOW, it can have a virtual hierarchy that is
+ * different from the default hierarchy.
+ */
+ bool bypassable:1;
+
/* the following two fields are initialized automtically during boot */
int id;
const char *name;
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index e06994f..8a729e4 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -175,6 +175,9 @@ struct cgroup_subsys *cgroup_subsys[] = {
/* some controllers can be threaded on the default hierarchy */
static u16 cgrp_dfl_threaded_ss_mask;
+/* some controllers can be bypassable on the default hierarchy */
+static u16 cgrp_dfl_bypass_ss_mask;
+
/* The list of hierarchy roots */
LIST_HEAD(cgroup_roots);
static int cgroup_root_count;
@@ -366,7 +369,8 @@ static bool cgroup_can_be_thread_root(struct cgroup *cgrp)
return false;
/* and no domain controllers can be enabled */
- if (cgrp->subtree_control & ~cgrp_dfl_threaded_ss_mask)
+ if ((cgrp->subtree_control|cgrp->subtree_bypass) &
+ ~cgrp_dfl_threaded_ss_mask)
return false;
return true;
@@ -388,7 +392,8 @@ bool cgroup_is_thread_root(struct cgroup *cgrp)
* enabled is a thread root.
*/
if (cgroup_has_tasks(cgrp) &&
- (cgrp->subtree_control & cgrp_dfl_threaded_ss_mask))
+ ((cgrp->subtree_control|cgrp->subtree_bypass)
+ & cgrp_dfl_threaded_ss_mask))
return true;
return false;
@@ -413,7 +418,7 @@ static bool cgroup_is_valid_domain(struct cgroup *cgrp)
}
/* subsystems visibly enabled on a cgroup */
-static u16 cgroup_control(struct cgroup *cgrp)
+static u16 cgroup_control(struct cgroup *cgrp, bool show_bypass)
{
struct cgroup *parent = cgroup_parent(cgrp);
u16 root_ss_mask = cgrp->root->subsys_mask;
@@ -421,6 +426,9 @@ static u16 cgroup_control(struct cgroup *cgrp)
if (parent) {
u16 ss_mask = parent->subtree_control;
+ if (show_bypass)
+ ss_mask |= parent->subtree_bypass;
+
/* threaded cgroups can only have threaded controllers */
if (cgroup_is_threaded(cgrp))
ss_mask &= cgrp_dfl_threaded_ss_mask;
@@ -434,13 +442,17 @@ static u16 cgroup_control(struct cgroup *cgrp)
}
/* subsystems enabled on a cgroup */
-static u16 cgroup_ss_mask(struct cgroup *cgrp)
+static u16 cgroup_ss_mask(struct cgroup *cgrp, bool show_bypass)
{
struct cgroup *parent = cgroup_parent(cgrp);
if (parent) {
u16 ss_mask = parent->subtree_ss_mask;
+
+ if (show_bypass)
+ ss_mask |= parent->subtree_bypass;
+
/* threaded cgroups can only have threaded controllers */
if (cgroup_is_threaded(cgrp))
ss_mask &= cgrp_dfl_threaded_ss_mask;
@@ -515,7 +527,7 @@ static struct cgroup_subsys_state *cgroup_e_css(struct cgroup *cgrp,
* This function is used while updating css associations and thus
* can't test the csses directly. Test ss_mask.
*/
- while (!(cgroup_ss_mask(cgrp) & (1 << ss->id))) {
+ while (!(cgroup_ss_mask(cgrp, false) & (1 << ss->id))) {
cgrp = cgroup_parent(cgrp);
if (!cgrp)
return NULL;
@@ -2414,7 +2426,7 @@ int cgroup_migrate_vet_dst(struct cgroup *dst_cgrp)
return 0;
/* apply no-internal-process constraint */
- if (dst_cgrp->subtree_control)
+ if (dst_cgrp->subtree_control|dst_cgrp->subtree_bypass)
return -EBUSY;
return 0;
@@ -2712,15 +2724,18 @@ void cgroup_procs_write_finish(struct task_struct *task)
ss->post_attach();
}
-static void cgroup_print_ss_mask(struct seq_file *seq, u16 ss_mask)
+static void cgroup_print_ss_mask(struct seq_file *seq, u16 ss_mask,
+ u16 bypass_mask)
{
struct cgroup_subsys *ss;
bool printed = false;
int ssid;
- do_each_subsys_mask(ss, ssid, ss_mask) {
+ do_each_subsys_mask(ss, ssid, ss_mask|bypass_mask) {
if (printed)
seq_putc(seq, ' ');
+ if (!(ss_mask & (1 << ssid)))
+ seq_putc(seq, '#');
seq_printf(seq, "%s", ss->name);
printed = true;
} while_each_subsys_mask();
@@ -2732,8 +2747,10 @@ static void cgroup_print_ss_mask(struct seq_file *seq, u16 ss_mask)
static int cgroup_controllers_show(struct seq_file *seq, void *v)
{
struct cgroup *cgrp = seq_css(seq)->cgroup;
+ struct cgroup *parent = cgroup_parent(cgrp);
+ u16 bypass = parent ? parent->subtree_bypass : 0;
- cgroup_print_ss_mask(seq, cgroup_control(cgrp));
+ cgroup_print_ss_mask(seq, cgroup_control(cgrp, false), bypass);
return 0;
}
@@ -2742,7 +2759,7 @@ static int cgroup_subtree_control_show(struct seq_file *seq, void *v)
{
struct cgroup *cgrp = seq_css(seq)->cgroup;
- cgroup_print_ss_mask(seq, cgrp->subtree_control);
+ cgroup_print_ss_mask(seq, cgrp->subtree_control, cgrp->subtree_bypass);
return 0;
}
@@ -2856,6 +2873,7 @@ static void cgroup_save_control(struct cgroup *cgrp)
cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) {
dsct->old_subtree_control = dsct->subtree_control;
dsct->old_subtree_ss_mask = dsct->subtree_ss_mask;
+ dsct->old_subtree_bypass = dsct->subtree_bypass;
dsct->old_dom_cgrp = dsct->dom_cgrp;
}
}
@@ -2874,10 +2892,13 @@ static void cgroup_propagate_control(struct cgroup *cgrp)
struct cgroup_subsys_state *d_css;
cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) {
- dsct->subtree_control &= cgroup_control(dsct);
+ u16 mask = cgroup_control(dsct, true);
+
+ dsct->subtree_control &= mask;
+ dsct->subtree_bypass &= mask;
dsct->subtree_ss_mask =
cgroup_calc_subtree_ss_mask(dsct->subtree_control,
- cgroup_ss_mask(dsct));
+ cgroup_ss_mask(dsct, true));
}
}
@@ -2897,6 +2918,7 @@ static void cgroup_restore_control(struct cgroup *cgrp)
cgroup_for_each_live_descendant_post(dsct, d_css, cgrp) {
dsct->subtree_control = dsct->old_subtree_control;
dsct->subtree_ss_mask = dsct->old_subtree_ss_mask;
+ dsct->subtree_bypass = dsct->old_subtree_bypass;
dsct->dom_cgrp = dsct->old_dom_cgrp;
}
}
@@ -2906,9 +2928,9 @@ static bool css_visible(struct cgroup_subsys_state *css)
struct cgroup_subsys *ss = css->ss;
struct cgroup *cgrp = css->cgroup;
- if (cgroup_control(cgrp) & (1 << ss->id))
+ if (cgroup_control(cgrp, false) & (1 << ss->id))
return true;
- if (!(cgroup_ss_mask(cgrp) & (1 << ss->id)))
+ if (!(cgroup_ss_mask(cgrp, false) & (1 << ss->id)))
return false;
return cgroup_on_dfl(cgrp) && ss->implicit_on_dfl;
}
@@ -2939,7 +2961,7 @@ static int cgroup_apply_control_enable(struct cgroup *cgrp)
WARN_ON_ONCE(css && percpu_ref_is_dying(&css->refcnt));
- if (!(cgroup_ss_mask(dsct) & (1 << ss->id)))
+ if (!(cgroup_ss_mask(dsct, false) & (1 << ss->id)))
continue;
if (!css) {
@@ -2989,7 +3011,7 @@ static void cgroup_apply_control_disable(struct cgroup *cgrp)
continue;
if (css->parent &&
- !(cgroup_ss_mask(dsct) & (1 << ss->id))) {
+ !(cgroup_ss_mask(dsct, false) & (1 << ss->id))) {
kill_css(css);
} else if (!css_visible(css)) {
css_clear_dir(css);
@@ -3101,7 +3123,8 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
char *buf, size_t nbytes,
loff_t off)
{
- u16 enable = 0, disable = 0;
+ u16 enable = 0, disable = 0, bypass = 0;
+ u16 child_enable = 0;
struct cgroup *cgrp, *child;
struct cgroup_subsys *ss;
char *tok;
@@ -3122,10 +3145,16 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
if (*tok == '+') {
enable |= 1 << ssid;
+ bypass &= ~(1 << ssid);
disable &= ~(1 << ssid);
} else if (*tok == '-') {
disable |= 1 << ssid;
enable &= ~(1 << ssid);
+ bypass &= ~(1 << ssid);
+ } else if (*tok == '#') {
+ bypass |= 1 << ssid;
+ enable &= ~(1 << ssid);
+ disable &= ~(1 << ssid);
} else {
return -EINVAL;
}
@@ -3139,35 +3168,42 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
if (!cgrp)
return -ENODEV;
- for_each_subsys(ss, ssid) {
- if (enable & (1 << ssid)) {
- if (cgrp->subtree_control & (1 << ssid)) {
- enable &= ~(1 << ssid);
- continue;
- }
+ /*
+ * Cannot use controllers that aren't allowed.
+ */
+ if (~cgroup_control(cgrp, true) & (enable|disable|bypass)) {
+ ret = -ENOENT;
+ goto out_unlock;
+ }
- if (!(cgroup_control(cgrp) & (1 << ssid))) {
- ret = -ENOENT;
- goto out_unlock;
- }
- } else if (disable & (1 << ssid)) {
- if (!(cgrp->subtree_control & (1 << ssid))) {
- disable &= ~(1 << ssid);
- continue;
- }
+ /*
+ * Strip out redundant bits.
+ */
+ enable &= ~cgrp->subtree_control;
+ bypass &= ~cgrp->subtree_bypass;
+ disable &= (cgrp->subtree_control|cgrp->subtree_bypass);
- /* a child has it enabled? */
- cgroup_for_each_live_child(child, cgrp) {
- if (child->subtree_control & (1 << ssid)) {
- ret = -EBUSY;
- goto out_unlock;
- }
- }
- }
+ if (!(enable|bypass|disable)) {
+ ret = 0;
+ goto out_unlock;
}
- if (!enable && !disable) {
- ret = 0;
+ /*
+ * Only bypassable controllers can be bypassed.
+ */
+ if (bypass & ~cgrp_dfl_bypass_ss_mask) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ cgroup_for_each_live_child(child, cgrp)
+ child_enable |= child->subtree_control|child->subtree_bypass;
+
+ /*
+ * Cannot change the state of a controller if enabled in children.
+ */
+ if ((enable|bypass|disable) & child_enable) {
+ ret = -EBUSY;
goto out_unlock;
}
@@ -3179,7 +3215,9 @@ static ssize_t cgroup_subtree_control_write(struct kernfs_open_file *of,
cgroup_save_control(cgrp);
cgrp->subtree_control |= enable;
- cgrp->subtree_control &= ~disable;
+ cgrp->subtree_control &= ~(bypass|disable);
+ cgrp->subtree_bypass |= bypass;
+ cgrp->subtree_bypass &= ~(enable|disable);
ret = cgroup_apply_control(cgrp);
cgroup_finalize_control(cgrp, ret);
@@ -4727,7 +4765,8 @@ static void css_release(struct percpu_ref *ref)
}
static void init_and_link_css(struct cgroup_subsys_state *css,
- struct cgroup_subsys *ss, struct cgroup *cgrp)
+ struct cgroup_subsys *ss, struct cgroup *cgrp,
+ struct cgroup_subsys_state *parent_css)
{
lockdep_assert_held(&cgroup_mutex);
@@ -4743,8 +4782,8 @@ static void init_and_link_css(struct cgroup_subsys_state *css,
css->serial_nr = css_serial_nr_next++;
atomic_set(&css->online_cnt, 0);
- if (cgroup_parent(cgrp)) {
- css->parent = cgroup_css(cgroup_parent(cgrp), ss);
+ if (parent_css) {
+ css->parent = parent_css;
css_get(css->parent);
}
@@ -4813,13 +4852,24 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
lockdep_assert_held(&cgroup_mutex);
+ /*
+ * If the cgroup subsystem is bypassble, we need to skip over
+ * ancestor cgroups with NULL CSS to find the right parent CSS.
+ */
+ if (ss->bypassable && !parent_css) {
+ struct cgroup *par = parent;
+
+ for (; par && !parent_css; par = cgroup_parent(par))
+ parent_css = cgroup_css(par, ss);
+ }
+
css = ss->css_alloc(parent_css);
if (!css)
css = ERR_PTR(-ENOMEM);
if (IS_ERR(css))
return css;
- init_and_link_css(css, ss, cgrp);
+ init_and_link_css(css, ss, cgrp, parent_css);
err = percpu_ref_init(&css->refcnt, css_release, 0, GFP_KERNEL);
if (err)
@@ -4941,7 +4991,7 @@ static struct cgroup *cgroup_create(struct cgroup *parent)
* subtree_control from the parent. Each is configured manually.
*/
if (!cgroup_on_dfl(cgrp))
- cgrp->subtree_control = cgroup_control(cgrp);
+ cgrp->subtree_control = cgroup_control(cgrp, false);
cgroup_propagate_control(cgrp);
@@ -5254,7 +5304,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)
css = ss->css_alloc(cgroup_css(&cgrp_dfl_root.cgrp, ss));
/* We don't handle early failures gracefully */
BUG_ON(IS_ERR(css));
- init_and_link_css(css, ss, &cgrp_dfl_root.cgrp);
+ init_and_link_css(css, ss, &cgrp_dfl_root.cgrp, NULL);
/*
* Root csses are never destroyed and we can't initialize
@@ -5412,6 +5462,9 @@ int __init cgroup_init(void)
if (ss->threaded)
cgrp_dfl_threaded_ss_mask |= 1 << ss->id;
+ if (ss->bypassable)
+ cgrp_dfl_bypass_ss_mask |= 1 << ss->id;
+
if (ss->dfl_cftypes == ss->legacy_cftypes) {
WARN_ON(cgroup_add_cftypes(ss, ss->dfl_cftypes));
} else {
--
1.8.3.1
^ permalink raw reply related
* [PATCH v5 0/5] cgroup: Introducing bypass mode
From: Waiman Long @ 2018-12-10 21:40 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Peter Zijlstra, Ingo Molnar, Jonathan Corbet,
Johannes Weiner
Cc: cgroups, linux-kernel, linux-doc, linux-api, Roman Gushchin,
Jens Axboe, Andrew Morton, Dennis Zhou, Shakeel Butt,
Michael Kerrisk, Waiman Long
v5:
- Fix a bug in css_create() reported by Dan Carpenter.
v4:
- Rebased to the latest for-4.21 branch of cgroup tree.
- Make each controller explicitly opt in to become bypassable by
setting the bypassable cgroup_subsys flag. Currently, only cpu
controller is made bypassable.
- Break out the cgroup_v2.rst documentation update as separate patch.
v3:
- Remove invalid cgroup subdirectory creation patch.
- Add use cases for the bypass mode and removing statements about
control files ownership in cgroup-v2.txt.
- Restrict bypass mode to non-domain (threaded) controllers only.
v3 patch - https://lkml.org/lkml/2017/8/9/604
v4 patch - https://lkml.org/lkml/2018/11/20/1361
This patchset introduces a new bypass mode to the cgroup v2 core to
give more freedom and flexibility to controllers which choose to become
bypassable the freedom to shape their own unique views of the virtual
cgroup hierarchies that can best suit thier own use cases.
Because of the inherent performance overhead in enabling cpu controller,
it is made bypassable so that the controller only needs to be enabled
at those cgroups that really need it instead of in every cgroups at a
given layer if at least one of them needs it.
The cpu controller performance problem is one of the major issues
in migrating from cgroup v1 to v2.
For example,
R - A(+) - B(#) - C(+)
\ D(#)
where "+" means the controller is enabled and "#" means the controller
is bypassed. For this controller's perspective, the cgroups are
equivalent to:
R - A|B|D - C
Underneath the root R, cgoups A, B and D are controlled by one set of
knobs and cgroup C is controlled by another set of knobs as a child of
cgroups A|B|C.
This patchset is layered on top of the "for-4.21" branch of Tejun's
cgroup git tree.
Patch 1 introduces a new bypass mode that allows a bypassable
controller to be disabled in a cgroup, but to be re-enabled again in its
children. This is enabled by writing the controller name prefixed with
'#' to the "cgroup.subtree_control" file. Then all its children will
have this controller in bypass mode.
Patch 2 extends the bypass mode mechanism to allow those child cgroups
that are put into the bypass mode for a particular bypassable controller
by their parent to be re-enabled again by writing the controller name
with the '+' prefix to the "cgroup.controllers" file.
Patch 3 extends the debug controller to expose additional controller
masks introduced by this patchset.
Patch 4 makes the cpu controller bypassable.
Patch 5 documents the new bypass mode in cgroup-v2.rst file.
Waiman Long (5):
cgroup: subtree_control bypass mode for bypassable controllers
cgroup: Allow reenabling of controller in bypass mode
cgroup: Make debug controller report new controller masks
sched/core: Make cpu cgroup controller bypassable
cgroup: Document bypass mode
Documentation/admin-guide/cgroup-v2.rst | 66 +++++---
include/linux/cgroup-defs.h | 26 +++-
kernel/cgroup/cgroup.c | 258 +++++++++++++++++++++++++-------
kernel/cgroup/debug.c | 2 +
kernel/sched/core.c | 1 +
5 files changed, 279 insertions(+), 74 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Palmer Dabbelt @ 2018-12-10 21:04 UTC (permalink / raw)
To: paul.burton
Cc: ldv, ralf, jhogan, oleg, luto, lineprinter, esyr, keescook, jannh,
linux-api, strace-devel, linux-kernel
In-Reply-To: <20181210180421.wq3ruldbpvu2jpfm@pburton-laptop>
On Mon, 10 Dec 2018 10:04:22 PST (-0800), paul.burton@mips.com wrote:
> Hi Dmitry,
>
> On Mon, Dec 10, 2018 at 07:09:40PM +0300, Dmitry V. Levin wrote:
>> We decided to add .frame_pointer to struct ptrace_syscall_info just for
>> consistency with .instruction_pointer and .stack_pointer; I must have been
>> misled by comments in asm-generic/ptrace.h into thinking that
>> frame_pointer() is universally available across architectures.
>
> Is it correct to say that you're using frame_pointer() purely on user
> register state, not kernel?
>
> If so then one option would be to define it for MIPS as something like:
>
> static inline unsigned long frame_pointer(struct pt_regs *regs)
> {
> return regs->regs[30];
> }
>
> My concern with that though would be that providing frame_pointer()
> unconditionally might mislead people into thinking that the kernel
> always has frame pointers, when in reality current MIPS kernels never
> do. In fact a comment in MIPS' asm/ptrace.h seems to suggest the lack of
> frame_pointer() is intentional for exactly that reason:
>
>> Don't use asm-generic/ptrace.h it defines FP accessors that don't make
>> sense on MIPS. We rather want an error if they get invoked.
>
> Looking across architectures though MIPS isn't going to be the only one
> missing frame_pointer(). With a little grepping it appears that these
> architectures provide frame_pointer():
>
> arm
> arm64
> hexagon
> nds32
> powerpc
> riscv
> sparc
> um
> x86
>
> That leaves a whole bunch of other architectures (16) which don't have
> frame_pointer(), or at least not in a way that I could see at a glance.
We (RISC-V) default to compiling without frame pointers. I'm not sure if it
even makes sense have frame_pointer() on RISC-V, as it'll usually return
garbage.
>> Unlike .instruction_pointer and .stack_pointer that are actually needed
>> in strace, .frame_pointer is not used, so from strace PoV we don't really
>> need it.
>>
>> So the question is, does anybody need a
>> struct ptrace_syscall_info.frame_pointer?
>>
>> If yes, how can frame_pointer() be defined on MIPS?
>> Or should we just forget about making sense of frame_pointer() and remove
>> struct ptrace_syscall_info.frame_pointer from the proposed API?
>
> So, along these lines my suggestion would be to avoid it if you don't
> really need it anyway.
>
> Thanks,
> Paul
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Andy Lutomirski @ 2018-12-10 19:38 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: LKML, Kees Cook, Jann Horn, jhogan-DgEjT+Ai2ygdnm+yROfE0A,
Oleg Nesterov, Ralf Baechle, Eugene Syromiatnikov,
paul.burton-8NJIiSa5LzA, Andrew Lutomirski, Linux API,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181210160940.GF14149-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
> On Dec 10, 2018, at 8:09 AM, Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi, things are getting too complicated and we need some advice how to deal
> with this frame_pointer issue.
>
>> On Mon, Dec 10, 2018 at 10:26:50PM +0800, kbuild test robot wrote:
>> Hi Elvira,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on linus/master]
>> [also build test ERROR on v4.20-rc6]
>> [cannot apply to next-20181207]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
>> config: mips-malta_kvm_defconfig (attached as .config)
>> compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.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.2.0 make.cross ARCH=mips
>>
>> All errors (new ones prefixed by >>):
>>
>> kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>>>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
>> .frame_pointer = frame_pointer(regs)
>> ^~~~~~~~~~~~~
>> trace_printk
>> cc1: some warnings being treated as errors
>>
>> vim +942 kernel/ptrace.c
>>
>> 931
>> 932 static int
>> 933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
>> 934 void __user *datavp)
>> 935 {
>> 936 struct pt_regs *regs = task_pt_regs(child);
>> 937 struct ptrace_syscall_info info = {
>> 938 .op = PTRACE_SYSCALL_INFO_NONE,
>> 939 .arch = syscall_get_arch(child),
>> 940 .instruction_pointer = instruction_pointer(regs),
>> 941 .stack_pointer = user_stack_pointer(regs),
>>> 942 .frame_pointer = frame_pointer(regs)
>> 943 };
>> 944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
>> 945 unsigned long write_size;
>> 946
>> 947 /*
>> 948 * This does not need lock_task_sighand() to access
>> 949 * child->last_siginfo because ptrace_freeze_traced()
>> 950 * called earlier by ptrace_check_attach() ensures that
>> 951 * the tracee cannot go away and clear its last_siginfo.
>> 952 */
>> 953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
>> 954 case SIGTRAP | 0x80:
>> 955 switch (child->ptrace_message) {
>> 956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
>> 957 actual_size = ptrace_get_syscall_info_entry(child, regs,
>> 958 &info);
>> 959 break;
>> 960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
>> 961 actual_size = ptrace_get_syscall_info_exit(child, regs,
>> 962 &info);
>> 963 break;
>> 964 }
>> 965 break;
>> 966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
>> 967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
>> 968 &info);
>> 969 break;
>> 970 }
>> 971
>> 972 write_size = min(actual_size, user_size);
>> 973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
>> 974 }
>> 975
>
> We decided to add .frame_pointer to struct ptrace_syscall_info just for
> consistency with .instruction_pointer and .stack_pointer; I must have been
> misled by comments in asm-generic/ptrace.h into thinking that
> frame_pointer() is universally available across architectures.
>
> Unlike .instruction_pointer and .stack_pointer that are actually needed
> in strace, .frame_pointer is not used, so from strace PoV we don't really
> need it.
>
> So the question is, does anybody need a
> struct ptrace_syscall_info.frame_pointer?
>
> If yes, how can frame_pointer() be defined on MIPS?
> Or should we just forget about making sense of frame_pointer() and remove
> struct ptrace_syscall_info.frame_pointer from the proposed API?
>
I would suggest getting rid of frame_pointer. Anyone who needs that
degree of debugging can use existing ptrace APIs for it.
>
> --
> ldv
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Paul Burton @ 2018-12-10 18:04 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Ralf Baechle, James Hogan, Oleg Nesterov, Andy Lutomirski,
Elvira Khabirova, Eugene Syromyatnikov, Kees Cook, Jann Horn,
linux-api@vger.kernel.org, strace-devel@lists.strace.io,
linux-kernel@vger.kernel.org
In-Reply-To: <20181210160940.GF14149@altlinux.org>
Hi Dmitry,
On Mon, Dec 10, 2018 at 07:09:40PM +0300, Dmitry V. Levin wrote:
> We decided to add .frame_pointer to struct ptrace_syscall_info just for
> consistency with .instruction_pointer and .stack_pointer; I must have been
> misled by comments in asm-generic/ptrace.h into thinking that
> frame_pointer() is universally available across architectures.
Is it correct to say that you're using frame_pointer() purely on user
register state, not kernel?
If so then one option would be to define it for MIPS as something like:
static inline unsigned long frame_pointer(struct pt_regs *regs)
{
return regs->regs[30];
}
My concern with that though would be that providing frame_pointer()
unconditionally might mislead people into thinking that the kernel
always has frame pointers, when in reality current MIPS kernels never
do. In fact a comment in MIPS' asm/ptrace.h seems to suggest the lack of
frame_pointer() is intentional for exactly that reason:
> Don't use asm-generic/ptrace.h it defines FP accessors that don't make
> sense on MIPS. We rather want an error if they get invoked.
Looking across architectures though MIPS isn't going to be the only one
missing frame_pointer(). With a little grepping it appears that these
architectures provide frame_pointer():
arm
arm64
hexagon
nds32
powerpc
riscv
sparc
um
x86
That leaves a whole bunch of other architectures (16) which don't have
frame_pointer(), or at least not in a way that I could see at a glance.
> Unlike .instruction_pointer and .stack_pointer that are actually needed
> in strace, .frame_pointer is not used, so from strace PoV we don't really
> need it.
>
> So the question is, does anybody need a
> struct ptrace_syscall_info.frame_pointer?
>
> If yes, how can frame_pointer() be defined on MIPS?
> Or should we just forget about making sense of frame_pointer() and remove
> struct ptrace_syscall_info.frame_pointer from the proposed API?
So, along these lines my suggestion would be to avoid it if you don't
really need it anyway.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH v3 0/2] sysctl: handle overflow for file-max
From: Kees Cook @ 2018-12-10 17:51 UTC (permalink / raw)
To: Andrew Morton
Cc: Christian Brauner, Christian Brauner, LKML, Eric W. Biederman,
Luis R. Rodriguez, Joe Lawrence, Waiman Long, Dominik Brodowski,
Al Viro, Alexey Dobriyan, Linux API
In-Reply-To: <CAPP7u0U72jSkT3km-E2imQdOvk-WRaDCH4t--N07mh8my7PvOQ@mail.gmail.com>
Hi Andrew,
Can you take this patch for -mm?
-Kees
On Sun, Dec 9, 2018 at 8:41 AM Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> On Mon, Oct 29, 2018 at 10:44 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Oct 29, 2018 at 7:58 AM, Christian Brauner <christian@brauner.io> wrote:
> > > On Wed, Oct 17, 2018 at 12:33:20AM +0200, Christian Brauner wrote:
> > >> Hey,
> > >>
> > >> Here is v3 of this patchset. Changelogs are in the individual commits.
> > >>
> > >> Currently, when writing
> > >>
> > >> echo 18446744073709551616 > /proc/sys/fs/file-max
> > >>
> > >> /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> > >> crashes the system.
> > >>
> > >> The first version of this patch intended to detect the overflow and cap
> > >> at ULONG_MAX. However, we should not do this and rather return EINVAL on
> > >> overflow. The reasons are:
> > >> - this aligns with other sysctl handlers that simply reject overflows
> > >> (cf. [1], [2], and a bunch of others)
> > >> - we already do a partial fail on overflow right now
> > >> Namely, when the TMPBUFLEN is exceeded. So we already reject values
> > >> such as 184467440737095516160 (21 chars) but accept values such as
> > >> 18446744073709551616 (20 chars) but both are overflows. So we should
> > >> just always reject 64bit overflows and not special-case this based on
> > >> the number of chars.
> > >>
> > >> (This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)
> > >
> > > Just so that we don't forget, can we make sure that this gets picked
> > > into linux-next? :)
> >
> > I was hoping akpm would take this? Andrew, does the v3 look okay to you?
>
> gentle ping again :)
>
> Christian
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Kees Cook @ 2018-12-10 17:44 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Andy Lutomirski, Elvira Khabirova,
Eugene Syromiatnikov, Jann Horn, Linux API, strace-devel, LKML
In-Reply-To: <20181210043126.GX6131@altlinux.org>
On Sun, Dec 9, 2018 at 8:31 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> From: Elvira Khabirova <lineprinter@altlinux.org>
>
> PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
> details of the syscall the tracee is blocked in.
>
> There are two reasons for a special syscall-related ptrace request.
>
> Firstly, with the current ptrace API there are cases when ptracer cannot
> retrieve necessary information about syscalls. Some examples include:
> * The notorious int-0x80-from-64-bit-task issue. See [1] for details.
> In short, if a 64-bit task performs a syscall through int 0x80, its tracer
> has no reliable means to find out that the syscall was, in fact,
> a compat syscall, and misidentifies it.
> * Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
> Common practice is to keep track of the sequence of ptrace-stops in order
> not to mix the two syscall-stops up. But it is not as simple as it looks;
> for example, strace had a (just recently fixed) long-standing bug where
> attaching strace to a tracee that is performing the execve system call
> led to the tracer identifying the following syscall-exit-stop as
> syscall-enter-stop, which messed up all the state tracking.
> * Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
> ("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
> and process_vm_readv become unavailable when the process dumpable flag
> is cleared. On such architectures as ia64 this results in all syscall
> arguments being unavailable for the tracer.
>
> Secondly, ptracers also have to support a lot of arch-specific code for
> obtaining information about the tracee. For some architectures, this
> requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
> argument and return value.
>
> ptrace(2) man page:
>
> long ptrace(enum __ptrace_request request, pid_t pid,
> void *addr, void *data);
> ...
> PTRACE_GET_SYSCALL_INFO
> Retrieve information about the syscall that caused the stop.
> The information is placed into the buffer pointed by "data"
> argument, which should be a pointer to a buffer of type
> "struct ptrace_syscall_info".
> The "addr" argument contains the size of the buffer pointed to
> by "data" argument (i.e., sizeof(struct ptrace_syscall_info)).
> The return value contains the number of bytes available
> to be written by the kernel.
> If the size of data to be written by the kernel exceeds the size
> specified by "addr" argument, the output is truncated.
>
> Co-authored-by: Dmitry V. Levin <ldv@altlinux.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Eugene Syromyatnikov <esyr@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: linux-api@vger.kernel.org
> Cc: strace-devel@lists.strace.io
> Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
>
> Notes:
> v5:
> * Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
> * Change struct ptrace_syscall_info: generalize instruction_pointer,
> stack_pointer, and frame_pointer fields by moving them from
> ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
> and initializing them for all stops.
> * Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
> so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
> instruction_pointer when the tracee is in a signal stop.
> * Make available for all architectures: do not conditionalize on
> CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
> are implemented on all architectures.
>
> v4:
> * Do not introduce task_struct.ptrace_event,
> use child->last_siginfo->si_code instead.
> * Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
> support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
> ptrace_syscall_info.{entry,exit}.
>
> v3:
> * Change struct ptrace_syscall_info.
> * Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
> * Add proper defines for ptrace_syscall_info.op values.
> * Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
> PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
> * and move them to uapi.
>
> v2:
> * Do not use task->ptrace.
> * Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
> * Use addr argument of sys_ptrace to get expected size of the struct;
> return full size of the struct.
>
> include/linux/tracehook.h | 9 ++--
> include/uapi/linux/ptrace.h | 39 +++++++++++++++
> kernel/ptrace.c | 99 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 143 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
> index df20f8bdbfa3..6bc7a3d58e2f 100644
> --- a/include/linux/tracehook.h
> +++ b/include/linux/tracehook.h
> @@ -57,13 +57,15 @@ struct linux_binprm;
> /*
> * ptrace report for syscall entry and exit looks identical.
> */
> -static inline int ptrace_report_syscall(struct pt_regs *regs)
> +static inline int ptrace_report_syscall(struct pt_regs *regs,
> + unsigned long message)
> {
> int ptrace = current->ptrace;
>
> if (!(ptrace & PT_PTRACED))
> return 0;
>
> + current->ptrace_message = message;
> ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
>
> /*
> @@ -76,6 +78,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
> current->exit_code = 0;
> }
>
> + current->ptrace_message = 0;
> return fatal_signal_pending(current);
> }
>
> @@ -101,7 +104,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
> static inline __must_check int tracehook_report_syscall_entry(
> struct pt_regs *regs)
> {
> - return ptrace_report_syscall(regs);
> + return ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_ENTRY);
> }
>
> /**
> @@ -126,7 +129,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
> if (step)
> user_single_step_report(regs);
> else
> - ptrace_report_syscall(regs);
> + ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);
> }
>
> /**
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index d5a1b8a492b9..f0af09fe4e17 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -73,6 +73,45 @@ struct seccomp_metadata {
> __u64 flags; /* Output: filter's flags */
> };
>
> +#define PTRACE_GET_SYSCALL_INFO 0x420e
> +#define PTRACE_SYSCALL_INFO_NONE 0
> +#define PTRACE_SYSCALL_INFO_ENTRY 1
> +#define PTRACE_SYSCALL_INFO_EXIT 2
> +#define PTRACE_SYSCALL_INFO_SECCOMP 3
> +
> +struct ptrace_syscall_info {
> + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> + __u8 __pad0[3];
> + __u32 arch;
> + __u64 instruction_pointer;
> + __u64 stack_pointer;
> + __u64 frame_pointer;
> + union {
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + } entry;
> + struct {
> + __s64 rval;
> + __u8 is_error;
> + __u8 __pad1[7];
> + } exit;
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + __u32 ret_data;
> + __u8 __pad2[4];
> + } seccomp;
> + };
> +};
> +
> +/*
> + * These values are stored in task->ptrace_message
> + * by tracehook_report_syscall_* to describe the current syscall-stop.
> + */
> +#define PTRACE_EVENTMSG_SYSCALL_ENTRY 1
> +#define PTRACE_EVENTMSG_SYSCALL_EXIT 2
> +
> /* Read signals from a shared (process wide) queue */
> #define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index c2cee9db5204..4562b2cb1087 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -30,6 +30,8 @@
> #include <linux/cn_proc.h>
> #include <linux/compat.h>
>
> +#include <asm/syscall.h> /* For syscall_get_* */
> +
> /*
> * Access another process' address space via ptrace.
> * Source/target buffer must be kernel space,
> @@ -878,7 +880,98 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> * to ensure no machine forgets it.
> */
> EXPORT_SYMBOL_GPL(task_user_regset_view);
> -#endif
> +#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
> +
> +static unsigned long
> +ptrace_get_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + unsigned long args[ARRAY_SIZE(info->entry.args)];
> + int i;
> +
> + info->op = PTRACE_SYSCALL_INFO_ENTRY;
> + info->entry.nr = syscall_get_nr(child, regs);
> + syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
> + for (i = 0; i < ARRAY_SIZE(args); i++)
> + info->entry.args[i] = args[i];
> +
> + return offsetofend(struct ptrace_syscall_info, entry);
> +}
> +
> +static unsigned long
> +ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + /*
> + * As struct ptrace_syscall_info.entry is currently a subset
> + * of struct ptrace_syscall_info.seccomp, it makes sense to
> + * initialize that subset using ptrace_get_syscall_info_entry().
> + * This can be reconsidered in the future if these structures
> + * diverge significantly enough.
> + */
> + ptrace_get_syscall_info_entry(child, regs, info);
> + info->op = PTRACE_SYSCALL_INFO_SECCOMP;
> + info->seccomp.ret_data = child->ptrace_message;
> +
> + return offsetofend(struct ptrace_syscall_info, seccomp);
> +}
> +
> +static unsigned long
> +ptrace_get_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + info->op = PTRACE_SYSCALL_INFO_EXIT;
> + info->exit.rval = syscall_get_error(child, regs);
> + info->exit.is_error = !!info->exit.rval;
> + if (!info->exit.is_error)
> + info->exit.rval = syscall_get_return_value(child, regs);
> +
> + return offsetofend(struct ptrace_syscall_info, exit);
> +}
> +
> +static int
> +ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> + void __user *datavp)
> +{
> + struct pt_regs *regs = task_pt_regs(child);
> + struct ptrace_syscall_info info = {
> + .op = PTRACE_SYSCALL_INFO_NONE,
> + .arch = syscall_get_arch(child),
> + .instruction_pointer = instruction_pointer(regs),
> + .stack_pointer = user_stack_pointer(regs),
> + .frame_pointer = frame_pointer(regs)
> + };
> + unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
> + unsigned long write_size;
> +
> + /*
> + * This does not need lock_task_sighand() to access
> + * child->last_siginfo because ptrace_freeze_traced()
> + * called earlier by ptrace_check_attach() ensures that
> + * the tracee cannot go away and clear its last_siginfo.
> + */
> + switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
> + case SIGTRAP | 0x80:
> + switch (child->ptrace_message) {
> + case PTRACE_EVENTMSG_SYSCALL_ENTRY:
> + actual_size = ptrace_get_syscall_info_entry(child, regs,
> + &info);
> + break;
> + case PTRACE_EVENTMSG_SYSCALL_EXIT:
> + actual_size = ptrace_get_syscall_info_exit(child, regs,
> + &info);
> + break;
> + }
> + break;
> + case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
> + actual_size = ptrace_get_syscall_info_seccomp(child, regs,
> + &info);
> + break;
> + }
> +
> + write_size = min(actual_size, user_size);
> + return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
> +}
>
> int ptrace_request(struct task_struct *child, long request,
> unsigned long addr, unsigned long data)
> @@ -1095,6 +1188,10 @@ int ptrace_request(struct task_struct *child, long request,
> ret = seccomp_get_metadata(child, addr, datavp);
> break;
>
> + case PTRACE_GET_SYSCALL_INFO:
> + ret = ptrace_get_syscall_info(child, addr, datavp);
> + break;
> +
> default:
> break;
> }
> --
> ldv
--
Kees Cook
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Carlos O'Donell @ 2018-12-10 17:39 UTC (permalink / raw)
To: Jonathan Corbet, Randy Dunlap
Cc: Greg KH, Daniel Colascione, linux-kernel, Joel Fernandes,
Linux API
In-Reply-To: <20181210092706.1aa54432@lwn.net>
On 12/10/18 11:27 AM, Jonathan Corbet wrote:
> On Sat, 8 Dec 2018 20:38:56 -0800
> Randy Dunlap <rdunlap@infradead.org> wrote:
>
>> On 11/12/18 8:08 AM, Jonathan Corbet wrote:
>>> On Sun, 11 Nov 2018 18:36:30 -0800
>>> Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>>> We should have a checklist. That's a great idea. Now to find someone
>>>> to write it... :)
>>>
>>> Do we think the LPC session might have the right people to create such a
>>> thing? If so, I can try to put together a coherent presentation of the
>>> result.
>>
>> Hi,
>> Did anything ever happen with this syscall checklist suggestion?
>
> No, we really didn't have the right people around to do that,
> unfortunately.
We already have Documentation/process/adding-syscalls.rst.
The documentation there is quite thorough.
It lists things that people commonly forget e.g. email linux-api@vger.kernel.org.
Would it be acceptable to attempt to collate per-libc information
into the adding-syscalls.rst under a new section called:
"Integration with libc"
--
Cheers,
Carlos.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Jonathan Corbet @ 2018-12-10 16:27 UTC (permalink / raw)
To: Randy Dunlap
Cc: Greg KH, Carlos O'Donell, Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API
In-Reply-To: <b3d765e1-69a0-d4f0-b04a-40c19df8111c@infradead.org>
On Sat, 8 Dec 2018 20:38:56 -0800
Randy Dunlap <rdunlap@infradead.org> wrote:
> On 11/12/18 8:08 AM, Jonathan Corbet wrote:
> > On Sun, 11 Nov 2018 18:36:30 -0800
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> >> We should have a checklist. That's a great idea. Now to find someone
> >> to write it... :)
> >
> > Do we think the LPC session might have the right people to create such a
> > thing? If so, I can try to put together a coherent presentation of the
> > result.
>
> Hi,
> Did anything ever happen with this syscall checklist suggestion?
No, we really didn't have the right people around to do that,
unfortunately.
jon
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-12-10 16:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Jann Horn, linux-api-u79uwXL29TY76Z2rM5mHXA,
Eugene Syromyatnikov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181210141107.GB4177-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1079 bytes --]
On Mon, Dec 10, 2018 at 03:11:07PM +0100, Oleg Nesterov wrote:
> On 12/10, Dmitry V. Levin wrote:
> >
> > +struct ptrace_syscall_info {
> > + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> > + __u8 __pad0[3];
> > + __u32 arch;
> > + __u64 instruction_pointer;
> > + __u64 stack_pointer;
> > + __u64 frame_pointer;
> > + union {
> > + struct {
> > + __u64 nr;
> > + __u64 args[6];
> > + } entry;
> > + struct {
> > + __s64 rval;
> > + __u8 is_error;
> > + __u8 __pad1[7];
> > + } exit;
> > + struct {
> > + __u64 nr;
> > + __u64 args[6];
> > + __u32 ret_data;
> > + __u8 __pad2[4];
> > + } seccomp;
> > + };
> > +};
>
> Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
> understand why...
I suppose the idea behind the use of these pads was to make the structure
arch-independent.
I don't think we really need to keep it exactly the same on all
architectures - the only practical requirement is to avoid any compat
issues, but I don't mind keeping the structure arch-independent.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-12-10 16:09 UTC (permalink / raw)
To: Paul Burton, Ralf Baechle, James Hogan, Oleg Nesterov,
Andy Lutomirski
Cc: Kees Cook, Jann Horn, linux-api-u79uwXL29TY76Z2rM5mHXA,
Eugene Syromyatnikov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <201812102200.snodXJSH%fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 4038 bytes --]
Hi, things are getting too complicated and we need some advice how to deal
with this frame_pointer issue.
On Mon, Dec 10, 2018 at 10:26:50PM +0800, kbuild test robot wrote:
> Hi Elvira,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.20-rc6]
> [cannot apply to next-20181207]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
> config: mips-malta_kvm_defconfig (attached as .config)
> compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.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.2.0 make.cross ARCH=mips
>
> All errors (new ones prefixed by >>):
>
> kernel/ptrace.c: In function 'ptrace_get_syscall_info':
> >> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
> .frame_pointer = frame_pointer(regs)
> ^~~~~~~~~~~~~
> trace_printk
> cc1: some warnings being treated as errors
>
> vim +942 kernel/ptrace.c
>
> 931
> 932 static int
> 933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> 934 void __user *datavp)
> 935 {
> 936 struct pt_regs *regs = task_pt_regs(child);
> 937 struct ptrace_syscall_info info = {
> 938 .op = PTRACE_SYSCALL_INFO_NONE,
> 939 .arch = syscall_get_arch(child),
> 940 .instruction_pointer = instruction_pointer(regs),
> 941 .stack_pointer = user_stack_pointer(regs),
> > 942 .frame_pointer = frame_pointer(regs)
> 943 };
> 944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
> 945 unsigned long write_size;
> 946
> 947 /*
> 948 * This does not need lock_task_sighand() to access
> 949 * child->last_siginfo because ptrace_freeze_traced()
> 950 * called earlier by ptrace_check_attach() ensures that
> 951 * the tracee cannot go away and clear its last_siginfo.
> 952 */
> 953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
> 954 case SIGTRAP | 0x80:
> 955 switch (child->ptrace_message) {
> 956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
> 957 actual_size = ptrace_get_syscall_info_entry(child, regs,
> 958 &info);
> 959 break;
> 960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
> 961 actual_size = ptrace_get_syscall_info_exit(child, regs,
> 962 &info);
> 963 break;
> 964 }
> 965 break;
> 966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
> 967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
> 968 &info);
> 969 break;
> 970 }
> 971
> 972 write_size = min(actual_size, user_size);
> 973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
> 974 }
> 975
We decided to add .frame_pointer to struct ptrace_syscall_info just for
consistency with .instruction_pointer and .stack_pointer; I must have been
misled by comments in asm-generic/ptrace.h into thinking that
frame_pointer() is universally available across architectures.
Unlike .instruction_pointer and .stack_pointer that are actually needed
in strace, .frame_pointer is not used, so from strace PoV we don't really
need it.
So the question is, does anybody need a
struct ptrace_syscall_info.frame_pointer?
If yes, how can frame_pointer() be defined on MIPS?
Or should we just forget about making sense of frame_pointer() and remove
struct ptrace_syscall_info.frame_pointer from the proposed API?
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: kbuild test robot @ 2018-12-10 14:26 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: kbuild-all, Oleg Nesterov, Andy Lutomirski, Elvira Khabirova,
Eugene Syromyatnikov, Kees Cook, Jann Horn, linux-api,
strace-devel, linux-kernel
In-Reply-To: <20181210043126.GX6131@altlinux.org>
[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]
Hi Elvira,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc6]
[cannot apply to next-20181207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-V-Levin/ptrace-add-PTRACE_GET_SYSCALL_INFO-request/20181210-174745
config: mips-malta_kvm_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.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.2.0 make.cross ARCH=mips
All errors (new ones prefixed by >>):
kernel/ptrace.c: In function 'ptrace_get_syscall_info':
>> kernel/ptrace.c:942:20: error: implicit declaration of function 'frame_pointer'; did you mean 'trace_printk'? [-Werror=implicit-function-declaration]
.frame_pointer = frame_pointer(regs)
^~~~~~~~~~~~~
trace_printk
cc1: some warnings being treated as errors
vim +942 kernel/ptrace.c
931
932 static int
933 ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
934 void __user *datavp)
935 {
936 struct pt_regs *regs = task_pt_regs(child);
937 struct ptrace_syscall_info info = {
938 .op = PTRACE_SYSCALL_INFO_NONE,
939 .arch = syscall_get_arch(child),
940 .instruction_pointer = instruction_pointer(regs),
941 .stack_pointer = user_stack_pointer(regs),
> 942 .frame_pointer = frame_pointer(regs)
943 };
944 unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
945 unsigned long write_size;
946
947 /*
948 * This does not need lock_task_sighand() to access
949 * child->last_siginfo because ptrace_freeze_traced()
950 * called earlier by ptrace_check_attach() ensures that
951 * the tracee cannot go away and clear its last_siginfo.
952 */
953 switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
954 case SIGTRAP | 0x80:
955 switch (child->ptrace_message) {
956 case PTRACE_EVENTMSG_SYSCALL_ENTRY:
957 actual_size = ptrace_get_syscall_info_entry(child, regs,
958 &info);
959 break;
960 case PTRACE_EVENTMSG_SYSCALL_EXIT:
961 actual_size = ptrace_get_syscall_info_exit(child, regs,
962 &info);
963 break;
964 }
965 break;
966 case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
967 actual_size = ptrace_get_syscall_info_seccomp(child, regs,
968 &info);
969 break;
970 }
971
972 write_size = min(actual_size, user_size);
973 return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
974 }
975
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19472 bytes --]
^ permalink raw reply
* Re: [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Oleg Nesterov @ 2018-12-10 14:11 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
Kees Cook, Jann Horn, linux-api, strace-devel, linux-kernel
In-Reply-To: <20181210043126.GX6131@altlinux.org>
On 12/10, Dmitry V. Levin wrote:
>
> +struct ptrace_syscall_info {
> + __u8 op; /* PTRACE_SYSCALL_INFO_* */
> + __u8 __pad0[3];
> + __u32 arch;
> + __u64 instruction_pointer;
> + __u64 stack_pointer;
> + __u64 frame_pointer;
> + union {
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + } entry;
> + struct {
> + __s64 rval;
> + __u8 is_error;
> + __u8 __pad1[7];
> + } exit;
> + struct {
> + __u64 nr;
> + __u64 args[6];
> + __u32 ret_data;
> + __u8 __pad2[4];
> + } seccomp;
> + };
> +};
Could you explain why ptrace_syscall_info needs __pad{0,1,2} ? I simply can't
understand why...
Otherwise the patch looks good to me. I am not going to discuss the API and
data layout, I am fine with anything which suits user-space needs.
I think the patch is technically correct, feel free to add
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* [PATCH v5 24/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-12-10 4:31 UTC (permalink / raw)
To: Oleg Nesterov, Andy Lutomirski
Cc: Kees Cook, Jann Horn, linux-api-u79uwXL29TY76Z2rM5mHXA,
Eugene Syromyatnikov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181210042352.GA6092@altlinux.org>
From: Elvira Khabirova <lineprinter-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.
There are two reasons for a special syscall-related ptrace request.
Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable for the tracer.
Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.
ptrace(2) man page:
long ptrace(enum __ptrace_request request, pid_t pid,
void *addr, void *data);
...
PTRACE_GET_SYSCALL_INFO
Retrieve information about the syscall that caused the stop.
The information is placed into the buffer pointed by "data"
argument, which should be a pointer to a buffer of type
"struct ptrace_syscall_info".
The "addr" argument contains the size of the buffer pointed to
by "data" argument (i.e., sizeof(struct ptrace_syscall_info)).
The return value contains the number of bytes available
to be written by the kernel.
If the size of data to be written by the kernel exceeds the size
specified by "addr" argument, the output is truncated.
Co-authored-by: Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Eugene Syromyatnikov <esyr-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jann Horn <jannh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
Signed-off-by: Elvira Khabirova <lineprinter-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
Signed-off-by: Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
---
Notes:
v5:
* Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
* Change struct ptrace_syscall_info: generalize instruction_pointer,
stack_pointer, and frame_pointer fields by moving them from
ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
and initializing them for all stops.
* Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
instruction_pointer when the tracee is in a signal stop.
* Make available for all architectures: do not conditionalize on
CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
are implemented on all architectures.
v4:
* Do not introduce task_struct.ptrace_event,
use child->last_siginfo->si_code instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.
v3:
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.
v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.
include/linux/tracehook.h | 9 ++--
include/uapi/linux/ptrace.h | 39 +++++++++++++++
kernel/ptrace.c | 99 ++++++++++++++++++++++++++++++++++++-
3 files changed, 143 insertions(+), 4 deletions(-)
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index df20f8bdbfa3..6bc7a3d58e2f 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -57,13 +57,15 @@ struct linux_binprm;
/*
* ptrace report for syscall entry and exit looks identical.
*/
-static inline int ptrace_report_syscall(struct pt_regs *regs)
+static inline int ptrace_report_syscall(struct pt_regs *regs,
+ unsigned long message)
{
int ptrace = current->ptrace;
if (!(ptrace & PT_PTRACED))
return 0;
+ current->ptrace_message = message;
ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
/*
@@ -76,6 +78,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
current->exit_code = 0;
}
+ current->ptrace_message = 0;
return fatal_signal_pending(current);
}
@@ -101,7 +104,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
static inline __must_check int tracehook_report_syscall_entry(
struct pt_regs *regs)
{
- return ptrace_report_syscall(regs);
+ return ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_ENTRY);
}
/**
@@ -126,7 +129,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
if (step)
user_single_step_report(regs);
else
- ptrace_report_syscall(regs);
+ ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);
}
/**
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..f0af09fe4e17 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,45 @@ struct seccomp_metadata {
__u64 flags; /* Output: filter's flags */
};
+#define PTRACE_GET_SYSCALL_INFO 0x420e
+#define PTRACE_SYSCALL_INFO_NONE 0
+#define PTRACE_SYSCALL_INFO_ENTRY 1
+#define PTRACE_SYSCALL_INFO_EXIT 2
+#define PTRACE_SYSCALL_INFO_SECCOMP 3
+
+struct ptrace_syscall_info {
+ __u8 op; /* PTRACE_SYSCALL_INFO_* */
+ __u8 __pad0[3];
+ __u32 arch;
+ __u64 instruction_pointer;
+ __u64 stack_pointer;
+ __u64 frame_pointer;
+ union {
+ struct {
+ __u64 nr;
+ __u64 args[6];
+ } entry;
+ struct {
+ __s64 rval;
+ __u8 is_error;
+ __u8 __pad1[7];
+ } exit;
+ struct {
+ __u64 nr;
+ __u64 args[6];
+ __u32 ret_data;
+ __u8 __pad2[4];
+ } seccomp;
+ };
+};
+
+/*
+ * These values are stored in task->ptrace_message
+ * by tracehook_report_syscall_* to describe the current syscall-stop.
+ */
+#define PTRACE_EVENTMSG_SYSCALL_ENTRY 1
+#define PTRACE_EVENTMSG_SYSCALL_EXIT 2
+
/* Read signals from a shared (process wide) queue */
#define PTRACE_PEEKSIGINFO_SHARED (1 << 0)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index c2cee9db5204..4562b2cb1087 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -30,6 +30,8 @@
#include <linux/cn_proc.h>
#include <linux/compat.h>
+#include <asm/syscall.h> /* For syscall_get_* */
+
/*
* Access another process' address space via ptrace.
* Source/target buffer must be kernel space,
@@ -878,7 +880,98 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
* to ensure no machine forgets it.
*/
EXPORT_SYMBOL_GPL(task_user_regset_view);
-#endif
+#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
+
+static unsigned long
+ptrace_get_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ unsigned long args[ARRAY_SIZE(info->entry.args)];
+ int i;
+
+ info->op = PTRACE_SYSCALL_INFO_ENTRY;
+ info->entry.nr = syscall_get_nr(child, regs);
+ syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
+ for (i = 0; i < ARRAY_SIZE(args); i++)
+ info->entry.args[i] = args[i];
+
+ return offsetofend(struct ptrace_syscall_info, entry);
+}
+
+static unsigned long
+ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ /*
+ * As struct ptrace_syscall_info.entry is currently a subset
+ * of struct ptrace_syscall_info.seccomp, it makes sense to
+ * initialize that subset using ptrace_get_syscall_info_entry().
+ * This can be reconsidered in the future if these structures
+ * diverge significantly enough.
+ */
+ ptrace_get_syscall_info_entry(child, regs, info);
+ info->op = PTRACE_SYSCALL_INFO_SECCOMP;
+ info->seccomp.ret_data = child->ptrace_message;
+
+ return offsetofend(struct ptrace_syscall_info, seccomp);
+}
+
+static unsigned long
+ptrace_get_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ info->op = PTRACE_SYSCALL_INFO_EXIT;
+ info->exit.rval = syscall_get_error(child, regs);
+ info->exit.is_error = !!info->exit.rval;
+ if (!info->exit.is_error)
+ info->exit.rval = syscall_get_return_value(child, regs);
+
+ return offsetofend(struct ptrace_syscall_info, exit);
+}
+
+static int
+ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
+ void __user *datavp)
+{
+ struct pt_regs *regs = task_pt_regs(child);
+ struct ptrace_syscall_info info = {
+ .op = PTRACE_SYSCALL_INFO_NONE,
+ .arch = syscall_get_arch(child),
+ .instruction_pointer = instruction_pointer(regs),
+ .stack_pointer = user_stack_pointer(regs),
+ .frame_pointer = frame_pointer(regs)
+ };
+ unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
+ unsigned long write_size;
+
+ /*
+ * This does not need lock_task_sighand() to access
+ * child->last_siginfo because ptrace_freeze_traced()
+ * called earlier by ptrace_check_attach() ensures that
+ * the tracee cannot go away and clear its last_siginfo.
+ */
+ switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
+ case SIGTRAP | 0x80:
+ switch (child->ptrace_message) {
+ case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+ actual_size = ptrace_get_syscall_info_entry(child, regs,
+ &info);
+ break;
+ case PTRACE_EVENTMSG_SYSCALL_EXIT:
+ actual_size = ptrace_get_syscall_info_exit(child, regs,
+ &info);
+ break;
+ }
+ break;
+ case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
+ actual_size = ptrace_get_syscall_info_seccomp(child, regs,
+ &info);
+ break;
+ }
+
+ write_size = min(actual_size, user_size);
+ return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
+}
int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
@@ -1095,6 +1188,10 @@ int ptrace_request(struct task_struct *child, long request,
ret = seccomp_get_metadata(child, addr, datavp);
break;
+ case PTRACE_GET_SYSCALL_INFO:
+ ret = ptrace_get_syscall_info(child, addr, datavp);
+ break;
+
default:
break;
}
--
ldv
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply related
* [PATCH v5 00/25] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-12-10 4:23 UTC (permalink / raw)
To: Oleg Nesterov, Andy Lutomirski
Cc: linux-s390, Rich Felker, linux-ia64, linux-sh,
Benjamin Herrenschmidt, Alexey Brodkin, Heiko Carstens, linux-api,
James E.J. Bottomley, Max Filippov, Guo Ren, Ralf Baechle,
linux-kselftest, H. Peter Anvin, Breno Leitao, Russell King,
linux-riscv, Vincent Chen, Shuah Khan, Thomas Gleixner,
Paul Mackerras, Jonas Bonn, Elvira Khabirova, sparclinux,
linux-arch
PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.
There are two reasons for a special syscall-related ptrace request.
Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable for the tracer.
Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.
PTRACE_GET_SYSCALL_INFO returns the following structure:
struct ptrace_syscall_info {
__u8 op; /* PTRACE_SYSCALL_INFO_* */
__u8 __pad0[3];
__u32 arch;
__u64 instruction_pointer;
__u64 stack_pointer;
__u64 frame_pointer;
union {
struct {
__u64 nr;
__u64 args[6];
} entry;
struct {
__s64 rval;
__u8 is_error;
__u8 __pad1[7];
} exit;
struct {
__u64 nr;
__u64 args[6];
__u32 ret_data;
__u8 __pad2[4];
} seccomp;
};
};
The structure was chosen according to [2], except for the following
changes:
* seccomp substructure was added as a superset of entry substructure;
* the type of nr field was changed from int to __u64 because syscall
numbers are, as a practical matter, 64 bits;
* stack_pointer and frame_pointer fields were added along with
instruction_pointer field since they are readily available and can save
the tracer from extra PTRACE_GETREGS/PTRACE_GETREGSET calls;
* arch is always initialized to aid with tracing system calls such as
execve();
* instruction_pointer, stack_pointer, and frame_pointer are always
initialized so they could be easily obtained for non-syscall stops;
* a boolean is_error field was added along with rval field, this way
the tracer can more reliably distinguish a return value
from an error value.
strace has been ported to PTRACE_GET_SYSCALL_INFO, you can find it
at [3] and [4].
[1] https://lore.kernel.org/lkml/CA+55aFzcSVmdDj9Lh_gdbz1OzHyEm6ZrGPBDAJnywm2LF_eVyg@mail.gmail.com/
[2] https://lore.kernel.org/lkml/CAObL_7GM0n80N7J_DFw_eQyfLyzq+sf4y2AvsCCV88Tb3AwEHA@mail.gmail.com/
[3] https://github.com/strace/strace/commits/ldv/PTRACE_GET_SYSCALL_INFO
[4] https://gitlab.com/strace/strace/commits/ldv/PTRACE_GET_SYSCALL_INFO
Notes:
v5:
* Merge separate series and patches into the single series.
* Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
* Change struct ptrace_syscall_info: generalize instruction_pointer,
stack_pointer, and frame_pointer fields by moving them from
ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
and initializing them for all stops.
* Add PTRACE_SYSCALL_INFO_NONE, assign it to ptrace_syscall_info.op
when not in a syscall stop, so e.g. "strace -i" could use the same
PTRACE_SYSCALL_INFO_SECCOMP interface to obtain instruction_pointer
when the tracee is in a signal stop.
* Patch all remaining architectures to provide all necessary
syscall_get_* functions.
* Make available for all architectures: do not conditionalize on
CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
are implemented on all architectures.
* Add a test for PTRACE_GET_SYSCALL_INFO to selftests/ptrace.
v4:
* Do not introduce task_struct.ptrace_event,
use child->last_siginfo->si_code instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.
v3:
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.
v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.
Dmitry V. Levin (23):
alpha: define remaining syscall_get_* functions
Move EM_ARCOMPACT and EM_ARCV2 to uapi/linux/elf-em.h
arc: define syscall_get_arch()
c6x: define syscall_get_arch()
elf-em.h: add EM_CSKY
csky: define syscall_get_arch()
h8300: define remaining syscall_get_* functions
Move EM_HEXAGON to uapi/linux/elf-em.h
hexagon: define remaining syscall_get_* functions
Move EM_NDS32 to uapi/linux/elf-em.h
nds32: define syscall_get_arch()
nios2: define syscall_get_arch()
m68k: add asm/syscall.h
mips: define syscall_get_error()
parisc: define syscall_get_error()
powerpc: define syscall_get_error()
riscv: define syscall_get_arch()
Move EM_XTENSA to uapi/linux/elf-em.h
xtensa: define syscall_get_* functions
Move EM_UNICORE to uapi/linux/elf-em.h
unicore32: add asm/syscall.h
syscall_get_arch: add "struct task_struct *" argument
selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO
Elvira Khabirova (2):
powerpc/ptrace: replace ptrace_report_syscall() with a tracehook call
ptrace: add PTRACE_GET_SYSCALL_INFO request
arch/alpha/include/asm/syscall.h | 29 +-
arch/arc/include/asm/elf.h | 6 +-
arch/arc/include/asm/syscall.h | 11 +
arch/arm/include/asm/syscall.h | 2 +-
arch/arm64/include/asm/syscall.h | 4 +-
arch/c6x/include/asm/syscall.h | 7 +
arch/csky/include/asm/syscall.h | 7 +
arch/h8300/include/asm/syscall.h | 18 ++
arch/hexagon/include/asm/elf.h | 6 +-
arch/hexagon/include/asm/syscall.h | 20 ++
arch/ia64/include/asm/syscall.h | 2 +-
arch/m68k/include/asm/syscall.h | 39 +++
arch/microblaze/include/asm/syscall.h | 2 +-
arch/mips/include/asm/syscall.h | 12 +-
arch/mips/kernel/ptrace.c | 2 +-
arch/nds32/include/asm/elf.h | 3 +-
arch/nds32/include/asm/syscall.h | 8 +
arch/nios2/include/asm/syscall.h | 6 +
arch/openrisc/include/asm/syscall.h | 2 +-
arch/parisc/include/asm/syscall.h | 11 +-
arch/powerpc/include/asm/syscall.h | 20 +-
arch/powerpc/kernel/ptrace.c | 7 +-
arch/riscv/include/asm/syscall.h | 10 +
arch/s390/include/asm/syscall.h | 4 +-
arch/sh/include/asm/syscall_32.h | 2 +-
arch/sh/include/asm/syscall_64.h | 2 +-
arch/sparc/include/asm/syscall.h | 5 +-
arch/unicore32/include/asm/elf.h | 3 +-
arch/unicore32/include/asm/syscall.h | 45 +++
arch/x86/include/asm/syscall.h | 8 +-
arch/x86/um/asm/syscall.h | 2 +-
arch/xtensa/include/asm/elf.h | 2 +-
arch/xtensa/include/asm/syscall.h | 69 +++++
include/asm-generic/syscall.h | 5 +-
include/linux/tracehook.h | 9 +-
include/uapi/linux/audit.h | 16 ++
include/uapi/linux/elf-em.h | 8 +
include/uapi/linux/ptrace.h | 39 +++
kernel/auditsc.c | 4 +-
kernel/ptrace.c | 99 ++++++-
kernel/seccomp.c | 4 +-
tools/testing/selftests/ptrace/.gitignore | 1 +
tools/testing/selftests/ptrace/Makefile | 2 +-
.../selftests/ptrace/get_syscall_info.c | 272 ++++++++++++++++++
44 files changed, 783 insertions(+), 52 deletions(-)
create mode 100644 arch/m68k/include/asm/syscall.h
create mode 100644 arch/unicore32/include/asm/syscall.h
create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c
--
ldv
^ permalink raw reply
* [PATCH v10 4/4] samples: add an example of seccomp user trap
From: Tycho Andersen @ 2018-12-09 18:24 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Aleksa Sarai, Jann Horn, linux-kernel, containers, linux-api,
Tycho Andersen
In-Reply-To: <20181209182414.30862-1-tycho@tycho.ws>
The idea here is just to give a demonstration of how one could safely use
the SECCOMP_RET_USER_NOTIF feature to do mount policies. This particular
policy is (as noted in the comment) not very interesting, but it serves to
illustrate how one might apply a policy dodging the various TOCTOU issues.
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Eric W. Biederman <ebiederm@xmission.com>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Christian Brauner <christian@brauner.io>
CC: Tyler Hicks <tyhicks@canonical.com>
CC: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
---
v5: new in v5
v7: updates for v7 API changes
v8: * add some more comments about what's happening in main() (Kees)
* move from ptrace API to SECCOMP_FILTER_FLAG_NEW_LISTENER
v9: * s/mknod/mount in error message
* switch to the SECCOMP_GET_NOTIF_SIZES API
* add a note about getting ENOENT from SECCOMP_IOCTL_NOTIF_SEND
---
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 7 +-
samples/seccomp/user-trap.c | 375 ++++++++++++++++++++++++++++++++++++
3 files changed, 382 insertions(+), 1 deletion(-)
diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
index 78fb78184291..d1e2e817d556 100644
--- a/samples/seccomp/.gitignore
+++ b/samples/seccomp/.gitignore
@@ -1,3 +1,4 @@
bpf-direct
bpf-fancy
dropper
+user-trap
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index cf34ff6b4065..4920903c8009 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef CROSS_COMPILE
-hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct
+hostprogs-$(CONFIG_SAMPLE_SECCOMP) := bpf-fancy dropper bpf-direct user-trap
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
@@ -16,6 +16,10 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
+HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
+user-trap-objs := user-trap.o
+
# Try to match the kernel target.
ifndef CONFIG_64BIT
@@ -33,6 +37,7 @@ HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
HOSTLDLIBS_bpf-direct += $(MFLAG)
HOSTLDLIBS_bpf-fancy += $(MFLAG)
HOSTLDLIBS_dropper += $(MFLAG)
+HOSTLDLIBS_user-trap += $(MFLAG)
endif
always := $(hostprogs-m)
endif
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
new file mode 100644
index 000000000000..61267cb59c8e
--- /dev/null
+++ b/samples/seccomp/user-trap.c
@@ -0,0 +1,375 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stddef.h>
+#include <sys/sysmacros.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <sys/user.h>
+#include <sys/ioctl.h>
+#include <sys/ptrace.h>
+#include <sys/mount.h>
+#include <linux/limits.h>
+#include <linux/filter.h>
+#include <linux/seccomp.h>
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
+static int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+
+static int send_fd(int sock, int fd)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+ *((int *)CMSG_DATA(cmsg)) = fd;
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ if (sendmsg(sock, &msg, 0) < 0) {
+ perror("sendmsg");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int recv_fd(int sock)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+
+ if (recvmsg(sock, &msg, 0) < 0) {
+ perror("recvmsg");
+ return -1;
+ }
+
+ cmsg = CMSG_FIRSTHDR(&msg);
+
+ return *((int *)CMSG_DATA(cmsg));
+}
+
+static int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+static int handle_req(struct seccomp_notif *req,
+ struct seccomp_notif_resp *resp, int listener)
+{
+ char path[PATH_MAX], source[PATH_MAX], target[PATH_MAX];
+ int ret = -1, mem;
+
+ resp->id = req->id;
+ resp->error = -EPERM;
+ resp->val = 0;
+
+ if (req->data.nr != __NR_mount) {
+ fprintf(stderr, "huh? trapped something besides mount? %d\n", req->data.nr);
+ return -1;
+ }
+
+ /* Only allow bind mounts. */
+ if (!(req->data.args[3] & MS_BIND))
+ return 0;
+
+ /*
+ * Ok, let's read the task's memory to see where they wanted their
+ * mount to go.
+ */
+ snprintf(path, sizeof(path), "/proc/%d/mem", req->pid);
+ mem = open(path, O_RDONLY);
+ if (mem < 0) {
+ perror("open mem");
+ return -1;
+ }
+
+ /*
+ * Now we avoid a TOCTOU: we referred to a pid by its pid, but since
+ * the pid that made the syscall may have died, we need to confirm that
+ * the pid is still valid after we open its /proc/pid/mem file. We can
+ * ask the listener fd this as follows.
+ *
+ * Note that this check should occur *after* any task-specific
+ * resources are opened, to make sure that the task has not died and
+ * we're not wrongly reading someone else's state in order to make
+ * decisions.
+ */
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_ID_VALID, &req->id) < 0) {
+ fprintf(stderr, "task died before we could map its memory\n");
+ goto out;
+ }
+
+ /*
+ * Phew, we've got the right /proc/pid/mem. Now we can read it. Note
+ * that to avoid another TOCTOU, we should read all of the pointer args
+ * before we decide to allow the syscall.
+ */
+ if (lseek(mem, req->data.args[0], SEEK_SET) < 0) {
+ perror("seek");
+ goto out;
+ }
+
+ ret = read(mem, source, sizeof(source));
+ if (ret < 0) {
+ perror("read");
+ goto out;
+ }
+
+ if (lseek(mem, req->data.args[1], SEEK_SET) < 0) {
+ perror("seek");
+ goto out;
+ }
+
+ ret = read(mem, target, sizeof(target));
+ if (ret < 0) {
+ perror("read");
+ goto out;
+ }
+
+ /*
+ * Our policy is to only allow bind mounts inside /tmp. This isn't very
+ * interesting, because we could do unprivlieged bind mounts with user
+ * namespaces already, but you get the idea.
+ */
+ if (!strncmp(source, "/tmp", 4) && !strncmp(target, "/tmp", 4)) {
+ if (mount(source, target, NULL, req->data.args[3], NULL) < 0) {
+ ret = -1;
+ perror("actual mount");
+ goto out;
+ }
+ resp->error = 0;
+ }
+
+ /* Even if we didn't allow it because of policy, generating the
+ * response was be a success, because we want to tell the worker EPERM.
+ */
+ ret = 0;
+
+out:
+ close(mem);
+ return ret;
+}
+
+int main(void)
+{
+ int sk_pair[2], ret = 1, status, listener;
+ pid_t worker = 0 , tracer = 0;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ perror("socketpair");
+ return 1;
+ }
+
+ worker = fork();
+ if (worker < 0) {
+ perror("fork");
+ goto close_pair;
+ }
+
+ if (worker == 0) {
+ listener = user_trap_syscall(__NR_mount,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ if (listener < 0) {
+ perror("seccomp");
+ exit(1);
+ }
+
+ /*
+ * Drop privileges. We definitely can't mount as uid 1000.
+ */
+ if (setuid(1000) < 0) {
+ perror("setuid");
+ exit(1);
+ }
+
+ /*
+ * Send the listener to the parent; also serves as
+ * synchronization.
+ */
+ if (send_fd(sk_pair[1], listener) < 0)
+ exit(1);
+ close(listener);
+
+ if (mkdir("/tmp/foo", 0755) < 0) {
+ perror("mkdir");
+ exit(1);
+ }
+
+ /*
+ * Try a bad mount just for grins.
+ */
+ if (mount("/dev/sda", "/tmp/foo", NULL, 0, NULL) != -1) {
+ fprintf(stderr, "huh? mounted /dev/sda?\n");
+ exit(1);
+ }
+
+ if (errno != EPERM) {
+ perror("bad error from mount");
+ exit(1);
+ }
+
+ /*
+ * Ok, we expect this one to succeed.
+ */
+ if (mount("/tmp/foo", "/tmp/foo", NULL, MS_BIND, NULL) < 0) {
+ perror("mount");
+ exit(1);
+ }
+
+ exit(0);
+ }
+
+ /*
+ * Get the listener from the child.
+ */
+ listener = recv_fd(sk_pair[0]);
+ if (listener < 0)
+ goto out_kill;
+
+ /*
+ * Fork a task to handle the requests. This isn't strictly necessary,
+ * but it makes the particular writing of this sample easier, since we
+ * can just wait ofr the tracee to exit and kill the tracer.
+ */
+ tracer = fork();
+ if (tracer < 0) {
+ perror("fork");
+ goto out_kill;
+ }
+
+ if (tracer == 0) {
+ struct seccomp_notif *req;
+ struct seccomp_notif_resp *resp;
+ struct seccomp_notif_sizes sizes;
+
+ if (seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes) < 0) {
+ perror("seccomp(GET_NOTIF_SIZES)");
+ goto out_close;
+ }
+
+ req = malloc(sizes.seccomp_notif);
+ if (!req)
+ goto out_close;
+ memset(req, 0, sizeof(*req));
+
+ resp = malloc(sizes.seccomp_notif_resp);
+ if (!resp)
+ goto out_req;
+ memset(resp, 0, sizeof(*resp));
+
+ while (1) {
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
+ perror("ioctl recv");
+ goto out_resp;
+ }
+
+ if (handle_req(req, resp, listener) < 0)
+ goto out_resp;
+
+ /*
+ * ENOENT here means that the task may have gotten a
+ * signal and restarted the syscall. It's up to the
+ * handler to decide what to do in this case, but for
+ * the sample code, we just ignore it. Probably
+ * something better should happen, like undoing the
+ * mount, or keeping track of the args to make sure we
+ * don't do it again.
+ */
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0 &&
+ errno != ENOENT) {
+ perror("ioctl send");
+ goto out_resp;
+ }
+ }
+out_resp:
+ free(resp);
+out_req:
+ free(req);
+out_close:
+ close(listener);
+ exit(1);
+ }
+
+ close(listener);
+
+ if (waitpid(worker, &status, 0) != worker) {
+ perror("waitpid");
+ goto out_kill;
+ }
+
+ if (umount2("/tmp/foo", MNT_DETACH) < 0 && errno != EINVAL) {
+ perror("umount2");
+ goto out_kill;
+ }
+
+ if (remove("/tmp/foo") < 0 && errno != ENOENT) {
+ perror("remove");
+ exit(1);
+ }
+
+ if (!WIFEXITED(status) || WEXITSTATUS(status)) {
+ fprintf(stderr, "worker exited nonzero\n");
+ goto out_kill;
+ }
+
+ ret = 0;
+
+out_kill:
+ if (tracer > 0)
+ kill(tracer, SIGKILL);
+ if (worker > 0)
+ kill(worker, SIGKILL);
+
+close_pair:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+ return ret;
+}
--
2.19.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox