* Re: RFC: userspace exception fixups
From: Jarkko Sakkinen @ 2018-11-20 10:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Hansen, Christopherson, Sean J, Jethro Beekman,
Florian Weimer, Linux API, Jann Horn, Linus Torvalds, X86 ML,
linux-arch, LKML, Peter Zijlstra, Rich Felker, nhorman,
npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <CALCETrWRVgPjnDd-CknskrXfNbke=rSnr_jezszNNWJUM_YJKQ@mail.gmail.com>
On Mon, Nov 19, 2018 at 09:00:08AM -0800, Andy Lutomirski wrote:
> On Mon, Nov 19, 2018 at 8:02 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Nov 19, 2018 at 07:29:36AM -0800, Andy Lutomirski wrote:
> > > 1. The kernel needs some way to know *when* to apply this fixup.
> > > Decoding the instruction stream and doing it to all exceptions that
> > > hit an ENCLU instruction seems like a poor design.
> >
> > I'm not sure why you would ever need to do any type of fixup as the idea
> > is to just return to AEP i.e. from chosen exceptions (EPCM, #UD) the AEP
> > would work the same way as for exceptions that the kernel can deal with
> > except filling the exception information to registers.
>
> Sure, but how does the kernel know when to do that and when to send a
> signal? I don't really like decoding the instruction stream to figure
> it out.
Hmm... why you have to decode instruction stream to find that out? Would
just depend on exception type (#GP with EPCM, #UD). Or are you saying
that kernel should need to SIGSEGV if there is in fact ENCLU so that
there is no infinite trap loop? Sorry, I'm a bit lost here that where
does this decoding requirement comes from in the first place. I
understand how it is used in Sean's proposal...
Anyway, this option can be probably discarded without further
consideration because apparently single stepping can cause #DB SS fault
if AEP handler is anything else than a single instruction.
For me it seems that by ruling out options, vDSO option is what is
left. I don't like it but at least it works...
/Jarkko
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-20 10:31 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Daniel Colascione, Aleksa Sarai, linux-kernel, Serge E. Hallyn,
Jann Horn, Andy Lutomirski, Andrew Morton, Oleg Nesterov, Al Viro,
Linux FS Devel, Linux API, Tim Murray, linux-man, Kees Cook
In-Reply-To: <87muq4xs2n.fsf@xmission.com>
On Mon, Nov 19, 2018 at 10:59:12PM -0600, Eric W. Biederman wrote:
> Daniel Colascione <dancol@google.com> writes:
>
> > On Mon, Nov 19, 2018 at 1:37 PM Christian Brauner <christian@brauner.io> wrote:
> >>
> >> On Mon, Nov 19, 2018 at 01:26:22PM -0800, Daniel Colascione wrote:
> >> > On Mon, Nov 19, 2018 at 1:21 PM, Christian Brauner <christian@brauner.io> wrote:
> >> > > That can be done without a loop by comparing the level counter for the
> >> > > two pid namespaces.
> >> > >
> >> > >>
> >> > >> And you can rewrite pidns_get_parent to use it. So you would instead be
> >> > >> doing:
> >> > >>
> >> > >> if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
> >> > >> return -EPERM;
> >> > >>
> >> > >> (Or you can just copy the 5-line loop into procfd_signal -- though I
> >> > >> imagine we'll need this for all of the procfd_* APIs.)
> >> >
> >> > Why is any of this even necessary? Why does the child namespace we're
> >> > considering even have a file descriptor to its ancestor's procfs? If
> >>
> >> Because you can send file descriptors between processes and container
> >> runtimes tend to do that.
> >
> > Right. But why *would* a container runtime send one of these procfs
> > FDs to a container?
> >
> >> > it has one of these FDs, it can already *read* all sorts of
> >> > information it really shouldn't be able to acquire, so the additional
> >> > ability to send a signal (subject to the usual permission checks)
> >> > feels like sticking a finger in a dike that's already well-perforated.
> >> > IMHO, we shouldn't bother with this check. The patch would be simpler
> >> > without it.
> >>
> >> We will definitely not allow signaling processes in an ancestor pid
> >> namespace! That is a security issue! I can imagine container runtimes
> >> killing their monitoring process etc. pp. Not happening, unless someone
> >> with deep expertise in signals can convince me otherwise.
> >
> > If parent namespace procfs FDs or mounts really can leak into child
> > namespaces as easily as Aleksa says, then I don't mind adding the
> > check. I was under the impression that if you find yourself in this
> > situation, you already have a big problem.
>
> There is one big reason to have the check, and I have not seen it
> mentioned yet in this thread.
>
> When SI_USER is set we report the pid of the sender of the signal in
> si_pid. When the signal comes from the kernel si_pid == 0. When signal
> is sent from an ancestor pid namespace si_pid also equals 0 (which is
> reasonable).
>
> A signal out to a process in a parent pid namespace such as SIGCHLD is
> reasonable as we can map the pid. I really don't see the point of
> forbidding that. From the perspective of the process in the parent pid
> namespace it is just another process in it's pid namespace. So it
> should pose no problem from the perspective of the receiving process.
>
> A signal to a process in a pid namespace that is neither a parent nor a
> descendent pid namespace would be a problem, as there is no well defined
> notion of what si_pid should be set to. So for that case perhaps we
> should have something like a noprocess pid that we can set. Perhaps we
> could set si_pid to 0xffffffff. That would take a small extension to
> pid_nr_ns.
>
> File descriptors are not namespaced. It is completely legitimate to use
> file descriptors to get around limitations of namespaces.
Frankly, I don't see a good argument for why we would allow that even if
safe. I have not heard a legitimate use-case or need for this.
At this point I care about very simple semantics. Being able to signal
into ancestor pid namespaces and cousin namespaces is interesting but
makes the syscall more brittle and harder to understand.
Changing pid_nr_ns() might be the solution but this function is called
all over the place in the kernel and I'm not going to risk breaking
something by changing it for a feature that no one so far has ever
asked for.
If you are ok with this then we should hold off on this. We can always
add this feature later by removing the check when someone has a use-case
for it.
I'll send a v2 of the patch that keeps the restriction for now. If you
insist on it being removed we can make the change in a follow-up
iteration.
Christian
>
> Adding limitations to a file descriptor based api because someone else
> can't set up their processes in such a way as to get the restrictions
> they are looking for seems very sad.
>
> Frankly I think it is one of the better features of namespaces that we
> have to carefully handle and define these cases so that when the
> inevitable leaks happen you are not immediately in a world of hurt. All
> of the other permission checks etc continue to do their job. Plus you
> are prepared for the case when someone wants their containers to have an
> interesting communication primitive.
>
> Eric
>
>
>
>
^ permalink raw reply
* [RFC PATCH 0/3] THP eligibility reporting via proc
From: Michal Hocko @ 2018-11-20 10:35 UTC (permalink / raw)
To: linux-api
Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, Dan Williams,
David Rientjes, Jan Kara, Michal Hocko
Hi,
this series of three patches aims at making THP eligibility reporting
much more robust and long term sustainable. The trigger for the change
is a regression report [1] and the long follow up discussion. In short
the specific application didn't have good API to query whether a particular
mapping can be backed by THP so it has used VMA flags to workaround that.
These flags represent a deep internal state of VMAs and as such they should
be used by userspace with a great deal of caution.
A similar has happened for [2] when users complained that VM_MIXEDMAP is
no longer set on DAX mappings. Again a lack of a proper API led to an
abuse.
The first patch in the series tries to emphasise that that the semantic
of flags might change and any application consuming those should be really
careful.
The remaining two patches provide a more suitable interface to address [1]
and provide a consistent API to query the THP status both for each VMA
and process wide as well.
[1] http://lkml.kernel.org/r/http://lkml.kernel.org/r/alpine.DEB.2.21.1809241054050.224429@chino.kir.corp.google.com
[2] http://lkml.kernel.org/r/20181002100531.GC4135@quack2.suse.cz
^ permalink raw reply
* [RFC PATCH 1/3] mm, proc: be more verbose about unstable VMA flags in /proc/<pid>/smaps
From: Michal Hocko @ 2018-11-20 10:35 UTC (permalink / raw)
To: linux-api
Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, Michal Hocko,
Jan Kara, Dan Williams, David Rientjes
In-Reply-To: <20181120103515.25280-1-mhocko@kernel.org>
From: Michal Hocko <mhocko@suse.com>
Even though vma flags exported via /proc/<pid>/smaps are explicitly
documented to be not guaranteed for future compatibility the warning
doesn't go far enough because it doesn't mention semantic changes to
those flags. And they are important as well because these flags are
a deep implementation internal to the MM code and the semantic might
change at any time.
Let's consider two recent examples:
http://lkml.kernel.org/r/20181002100531.GC4135@quack2.suse.cz
: commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
: removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
: mean time certain customer of ours started poking into /proc/<pid>/smaps
: and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
: flags, the application just fails to start complaining that DAX support is
: missing in the kernel.
http://lkml.kernel.org/r/alpine.DEB.2.21.1809241054050.224429@chino.kir.corp.google.com
: Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
: introduced a regression in that userspace cannot always determine the set
: of vmas where thp is ineligible.
: Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
: to determine if a vma is eligible to be backed by hugepages.
: Previous to this commit, prctl(PR_SET_THP_DISABLE, 1) would cause thp to
: be disabled and emit "nh" as a flag for the corresponding vmas as part of
: /proc/pid/smaps. After the commit, thp is disabled by means of an mm
: flag and "nh" is not emitted.
: This causes smaps parsing libraries to assume a vma is eligible for thp
: and ends up puzzling the user on why its memory is not backed by thp.
In both cases userspace was relying on a semantic of a specific VMA
flag. The primary reason why that happened is a lack of a proper
internface. While this has been worked on and it will be fixed properly,
it seems that our wording could see some refinement and be more vocal
about semantic aspect of these flags as well.
Cc: Jan Kara <jack@suse.cz>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Rientjes <rientjes@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Documentation/filesystems/proc.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 12a5e6e693b6..b1fda309f067 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -496,7 +496,9 @@ flags associated with the particular virtual memory area in two letter encoded
Note that there is no guarantee that every flag and associated mnemonic will
be present in all further kernel releases. Things get changed, the flags may
-be vanished or the reverse -- new added.
+be vanished or the reverse -- new added. Interpretatation of their meaning
+might change in future as well. So each consumnent of these flags have to
+follow each specific kernel version for the exact semantic.
This file is only present if the CONFIG_MMU kernel configuration option is
enabled.
--
2.19.1
^ permalink raw reply related
* [RFC PATCH 2/3] mm, thp, proc: report THP eligibility for each vma
From: Michal Hocko @ 2018-11-20 10:35 UTC (permalink / raw)
To: linux-api; +Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, Michal Hocko
In-Reply-To: <20181120103515.25280-1-mhocko@kernel.org>
From: Michal Hocko <mhocko@suse.com>
Userspace falls short when trying to find out whether a specific memory
range is eligible for THP. There are usecases that would like to know
that
http://lkml.kernel.org/r/alpine.DEB.2.21.1809251248450.50347@chino.kir.corp.google.com
: This is used to identify heap mappings that should be able to fault thp
: but do not, and they normally point to a low-on-memory or fragmentation
: issue.
The only way to deduce this now is to query for hg resp. nh flags and
confronting the state with the global setting. Except that there is
also PR_SET_THP_DISABLE that might change the picture. So the final
logic is not trivial. Moreover the eligibility of the vma depends on
the type of VMA as well. In the past we have supported only anononymous
memory VMAs but things have changed and shmem based vmas are supported
as well these days and the query logic gets even more complicated
because the eligibility depends on the mount option and another global
configuration knob.
Simplify the current state and report the THP eligibility in
/proc/<pid>/smaps for each existing vma. Reuse transparent_hugepage_enabled
for this purpose. The original implementation of this function assumes
that the caller knows that the vma itself is supported for THP so make
the core checks into __transparent_hugepage_enabled and use it for
existing callers. __show_smap just use the new transparent_hugepage_enabled
which also checks the vma support status (please note that this one has
to be out of line due to include dependency issues).
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Documentation/filesystems/proc.txt | 3 +++
fs/proc/task_mmu.c | 2 ++
include/linux/huge_mm.h | 13 ++++++++++++-
mm/huge_memory.c | 12 +++++++++++-
mm/memory.c | 4 ++--
5 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index b1fda309f067..06562bab509a 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -425,6 +425,7 @@ SwapPss: 0 kB
KernelPageSize: 4 kB
MMUPageSize: 4 kB
Locked: 0 kB
+THPeligible: 0
VmFlags: rd ex mr mw me dw
the first of these lines shows the same information as is displayed for the
@@ -462,6 +463,8 @@ replaced by copy-on-write) part of the underlying shmem object out on swap.
"SwapPss" shows proportional swap share of this mapping. Unlike "Swap", this
does not take into account swapped out page of underlying shmem objects.
"Locked" indicates whether the mapping is locked in memory or not.
+"THPeligible" indicates whether the mapping is eligible for THP pages - 1 if
+true, 0 otherwise.
"VmFlags" field deserves a separate description. This member represents the kernel
flags associated with the particular virtual memory area in two letter encoded
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 47c3764c469b..c9f160eb9fbc 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -790,6 +790,8 @@ static int show_smap(struct seq_file *m, void *v)
__show_smap(m, &mss);
+ seq_printf(m, "THPeligible: %d\n", transparent_hugepage_enabled(vma));
+
if (arch_pkeys_enabled())
seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
show_smap_vma_flags(m, vma);
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 4663ee96cf59..381e872bfde0 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -93,7 +93,11 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
extern unsigned long transparent_hugepage_flags;
-static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
+/*
+ * to be used on vmas which are known to support THP.
+ * Use transparent_hugepage_enabled otherwise
+ */
+static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
{
if (vma->vm_flags & VM_NOHUGEPAGE)
return false;
@@ -117,6 +121,8 @@ static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
return false;
}
+bool transparent_hugepage_enabled(struct vm_area_struct *vma);
+
#define transparent_hugepage_use_zero_page() \
(transparent_hugepage_flags & \
(1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
@@ -257,6 +263,11 @@ static inline bool thp_migration_supported(void)
#define hpage_nr_pages(x) 1
+static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
+{
+ return false;
+}
+
static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
{
return false;
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 55478ab3c83b..f64733c23067 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -62,6 +62,16 @@ static struct shrinker deferred_split_shrinker;
static atomic_t huge_zero_refcount;
struct page *huge_zero_page __read_mostly;
+bool transparent_hugepage_enabled(struct vm_area_struct *vma)
+{
+ if (vma_is_anonymous(vma))
+ return __transparent_hugepage_enabled(vma);
+ if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma))
+ return __transparent_hugepage_enabled(vma);
+
+ return false;
+}
+
static struct page *get_huge_zero_page(void)
{
struct page *zero_page;
@@ -1303,7 +1313,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
get_page(page);
spin_unlock(vmf->ptl);
alloc:
- if (transparent_hugepage_enabled(vma) &&
+ if (__transparent_hugepage_enabled(vma) &&
!transparent_hugepage_debug_cow()) {
huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
new_page = alloc_pages_vma(huge_gfp, HPAGE_PMD_ORDER, vma,
diff --git a/mm/memory.c b/mm/memory.c
index 4ad2d293ddc2..3c2716ec7fbd 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3830,7 +3830,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
vmf.pud = pud_alloc(mm, p4d, address);
if (!vmf.pud)
return VM_FAULT_OOM;
- if (pud_none(*vmf.pud) && transparent_hugepage_enabled(vma)) {
+ if (pud_none(*vmf.pud) && __transparent_hugepage_enabled(vma)) {
ret = create_huge_pud(&vmf);
if (!(ret & VM_FAULT_FALLBACK))
return ret;
@@ -3856,7 +3856,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
vmf.pmd = pmd_alloc(mm, vmf.pud, address);
if (!vmf.pmd)
return VM_FAULT_OOM;
- if (pmd_none(*vmf.pmd) && transparent_hugepage_enabled(vma)) {
+ if (pmd_none(*vmf.pmd) && __transparent_hugepage_enabled(vma)) {
ret = create_huge_pmd(&vmf);
if (!(ret & VM_FAULT_FALLBACK))
return ret;
--
2.19.1
^ permalink raw reply related
* [RFC PATCH 3/3] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-11-20 10:35 UTC (permalink / raw)
To: linux-api; +Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, Michal Hocko
In-Reply-To: <20181120103515.25280-1-mhocko@kernel.org>
From: Michal Hocko <mhocko@suse.com>
David Rientjes has reported that 1860033237d4 ("mm: make
PR_SET_THP_DISABLE immediately active") has changed the way how
we report THPable VMAs to the userspace. Their monitoring tool is
triggering false alarms on PR_SET_THP_DISABLE tasks because it considers
an insufficient THP usage as a memory fragmentation resp. memory
pressure issue.
Before the said commit each newly created VMA inherited VM_NOHUGEPAGE
flag and that got exposed to the userspace via /proc/<pid>/smaps file.
This implementation had its downsides as explained in the commit message
but it is true that the userspace doesn't have any means to query for
the process wide THP enabled/disabled status.
PR_SET_THP_DISABLE is a process wide flag so it makes a lot of sense
to export in the process wide context rather than per-vma. Introduce
a new field to /proc/<pid>/status which export this status. If
PR_SET_THP_DISABLE is used then it reports false same as when the THP is
not compiled in. It doesn't consider the global THP status because we
already export that information via sysfs
Fixes: 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Documentation/filesystems/proc.txt | 3 +++
fs/proc/array.c | 10 ++++++++++
2 files changed, 13 insertions(+)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 06562bab509a..7995e9322889 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -182,6 +182,7 @@ For example, to get the status information of a process, all you have to do is
VmSwap: 0 kB
HugetlbPages: 0 kB
CoreDumping: 0
+ THP_enabled: 1
Threads: 1
SigQ: 0/28578
SigPnd: 0000000000000000
@@ -256,6 +257,8 @@ Table 1-2: Contents of the status files (as of 4.8)
HugetlbPages size of hugetlb memory portions
CoreDumping process's memory is currently being dumped
(killing the process may lead to a corrupted core)
+ THP_enabled process is allowed to use THP (returns 0 when
+ PR_SET_THP_DISABLE is set on the process
Threads number of threads
SigQ number of signals queued/max. number for queue
SigPnd bitmap of pending signals for the thread
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 0ceb3b6b37e7..9d428d5a0ac8 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -392,6 +392,15 @@ static inline void task_core_dumping(struct seq_file *m, struct mm_struct *mm)
seq_putc(m, '\n');
}
+static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
+{
+ bool thp_enabled = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE);
+
+ if (thp_enabled)
+ thp_enabled = !test_bit(MMF_DISABLE_THP, &mm->flags);
+ seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
+}
+
int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
@@ -406,6 +415,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
if (mm) {
task_mem(m, mm);
task_core_dumping(m, mm);
+ task_thp_status(m, mm);
mmput(mm);
}
task_sig(m, task);
--
2.19.1
^ permalink raw reply related
* [PATCH v2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-20 10:51 UTC (permalink / raw)
To: ebiederm, linux-kernel
Cc: serge, jannh, luto, akpm, oleg, cyphar, viro, linux-fsdevel,
linux-api, dancol, timmurray, linux-man, Christian Brauner,
Kees Cook
The kill() syscall operates on process identifiers. After a process has
exited its pid can be reused by another process. If a caller sends a signal
to a reused pid it will end up signaling the wrong process. This issue has
often surfaced and there has been a push [1] to address this problem.
This patch uses file descriptors from proc/<pid> as stable handles on
struct pid. Even if a pid is recycled the handle will not change. The file
descriptor can be used to send signals to the referenced process.
Thus, the new syscall procfd_signal() is introduced to solve this problem.
It operates on a process file descriptor.
The syscall takes an additional siginfo_t and flags argument. If siginfo_t
is NULL then procfd_signal() behaves like kill() if it is not NULL it
behaves like rt_sigqueueinfo.
The flags argument is added to allow for future extensions of this syscall.
It currently needs to be passed as 0.
With this patch a process can be killed via:
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <signal.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>
int main(int argc, char *argv[])
{
int ret;
char buf[1000];
if (argc < 2)
exit(EXIT_FAILURE);
ret = snprintf(buf, sizeof(buf), "/proc/%s", argv[1]);
if (ret < 0)
exit(EXIT_FAILURE);
int fd = open(buf, O_DIRECTORY | O_CLOEXEC);
if (fd < 0) {
printf("%s - Failed to open \"%s\"\n", strerror(errno), buf);
exit(EXIT_FAILURE);
}
ret = syscall(__NR_procfd_signal, fd, SIGKILL, NULL, 0);
if (ret < 0) {
printf("Failed to send SIGKILL \"%s\"\n", strerror(errno));
close(fd);
exit(EXIT_FAILURE);
}
close(fd);
exit(EXIT_SUCCESS);
}
[1]: https://lkml.org/lkml/2018/11/18/130
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
Changelog:
v2:
- define __NR_procfd_signal in unistd.h
- wire up compat syscall
- s/proc_is_procfd/proc_is_tgid_procfd/g
- provide stubs when CONFIG_PROC_FS=n
- move proc_pid() to linux/proc_fs.h header
- use proc_pid() to grab struct pid from /proc/<pid> fd
v1:
- patch introduced
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 2 +
fs/proc/base.c | 11 ++-
fs/proc/internal.h | 5 -
include/linux/proc_fs.h | 12 +++
include/linux/syscalls.h | 2 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/signal.c | 127 +++++++++++++++++++++++--
8 files changed, 151 insertions(+), 13 deletions(-)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 3cf7b533b3d1..3f27ffd8ae87 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -398,3 +398,4 @@
384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
386 i386 rseq sys_rseq __ia32_sys_rseq
+387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index f0b1709a5ffb..8a30cde82450 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,7 @@
332 common statx __x64_sys_statx
333 common io_pgetevents __x64_sys_io_pgetevents
334 common rseq __x64_sys_rseq
+335 64 procfd_signal __x64_sys_procfd_signal
#
# x32-specific system call numbers start at 512 to avoid cache impact
@@ -386,3 +387,4 @@
545 x32 execveat __x32_compat_sys_execveat/ptregs
546 x32 preadv2 __x32_compat_sys_preadv64v2
547 x32 pwritev2 __x32_compat_sys_pwritev64v2
+548 x32 procfd_signal __x32_compat_sys_procfd_signal
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ce3465479447..771c6bd1cac6 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -716,7 +716,10 @@ static int proc_pid_permission(struct inode *inode, int mask)
return generic_permission(inode, mask);
}
-
+struct pid *proc_pid(const struct inode *inode)
+{
+ return PROC_I(inode)->pid;
+}
static const struct inode_operations proc_def_inode_operations = {
.setattr = proc_setattr,
@@ -3038,6 +3041,12 @@ static const struct file_operations proc_tgid_base_operations = {
.llseek = generic_file_llseek,
};
+bool proc_is_tgid_procfd(const struct file *file)
+{
+ return d_is_dir(file->f_path.dentry) &&
+ (file->f_op == &proc_tgid_base_operations);
+}
+
static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
{
return proc_pident_lookup(dir, dentry,
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 5185d7f6a51e..eb69afba83f3 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -113,11 +113,6 @@ static inline void *__PDE_DATA(const struct inode *inode)
return PDE(inode)->data;
}
-static inline struct pid *proc_pid(const struct inode *inode)
-{
- return PROC_I(inode)->pid;
-}
-
static inline struct task_struct *get_proc_task(const struct inode *inode)
{
return get_pid_task(proc_pid(inode), PIDTYPE_PID);
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d0e1f1522a78..96df2fe6311d 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -73,6 +73,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
int (*show)(struct seq_file *, void *),
proc_write_t write,
void *data);
+extern bool proc_is_tgid_procfd(const struct file *file);
+extern struct pid *proc_pid(const struct inode *inode);
#else /* CONFIG_PROC_FS */
@@ -114,6 +116,16 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
#define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
#define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
+static inline bool proc_is_tgid_procfd(const struct file *file)
+{
+ return false;
+}
+
+static inline struct pid *proc_pid(const struct inode *inode)
+{
+ return NULL;
+}
+
#endif /* CONFIG_PROC_FS */
struct net;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2ac3d13a915b..a5ca8cb84566 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -907,6 +907,8 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
unsigned mask, struct statx __user *buffer);
asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
int flags, uint32_t sig);
+asmlinkage long sys_procfd_signal(int fd, int sig, siginfo_t __user *info,
+ int flags);
/*
* Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 538546edbfbd..4dc81a994ad1 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
__SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
#define __NR_rseq 293
__SYSCALL(__NR_rseq, sys_rseq)
+#define __NR_procfd_signal 294
+__SC_COMP(__NR_procfd_signal, sys_procfd_signal, compat_sys_procfd_signal)
#undef __NR_syscalls
-#define __NR_syscalls 294
+#define __NR_syscalls 295
/*
* 32 bit systems traditionally used different
diff --git a/kernel/signal.c b/kernel/signal.c
index 9a32bc2088c9..13695342f150 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -19,7 +19,9 @@
#include <linux/sched/task.h>
#include <linux/sched/task_stack.h>
#include <linux/sched/cputime.h>
+#include <linux/file.h>
#include <linux/fs.h>
+#include <linux/proc_fs.h>
#include <linux/tty.h>
#include <linux/binfmts.h>
#include <linux/coredump.h>
@@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
}
#endif
+static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
+{
+ clear_siginfo(info);
+ info->si_signo = sig;
+ info->si_errno = 0;
+ info->si_code = SI_USER;
+ info->si_pid = task_tgid_vnr(current);
+ info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
+}
+
/**
* sys_kill - send a signal to a process
* @pid: the PID of the process
@@ -3295,16 +3307,119 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
{
struct kernel_siginfo info;
- clear_siginfo(&info);
- info.si_signo = sig;
- info.si_errno = 0;
- info.si_code = SI_USER;
- info.si_pid = task_tgid_vnr(current);
- info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+ prepare_kill_siginfo(sig, &info);
return kill_something_info(sig, &info, pid);
}
+/*
+ * Verify that the signaler and signalee either are in the same pid namespace
+ * or that the signaler's pid namespace is an ancestor of the signalee's pid
+ * namespace.
+ */
+static bool may_signal_procfd(struct pid *pid)
+{
+ struct pid_namespace *active = task_active_pid_ns(current);
+ struct pid_namespace *p = ns_of_pid(pid);
+
+ for (;;) {
+ if (!p)
+ return false;
+ if (p == active)
+ break;
+ p = p->parent;
+ }
+
+ return true;
+}
+
+static int do_procfd_signal(int fd, int sig, kernel_siginfo_t *kinfo, int flags,
+ bool had_siginfo)
+{
+ int ret;
+ struct fd f;
+ struct pid *pid;
+
+ /* Enforce flags be set to 0 until we add an extension. */
+ if (flags)
+ return -EINVAL;
+
+ f = fdget_raw(fd);
+ if (!f.file)
+ return -EBADF;
+
+ /* Is this a process file descriptor? */
+ ret = -EINVAL;
+ if (!proc_is_tgid_procfd(f.file))
+ goto err;
+
+ /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
+ pid = proc_pid(file_inode(f.file));
+ if (!pid)
+ goto err;
+
+ if (!may_signal_procfd(pid))
+ goto err;
+
+ if (had_siginfo) {
+ /*
+ * Not even root can pretend to send signals from the kernel.
+ * Nor can they impersonate a kill()/tgkill(), which adds
+ * source info.
+ */
+ ret = -EPERM;
+ if ((kinfo->si_code >= 0 || kinfo->si_code == SI_TKILL) &&
+ (task_pid(current) != pid))
+ goto err;
+ } else {
+ prepare_kill_siginfo(sig, kinfo);
+ }
+
+ ret = kill_pid_info(sig, kinfo, pid);
+
+err:
+ fdput(f);
+ return ret;
+}
+
+/**
+ * sys_procfd_signal - send a signal to a process through a process file
+ * descriptor
+ * @fd: the file descriptor of the process
+ * @sig: signal to be sent
+ * @info: the signal info
+ * @flags: future flags to be passed
+ */
+SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
+ int, flags)
+{
+ kernel_siginfo_t kinfo;
+
+ if (info) {
+ int ret = __copy_siginfo_from_user(sig, &kinfo, info);
+ if (unlikely(ret))
+ return ret;
+ }
+
+ return do_procfd_signal(fd, sig, &kinfo, flags, !!info);
+}
+
+#ifdef CONFIG_COMPAT
+COMPAT_SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig,
+ struct compat_siginfo __user *, info, int, flags)
+{
+ kernel_siginfo_t kinfo;
+
+ if (info) {
+ int ret = __copy_siginfo_from_user32(sig, &kinfo, info);
+ if (unlikely(ret))
+ return ret;
+ }
+
+ return do_procfd_signal(fd, sig, &kinfo, flags, !!info);
+}
+#endif
+
static int
do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
{
--
2.19.1
^ permalink raw reply related
* [PATCH v2] procfd_signal.2: document procfd_signal syscall
From: Christian Brauner @ 2018-11-20 10:51 UTC (permalink / raw)
To: ebiederm, linux-kernel
Cc: serge, jannh, luto, akpm, oleg, cyphar, viro, linux-fsdevel,
linux-api, dancol, timmurray, linux-man, Christian Brauner
In-Reply-To: <20181120105124.14733-1-christian@brauner.io>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
man2/procfd_signal.2 | 208 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 208 insertions(+)
create mode 100644 man2/procfd_signal.2
diff --git a/man2/procfd_signal.2 b/man2/procfd_signal.2
new file mode 100644
index 000000000..889bb426f
--- /dev/null
+++ b/man2/procfd_signal.2
@@ -0,0 +1,208 @@
+.\" Copyright (C) 2018 Christian Brauner <christian@brauner.io>
+.\"
+.\" %%%LICENSE_START(VERBATIM)
+.\" Permission is granted to make and distribute verbatim copies of this
+.\" manual provided the copyright notice and this permission notice are
+.\" preserved on all copies.
+.\"
+.\" Permission is granted to copy and distribute modified versions of this
+.\" manual under the conditions for verbatim copying, provided that the
+.\" entire resulting derived work is distributed under the terms of a
+.\" permission notice identical to this one.
+.\"
+.\" Since the Linux kernel and libraries are constantly changing, this
+.\" manual page may be incorrect or out-of-date. The author(s) assume no
+.\" responsibility for errors or omissions, or for damages resulting from
+.\" the use of the information contained herein. The author(s) may not
+.\" have taken the same level of care in the production of this manual,
+.\" which is licensed free of charge, as they might when working
+.\" professionally.
+.\"
+.\" Formatted or processed versions of this manual, if unaccompanied by
+.\" the source, must acknowledge the copyright and authors of this work.
+.\" %%%LICENSE_END
+.\"
+.TH PROCFD_SIGNAL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
+.SH NAME
+procfd_signal \- send signal to a process through a process descriptor
+.SH SYNOPSIS
+.nf
+.B #include <sys/types.h>
+.B #include <signal.h>
+.PP
+.BI "int procfd_signal(int " fd ", int " sig ", siginfo_t *" info ", int " flags );
+.fi
+.SH DESCRIPTION
+.BR procfd_signal ()
+sends the signal specified in
+.I sig
+to the process identified by the file descriptor
+.IR fd .
+The permissions required to send a signal are the same as for
+.BR kill (2).
+As with
+.BR kill (2),
+the null signal (0) can be used to check if a process with a given
+PID exists.
+.PP
+The optional
+.I info
+argument specifies the data to accompany the signal.
+This argument is a pointer to a structure of type
+.IR siginfo_t ,
+described in
+.BR sigaction (2)
+(and defined by including
+.IR <sigaction.h> ).
+The caller should set the following fields in this structure:
+.TP
+.I si_code
+This must be one of the
+.B SI_*
+codes in the Linux kernel source file
+.IR include/asm-generic/siginfo.h ,
+with the restriction that the code must be negative
+(i.e., cannot be
+.BR SI_USER ,
+which is used by the kernel to indicate a signal sent by
+.BR kill (2))
+and cannot (since Linux 2.6.39) be
+.BR SI_TKILL
+(which is used by the kernel to indicate a signal sent using
+.\" tkill(2) or
+.BR tgkill (2)).
+.TP
+.I si_pid
+This should be set to a process ID,
+typically the process ID of the sender.
+.TP
+.I si_uid
+This should be set to a user ID,
+typically the real user ID of the sender.
+.TP
+.I si_value
+This field contains the user data to accompany the signal.
+For more information, see the description of the last
+.RI ( "union sigval" )
+argument of
+.BR sigqueue (3).
+.PP
+Internally, the kernel sets the
+.I si_signo
+field to the value specified in
+.IR sig ,
+so that the receiver of the signal can also obtain
+the signal number via that field.
+.PP
+The
+.I flags
+argument is reserved for future extension and must be set to 0.
+.PP
+.SH RETURN VALUE
+On success, this system call returns 0.
+On error, it returns \-1 and
+.I errno
+is set to indicate the error.
+.SH ERRORS
+.TP
+.B EBADF
+.I fd
+is not a valid file descriptor.
+.TP
+.B EINVAL
+An invalid signal was specified.
+.TP
+.B EINVAL
+.I fd
+does not refer to a process.
+.TP
+.B EINVAL
+The flags argument was not 0.
+.TP
+.B EINVAL
+The calling process and the target process are neither located
+in the same pid namespace nor is the calling process's
+pid namespace an ancestor of the target process's pid namespaces
+(see
+.BR pid_namespaces (7)).
+.TP
+.B EPERM
+The caller does not have permission to send the signal to the target.
+For the required permissions, see
+.BR kill (2).
+Or:
+.I uinfo->si_code
+is invalid.
+.TP
+.B ESRCH
+The process or process group does not exist.
+Note that an existing process might be a zombie,
+a process that has terminated execution, but
+has not yet been
+.BR wait (2)ed
+for.
+.SH CONFORMING TO
+This system call is Linux-specific.
+.SH NOTES
+The
+.BR procfd_signal ()
+system call will only send signals to processes which either are in
+the same pid namespace or are in a descendant pid namespace of
+the caller.
+.SH EXAMPLE
+The following program demonstrates how to kill a process with
+.BR procfd_signal ()
+.SS Program source
+.EX
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <signal.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+int
+main(int argc, char *argv[])
+{
+ int ret;
+ char buf[1000];
+
+ if (argc < 2)
+ exit(EXIT_FAILURE);
+
+ ret = snprintf(buf, sizeof(buf), "/proc/%s", argv[1]);
+ if (ret < 0)
+ exit(EXIT_FAILURE);
+
+ int fd = open(buf, O_DIRECTORY | O_CLOEXEC);
+ if (fd < 0) {
+ printf("%s - Failed to open \\"%s\\"\\n", strerror(errno), buf);
+ exit(EXIT_FAILURE);
+ }
+
+ ret = procfd_signal(fd, SIGKILL, NULL, 0);
+ if (ret < 0) {
+ printf("Failed to send SIGKILL \\"%s\\"\\n", strerror(errno));
+ close(fd);
+ exit(EXIT_FAILURE);
+ }
+
+ close(fd);
+
+ exit(EXIT_SUCCESS);
+}
+.EE
+.SH SEE ALSO
+.BR kill (2),
+.BR sigaction (2),
+.BR sigprocmask (2),
+.BR tgkill (2),
+.BR pthread_sigqueue (3),
+.BR rt_sigqueueinfo (2),
+.BR sigqueue (3),
+.BR signal (7)
--
2.19.1
^ permalink raw reply related
* Re: [RFC PATCH 1/3] mm, proc: be more verbose about unstable VMA flags in /proc/<pid>/smaps
From: Jan Kara @ 2018-11-20 10:51 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-api, Andrew Morton, Alexey Dobriyan, linux-mm, LKML,
Michal Hocko, Jan Kara, Dan Williams, David Rientjes
In-Reply-To: <20181120103515.25280-2-mhocko@kernel.org>
On Tue 20-11-18 11:35:13, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Even though vma flags exported via /proc/<pid>/smaps are explicitly
> documented to be not guaranteed for future compatibility the warning
> doesn't go far enough because it doesn't mention semantic changes to
> those flags. And they are important as well because these flags are
> a deep implementation internal to the MM code and the semantic might
> change at any time.
>
> Let's consider two recent examples:
> http://lkml.kernel.org/r/20181002100531.GC4135@quack2.suse.cz
> : commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
> : removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
> : mean time certain customer of ours started poking into /proc/<pid>/smaps
> : and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
> : flags, the application just fails to start complaining that DAX support is
> : missing in the kernel.
>
> http://lkml.kernel.org/r/alpine.DEB.2.21.1809241054050.224429@chino.kir.corp.google.com
> : Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> : introduced a regression in that userspace cannot always determine the set
> : of vmas where thp is ineligible.
> : Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
> : to determine if a vma is eligible to be backed by hugepages.
> : Previous to this commit, prctl(PR_SET_THP_DISABLE, 1) would cause thp to
> : be disabled and emit "nh" as a flag for the corresponding vmas as part of
> : /proc/pid/smaps. After the commit, thp is disabled by means of an mm
> : flag and "nh" is not emitted.
> : This causes smaps parsing libraries to assume a vma is eligible for thp
> : and ends up puzzling the user on why its memory is not backed by thp.
>
> In both cases userspace was relying on a semantic of a specific VMA
> flag. The primary reason why that happened is a lack of a proper
> internface. While this has been worked on and it will be fixed properly,
> it seems that our wording could see some refinement and be more vocal
> about semantic aspect of these flags as well.
>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Rientjes <rientjes@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Honestly, it just shows that no amount of documentation is going to stop
userspace from abusing API that's exposing too much if there's no better
alternative. But this is a good clarification regardless. So feel free to
add:
Acked-by: Jan Kara <jack@suse.cz>
Honza
> ---
> Documentation/filesystems/proc.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index 12a5e6e693b6..b1fda309f067 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -496,7 +496,9 @@ flags associated with the particular virtual memory area in two letter encoded
>
> Note that there is no guarantee that every flag and associated mnemonic will
> be present in all further kernel releases. Things get changed, the flags may
> -be vanished or the reverse -- new added.
> +be vanished or the reverse -- new added. Interpretatation of their meaning
> +might change in future as well. So each consumnent of these flags have to
> +follow each specific kernel version for the exact semantic.
>
> This file is only present if the CONFIG_MMU kernel configuration option is
> enabled.
> --
> 2.19.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size
From: Lorenz Bauer @ 2018-11-20 11:35 UTC (permalink / raw)
To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAH3MdRVd=ipYg+b9sGRR-FFUuOMjZ4OGZzkmJuFEShrYVXvwWA@mail.gmail.com>
On Sun, 18 Nov 2018 at 05:59, Y Song <ys114321@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 12:55 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Make sure that bpf_prog_test_run returns the correct length
> > in the size_out argument and that the kernel respects the
> > output size hint.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> > tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 560d7527b86b..6ab98e10e86f 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -124,6 +124,39 @@ static void test_pkt_access(void)
> > bpf_object__close(obj);
> > }
> >
> > +static void test_output_size_hint(void)
> > +{
> > + const char *file = "./test_pkt_access.o";
> > + struct bpf_object *obj;
> > + __u32 retval, size, duration;
> > + int err, prog_fd;
> > + char buf[10];
> > +
> > + err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > + if (err) {
> > + error_cnt++;
> > + return;
> > + }
> CHECK can also be used here.
> if (CHECK(...)) {
> goto done;
> }
> where label "done" is right before bpf_object__close.
I just copied this part from test_pkt_access, happy to change it though.
However, I think "goto done" would lead to freeing an unallocated
object in this case?
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [RFC PATCH 1/3] mm, proc: be more verbose about unstable VMA flags in /proc/<pid>/smaps
From: Michal Hocko @ 2018-11-20 11:41 UTC (permalink / raw)
To: Jan Kara
Cc: linux-api, Andrew Morton, Alexey Dobriyan, linux-mm, LKML,
Dan Williams, David Rientjes
In-Reply-To: <20181120105135.GF8842@quack2.suse.cz>
On Tue 20-11-18 11:51:35, Jan Kara wrote:
> Honestly, it just shows that no amount of documentation is going to stop
> userspace from abusing API that's exposing too much if there's no better
> alternative.
Yeah, I agree. And we should never expose such a low level stuff in the
first place. But, well, this ship has already sailed...
> But this is a good clarification regardless. So feel free to
> add:
>
> Acked-by: Jan Kara <jack@suse.cz>
Thanks!
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH 2/3] mm, thp, proc: report THP eligibility for each vma
From: Michal Hocko @ 2018-11-20 11:42 UTC (permalink / raw)
To: linux-api; +Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, David Rientjes
In-Reply-To: <20181120103515.25280-3-mhocko@kernel.org>
Damn, David somehow didn't make it to the CC list. Sorry about that.
On Tue 20-11-18 11:35:14, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Userspace falls short when trying to find out whether a specific memory
> range is eligible for THP. There are usecases that would like to know
> that
> http://lkml.kernel.org/r/alpine.DEB.2.21.1809251248450.50347@chino.kir.corp.google.com
> : This is used to identify heap mappings that should be able to fault thp
> : but do not, and they normally point to a low-on-memory or fragmentation
> : issue.
>
> The only way to deduce this now is to query for hg resp. nh flags and
> confronting the state with the global setting. Except that there is
> also PR_SET_THP_DISABLE that might change the picture. So the final
> logic is not trivial. Moreover the eligibility of the vma depends on
> the type of VMA as well. In the past we have supported only anononymous
> memory VMAs but things have changed and shmem based vmas are supported
> as well these days and the query logic gets even more complicated
> because the eligibility depends on the mount option and another global
> configuration knob.
>
> Simplify the current state and report the THP eligibility in
> /proc/<pid>/smaps for each existing vma. Reuse transparent_hugepage_enabled
> for this purpose. The original implementation of this function assumes
> that the caller knows that the vma itself is supported for THP so make
> the core checks into __transparent_hugepage_enabled and use it for
> existing callers. __show_smap just use the new transparent_hugepage_enabled
> which also checks the vma support status (please note that this one has
> to be out of line due to include dependency issues).
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> Documentation/filesystems/proc.txt | 3 +++
> fs/proc/task_mmu.c | 2 ++
> include/linux/huge_mm.h | 13 ++++++++++++-
> mm/huge_memory.c | 12 +++++++++++-
> mm/memory.c | 4 ++--
> 5 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index b1fda309f067..06562bab509a 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -425,6 +425,7 @@ SwapPss: 0 kB
> KernelPageSize: 4 kB
> MMUPageSize: 4 kB
> Locked: 0 kB
> +THPeligible: 0
> VmFlags: rd ex mr mw me dw
>
> the first of these lines shows the same information as is displayed for the
> @@ -462,6 +463,8 @@ replaced by copy-on-write) part of the underlying shmem object out on swap.
> "SwapPss" shows proportional swap share of this mapping. Unlike "Swap", this
> does not take into account swapped out page of underlying shmem objects.
> "Locked" indicates whether the mapping is locked in memory or not.
> +"THPeligible" indicates whether the mapping is eligible for THP pages - 1 if
> +true, 0 otherwise.
>
> "VmFlags" field deserves a separate description. This member represents the kernel
> flags associated with the particular virtual memory area in two letter encoded
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 47c3764c469b..c9f160eb9fbc 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -790,6 +790,8 @@ static int show_smap(struct seq_file *m, void *v)
>
> __show_smap(m, &mss);
>
> + seq_printf(m, "THPeligible: %d\n", transparent_hugepage_enabled(vma));
> +
> if (arch_pkeys_enabled())
> seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
> show_smap_vma_flags(m, vma);
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 4663ee96cf59..381e872bfde0 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -93,7 +93,11 @@ extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>
> extern unsigned long transparent_hugepage_flags;
>
> -static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> +/*
> + * to be used on vmas which are known to support THP.
> + * Use transparent_hugepage_enabled otherwise
> + */
> +static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
> {
> if (vma->vm_flags & VM_NOHUGEPAGE)
> return false;
> @@ -117,6 +121,8 @@ static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> return false;
> }
>
> +bool transparent_hugepage_enabled(struct vm_area_struct *vma);
> +
> #define transparent_hugepage_use_zero_page() \
> (transparent_hugepage_flags & \
> (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
> @@ -257,6 +263,11 @@ static inline bool thp_migration_supported(void)
>
> #define hpage_nr_pages(x) 1
>
> +static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
> +{
> + return false;
> +}
> +
> static inline bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> {
> return false;
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 55478ab3c83b..f64733c23067 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -62,6 +62,16 @@ static struct shrinker deferred_split_shrinker;
> static atomic_t huge_zero_refcount;
> struct page *huge_zero_page __read_mostly;
>
> +bool transparent_hugepage_enabled(struct vm_area_struct *vma)
> +{
> + if (vma_is_anonymous(vma))
> + return __transparent_hugepage_enabled(vma);
> + if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma))
> + return __transparent_hugepage_enabled(vma);
> +
> + return false;
> +}
> +
> static struct page *get_huge_zero_page(void)
> {
> struct page *zero_page;
> @@ -1303,7 +1313,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
> get_page(page);
> spin_unlock(vmf->ptl);
> alloc:
> - if (transparent_hugepage_enabled(vma) &&
> + if (__transparent_hugepage_enabled(vma) &&
> !transparent_hugepage_debug_cow()) {
> huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
> new_page = alloc_pages_vma(huge_gfp, HPAGE_PMD_ORDER, vma,
> diff --git a/mm/memory.c b/mm/memory.c
> index 4ad2d293ddc2..3c2716ec7fbd 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3830,7 +3830,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
> vmf.pud = pud_alloc(mm, p4d, address);
> if (!vmf.pud)
> return VM_FAULT_OOM;
> - if (pud_none(*vmf.pud) && transparent_hugepage_enabled(vma)) {
> + if (pud_none(*vmf.pud) && __transparent_hugepage_enabled(vma)) {
> ret = create_huge_pud(&vmf);
> if (!(ret & VM_FAULT_FALLBACK))
> return ret;
> @@ -3856,7 +3856,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
> vmf.pmd = pmd_alloc(mm, vmf.pud, address);
> if (!vmf.pmd)
> return VM_FAULT_OOM;
> - if (pmd_none(*vmf.pmd) && transparent_hugepage_enabled(vma)) {
> + if (pmd_none(*vmf.pmd) && __transparent_hugepage_enabled(vma)) {
> ret = create_huge_pmd(&vmf);
> if (!(ret & VM_FAULT_FALLBACK))
> return ret;
> --
> 2.19.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH 3/3] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-11-20 11:42 UTC (permalink / raw)
To: linux-api; +Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, David Rientjes
In-Reply-To: <20181120103515.25280-4-mhocko@kernel.org>
Damn, David somehow didn't make it to the CC list. Sorry about that.
On Tue 20-11-18 11:35:15, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> David Rientjes has reported that 1860033237d4 ("mm: make
> PR_SET_THP_DISABLE immediately active") has changed the way how
> we report THPable VMAs to the userspace. Their monitoring tool is
> triggering false alarms on PR_SET_THP_DISABLE tasks because it considers
> an insufficient THP usage as a memory fragmentation resp. memory
> pressure issue.
>
> Before the said commit each newly created VMA inherited VM_NOHUGEPAGE
> flag and that got exposed to the userspace via /proc/<pid>/smaps file.
> This implementation had its downsides as explained in the commit message
> but it is true that the userspace doesn't have any means to query for
> the process wide THP enabled/disabled status.
>
> PR_SET_THP_DISABLE is a process wide flag so it makes a lot of sense
> to export in the process wide context rather than per-vma. Introduce
> a new field to /proc/<pid>/status which export this status. If
> PR_SET_THP_DISABLE is used then it reports false same as when the THP is
> not compiled in. It doesn't consider the global THP status because we
> already export that information via sysfs
>
> Fixes: 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> Documentation/filesystems/proc.txt | 3 +++
> fs/proc/array.c | 10 ++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index 06562bab509a..7995e9322889 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -182,6 +182,7 @@ For example, to get the status information of a process, all you have to do is
> VmSwap: 0 kB
> HugetlbPages: 0 kB
> CoreDumping: 0
> + THP_enabled: 1
> Threads: 1
> SigQ: 0/28578
> SigPnd: 0000000000000000
> @@ -256,6 +257,8 @@ Table 1-2: Contents of the status files (as of 4.8)
> HugetlbPages size of hugetlb memory portions
> CoreDumping process's memory is currently being dumped
> (killing the process may lead to a corrupted core)
> + THP_enabled process is allowed to use THP (returns 0 when
> + PR_SET_THP_DISABLE is set on the process
> Threads number of threads
> SigQ number of signals queued/max. number for queue
> SigPnd bitmap of pending signals for the thread
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 0ceb3b6b37e7..9d428d5a0ac8 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -392,6 +392,15 @@ static inline void task_core_dumping(struct seq_file *m, struct mm_struct *mm)
> seq_putc(m, '\n');
> }
>
> +static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
> +{
> + bool thp_enabled = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE);
> +
> + if (thp_enabled)
> + thp_enabled = !test_bit(MMF_DISABLE_THP, &mm->flags);
> + seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
> +}
> +
> int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
> struct pid *pid, struct task_struct *task)
> {
> @@ -406,6 +415,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
> if (mm) {
> task_mem(m, mm);
> task_core_dumping(m, mm);
> + task_thp_status(m, mm);
> mmput(mm);
> }
> task_sig(m, task);
> --
> 2.19.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] procfd_signal.2: document procfd_signal syscall
From: Michael Kerrisk (man-pages) @ 2018-11-20 13:29 UTC (permalink / raw)
To: Christian Brauner, ebiederm, linux-kernel
Cc: mtk.manpages, serge, jannh, luto, akpm, oleg, cyphar, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man
In-Reply-To: <20181119103241.5229-4-christian@brauner.io>
Hello Christian,
On 11/19/18 11:32 AM, Christian Brauner wrote:
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> Changelog:
> v1:
> - patch introduced
> ---
> man2/procfd_signal.2 | 147 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 147 insertions(+)
> create mode 100644 man2/procfd_signal.2
>
> diff --git a/man2/procfd_signal.2 b/man2/procfd_signal.2
> new file mode 100644
> index 000000000..6af0b74f4
> --- /dev/null
> +++ b/man2/procfd_signal.2
> @@ -0,0 +1,147 @@
> +.\" Copyright (C) 2018 Christian Brauner <christian@brauner.io>
> +.\"
> +.\" %%%LICENSE_START(VERBATIM)
> +.\" Permission is granted to make and distribute verbatim copies of this
> +.\" manual provided the copyright notice and this permission notice are
> +.\" preserved on all copies.
> +.\"
> +.\" Permission is granted to copy and distribute modified versions of this
> +.\" manual under the conditions for verbatim copying, provided that the
> +.\" entire resulting derived work is distributed under the terms of a
> +.\" permission notice identical to this one.
> +.\"
> +.\" Since the Linux kernel and libraries are constantly changing, this
> +.\" manual page may be incorrect or out-of-date. The author(s) assume no
> +.\" responsibility for errors or omissions, or for damages resulting from
> +.\" the use of the information contained herein. The author(s) may not
> +.\" have taken the same level of care in the production of this manual,
> +.\" which is licensed free of charge, as they might when working
> +.\" professionally.
> +.\"
> +.\" Formatted or processed versions of this manual, if unaccompanied by
> +.\" the source, must acknowledge the copyright and authors of this work.
> +.\" %%%LICENSE_END
> +.\"
> +.TH PROCFD_SIGNAL 2 2017-09-15 "Linux" "Linux Programmer's Manual"
Bad timestamp :-)
> +.SH NAME
> +procfd_signal \- send signal to a process through a process descriptor
s/through/via/
> +.SH SYNOPSIS
> +.nf
> +.B #include <sys/types.h>
> +.B #include <signal.h>
> +.PP
> +.BI "int procfd_signal(int " fd ", int " sig ", siginfo_t *" info ", int " flags );
> +.fi
> +.SH DESCRIPTION
> +.BR procfd_signal ()
> +sends the signal specified in
> +.I sig
> +to the process identified by the file descriptor
> +.IR fd .
Here, I think we need some words about how one obtains that
file descriptor.
> +The permissions required to send a signal are the same as for
> +.BR kill (2).
> +As with
> +.BR kill (2),
> +the null signal (0) can be used to check if a process with a given
> +PID exists.
But there is no PID mentioned on this page?
I suppose:
As with
.BR kill (2),
the null signal (0) can be used to check if the process referred to by
.I fd
exists.
?
> +.PP
> +The optional
> +.I info
> +argument specifies the data to accompany the signal.
> +This argument is a pointer to a structure of type
> +.IR siginfo_t ,
> +described in
> +.BR sigaction (2)
> +(and defined by including
> +.IR <sigaction.h> ).
> +The caller should set the following fields in this structure:
> +.TP
> +.I si_code
> +This must be one of the
> +.B SI_*
> +codes in the Linux kernel source file
> +.IR include/asm-generic/siginfo.h ,
> +with the restriction that the code must be negative
> +(i.e., cannot be
> +.BR SI_USER ,
> +which is used by the kernel to indicate a signal sent by
> +.BR kill (2))
> +and cannot (since Linux 2.6.39) be
> +.BR SI_TKILL
> +(which is used by the kernel to indicate a signal sent using
> +.\" tkill(2) or
> +.BR tgkill (2)).
> +.TP
> +.I si_pid
> +This should be set to a process ID,
> +typically the process ID of the sender.
> +.TP
> +.I si_uid
> +This should be set to a user ID,
> +typically the real user ID of the sender.
> +.TP
> +.I si_value
> +This field contains the user data to accompany the signal.
> +For more information, see the description of the last
> +.RI ( "union sigval" )
> +argument of
> +.BR sigqueue (3).
With sigqueue(3), one sends only a signal plus accompanying
data. It is of course based on the lower level rt_sigqueueinfo(2).
The man page for that system call says:
These system calls are not intended for direct application use;
they are provided to allow the implementation of sigqueue(3) and
pthread_sigqueue(3).
Is procfd_signal() intended to be the API directly used from
user space? If it is, then I think there should be some
explanation of why there is a 'siginfo_t' argument (vis-à-vis
sigqueue(3), which makes do with just union sigval).
If procfd_signal() is not intended to be the API used directly
from user space, then I think there needs to be a paragraph
similar to the one in the rt_sigqueueinfo(2) page queoted above.
> +.PP
> +Internally, the kernel sets the
> +.I si_signo
> +field to the value specified in
> +.IR sig ,
> +so that the receiver of the signal can also obtain
> +the signal number via that field.
> +.PP
> +The
> +.I flags
> +argument is reserved for future extension and must be set to 0.
> +.PP
> +.SH RETURN VALUE
> +On success, this system call returns 0.
> +On error, it returns \-1 and
> +.I errno
> +is set to indicate the error.
> +.SH ERRORS
> +.TP
> +.B EBADF
> +.I fd
> +is not a valid file descriptor.
> +.TP
> +.B EINVAL
> +An invalid signal was specified.
> +.TP
> +.B EINVAL
> +.I fd
> +does not refer to a process.
> +.TP
> +.B EINVAL
> +The flags argument was not 0.
> +.TP
> +.B EPERM
> +The caller does not have permission to send the signal to the target.
> +For the required permissions, see
> +.BR kill (2).
> +Or:
> +.I uinfo->si_code
> +is invalid.
> +.TP
> +.B ESRCH
> +The process or process group does not exist.
"or process group"? I suspect a cut and paste error here :-)
The connection between the preceding sentence and the one
that follows it is not quite clear:
> +Note that an existing process might be a zombie,
> +a process that has terminated execution, but
> +has not yet been
> +.BR wait (2)ed
> +for.
Is it the case that using procfd_signal() with a file
descriptor that refers to a zombie will yield EINVAL?
If yes, this could be made clearer with the following:
.B ESRCH
The specified process no longer exists or is a process in the
zombie state (a process that has terminated execution, but
has not yet been
BR wait (2)ed
for).
> +.SH CONFORMING TO
> +This system call is Linux-specific.
> +.SH SEE ALSO
> +.BR kill (2),
> +.BR sigaction (2),
> +.BR sigprocmask (2),
> +.BR tgkill (2),
> +.BR pthread_sigqueue (3),
> +.BR rt_sigqueueinfo (2),
> +.BR sigqueue (3),
> +.BR signal (7)
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH -next 1/2] mm/memfd: make F_SEAL_FUTURE_WRITE seal more robust
From: Andy Lutomirski @ 2018-11-20 15:13 UTC (permalink / raw)
To: Joel Fernandes
Cc: LKML, Andrew Lutomirski, Andrew Morton, Hugh Dickins, Jann Horn,
Khalid Aziz, Linux API, open list:KERNEL SELFTEST FRAMEWORK,
Linux-MM, marcandre.lureau, Matthew Wilcox, Mike Kravetz,
Shuah Khan, Stephen Rothwell
In-Reply-To: <20181120052137.74317-1-joel@joelfernandes.org>
On Mon, Nov 19, 2018 at 9:21 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> A better way to do F_SEAL_FUTURE_WRITE seal was discussed [1] last week
> where we don't need to modify core VFS structures to get the same
> behavior of the seal. This solves several side-effects pointed out by
> Andy [2].
>
> [1] https://lore.kernel.org/lkml/20181111173650.GA256781@google.com/
> [2] https://lore.kernel.org/lkml/69CE06CC-E47C-4992-848A-66EB23EE6C74@amacapital.net/
>
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Fixes: 5e653c2923fd ("mm: Add an F_SEAL_FUTURE_WRITE seal to memfd")
What tree is that commit in? Can we not just fold this in?
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-20 15:19 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andrew Lutomirski, Dave Hansen, Christopherson, Sean J,
Jethro Beekman, Florian Weimer, Linux API, Jann Horn,
Linus Torvalds, X86 ML, linux-arch, LKML, Peter Zijlstra,
Rich Felker, nhorman, npmccallum, Ayoun, Serge, shay.katz-zamir,
linux-sgx, Andy Shevchenko, Thomas Gleixner, Ingo Molnar
In-Reply-To: <20181120101133.GA7319@linux.intel.com>
On Tue, Nov 20, 2018 at 2:11 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Nov 19, 2018 at 09:00:08AM -0800, Andy Lutomirski wrote:
> > On Mon, Nov 19, 2018 at 8:02 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, Nov 19, 2018 at 07:29:36AM -0800, Andy Lutomirski wrote:
> > > > 1. The kernel needs some way to know *when* to apply this fixup.
> > > > Decoding the instruction stream and doing it to all exceptions that
> > > > hit an ENCLU instruction seems like a poor design.
> > >
> > > I'm not sure why you would ever need to do any type of fixup as the idea
> > > is to just return to AEP i.e. from chosen exceptions (EPCM, #UD) the AEP
> > > would work the same way as for exceptions that the kernel can deal with
> > > except filling the exception information to registers.
> >
> > Sure, but how does the kernel know when to do that and when to send a
> > signal? I don't really like decoding the instruction stream to figure
> > it out.
>
> Hmm... why you have to decode instruction stream to find that out? Would
> just depend on exception type (#GP with EPCM, #UD).
What is "#GP with EPCM"? We certainly don't want to react to #UD in
general by mucking with some regs and retrying -- that will infinite
loop and confuse everyone. I'm not even 100% convinced that decoding
the insn stream is useful -- AEP can point to something that isn't
ENCLU.
IOW the kernel needs to know *when* to apply this special behavior.
Sadly there is no bit in the exception frame that says "came from
SGX".
^ permalink raw reply
* [PATCH v2 0/4] Fix unsafe BPF_PROG_TEST_RUN interface
From: Lorenz Bauer @ 2018-11-20 15:43 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.
Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out
Lorenz Bauer (4):
bpf: respect size hint to BPF_PROG_TEST_RUN if present
tools: sync uapi/linux/bpf.h
libbpf: require size hint in bpf_prog_test_run
selftests: add a test for bpf_prog_test_run output size
include/uapi/linux/bpf.h | 7 ++--
net/bpf/test_run.c | 15 ++++++--
tools/include/uapi/linux/bpf.h | 7 ++--
tools/lib/bpf/bpf.c | 7 +++-
tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
5 files changed, 73 insertions(+), 7 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH v2 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present
From: Lorenz Bauer @ 2018-11-20 15:43 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181120154306.16657-1-lmb@cloudflare.com>
Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 7 +++++--
net/bpf/test_run.c | 15 +++++++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
{
void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
int err = -EFAULT;
+ u32 copy_size = size;
- if (data_out && copy_to_user(data_out, data, size))
+ /* Clamp copy if the user has provided a size hint, but copy the full
+ * buffer if not to retain old behaviour.
+ */
+ if (kattr->test.data_size_out &&
+ copy_size > kattr->test.data_size_out) {
+ copy_size = kattr->test.data_size_out;
+ err = -ENOSPC;
+ }
+
+ if (data_out && copy_to_user(data_out, data, copy_size))
goto out;
if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
goto out;
if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
goto out;
- err = 0;
+ if (err != -ENOSPC)
+ err = 0;
out:
return err;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/4] tools: sync uapi/linux/bpf.h
From: Lorenz Bauer @ 2018-11-20 15:43 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181120154306.16657-1-lmb@cloudflare.com>
Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 3/4] libbpf: require size hint in bpf_prog_test_run
From: Lorenz Bauer @ 2018-11-20 15:43 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181120154306.16657-1-lmb@cloudflare.com>
Require size_out to be non-NULL if data_out is given. This prevents
accidental overwriting of process memory after the output buffer.
Adjust callers of bpf_prog_test_run to this behaviour.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/lib/bpf/bpf.c | 7 ++++++-
tools/testing/selftests/bpf/test_progs.c | 10 ++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 961e1b9fc592..1a835ff27486 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -407,15 +407,20 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
union bpf_attr attr;
int ret;
+ if (data_out && !size_out)
+ return -EINVAL;
+
bzero(&attr, sizeof(attr));
attr.test.prog_fd = prog_fd;
attr.test.data_in = ptr_to_u64(data);
attr.test.data_out = ptr_to_u64(data_out);
attr.test.data_size_in = size;
+ if (data_out)
+ attr.test.data_size_out = *size_out;
attr.test.repeat = repeat;
ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
- if (size_out)
+ if (data_out)
*size_out = attr.test.data_size_out;
if (retval)
*retval = attr.test.retval;
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..299938603cb6 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -150,6 +150,7 @@ static void test_xdp(void)
bpf_map_update_elem(map_fd, &key4, &value4, 0);
bpf_map_update_elem(map_fd, &key6, &value6, 0);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
buf, &size, &retval, &duration);
@@ -158,6 +159,7 @@ static void test_xdp(void)
"err %d errno %d retval %d size %d\n",
err, errno, retval, size);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
buf, &size, &retval, &duration);
CHECK(err || retval != XDP_TX || size != 114 ||
@@ -182,6 +184,7 @@ static void test_xdp_adjust_tail(void)
return;
}
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
buf, &size, &retval, &duration);
@@ -189,6 +192,7 @@ static void test_xdp_adjust_tail(void)
"ipv4", "err %d errno %d retval %d size %d\n",
err, errno, retval, size);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, 1, &pkt_v6, sizeof(pkt_v6),
buf, &size, &retval, &duration);
CHECK(err || retval != XDP_TX || size != 54,
@@ -252,6 +256,7 @@ static void test_l4lb(const char *file)
goto out;
bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
buf, &size, &retval, &duration);
CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 54 ||
@@ -259,6 +264,7 @@ static void test_l4lb(const char *file)
"err %d errno %d retval %d size %d magic %x\n",
err, errno, retval, size, *magic);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
buf, &size, &retval, &duration);
CHECK(err || retval != 7/*TC_ACT_REDIRECT*/ || size != 74 ||
@@ -341,6 +347,7 @@ static void test_xdp_noinline(void)
goto out;
bpf_map_update_elem(map_fd, &real_num, &real_def, 0);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
buf, &size, &retval, &duration);
CHECK(err || retval != 1 || size != 54 ||
@@ -348,6 +355,7 @@ static void test_xdp_noinline(void)
"err %d errno %d retval %d size %d magic %x\n",
err, errno, retval, size, *magic);
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v6, sizeof(pkt_v6),
buf, &size, &retval, &duration);
CHECK(err || retval != 1 || size != 74 ||
@@ -1795,6 +1803,7 @@ static void test_queue_stack_map(int type)
pkt_v4.iph.saddr = vals[MAP_SIZE - 1 - i] * 5;
}
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
buf, &size, &retval, &duration);
if (err || retval || size != sizeof(pkt_v4) ||
@@ -1808,6 +1817,7 @@ static void test_queue_stack_map(int type)
err, errno, retval, size, iph->daddr);
/* Queue is empty, program should return TC_ACT_SHOT */
+ size = sizeof(buf);
err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
buf, &size, &retval, &duration);
CHECK(err || retval != 2 /* TC_ACT_SHOT */|| size != sizeof(pkt_v4),
--
2.17.1
^ permalink raw reply related
* [PATCH v2 4/4] selftests: add a test for bpf_prog_test_run output size
From: Lorenz Bauer @ 2018-11-20 15:43 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181120154306.16657-1-lmb@cloudflare.com>
Make sure that bpf_prog_test_run returns the correct length
in the size_out argument and that the kernel respects the
output size hint. Also check that errno indicates ENOSPC.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 299938603cb6..92e48c2ba2c6 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -124,6 +124,39 @@ static void test_pkt_access(void)
bpf_object__close(obj);
}
+static void test_output_size_hint(void)
+{
+ const char *file = "./test_pkt_access.o";
+ struct bpf_object *obj;
+ __u32 retval, size, duration;
+ int err, prog_fd;
+ char buf[10];
+
+ err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+ if (CHECK(err, "load", "err %d errno %d\n", err, errno))
+ return;
+
+ memset(buf, 0, sizeof(buf));
+
+ size = 5;
+ err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+ buf, &size, &retval, &duration);
+ CHECK(err != -1 || errno != ENOSPC || retval, "run",
+ "err %d errno %d retval %d\n",
+ err, errno, retval);
+
+ duration = 0;
+
+ CHECK(size != sizeof(pkt_v4), "out_size",
+ "incorrect output size, want %lu have %u\n",
+ sizeof(pkt_v4), size);
+
+ CHECK(buf[5] != 0, "overflow",
+ "prog_test_run ignored size hint\n");
+
+ bpf_object__close(obj);
+}
+
static void test_xdp(void)
{
struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1847,6 +1880,7 @@ int main(void)
jit_enabled = is_jit_enabled();
test_pkt_access();
+ test_output_size_hint();
test_xdp();
test_xdp_adjust_tail();
test_l4lb_all();
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 3/3] selftests: add a test for bpf_prog_test_run output size
From: Y Song @ 2018-11-20 16:58 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CACAyw99F8L7BoB_TVAS6fvY9yb7cu9sCGXhjYPQ82sFZR=xKYQ@mail.gmail.com>
On Tue, Nov 20, 2018 at 3:35 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Sun, 18 Nov 2018 at 05:59, Y Song <ys114321@gmail.com> wrote:
> >
> > On Fri, Nov 16, 2018 at 12:55 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > Make sure that bpf_prog_test_run returns the correct length
> > > in the size_out argument and that the kernel respects the
> > > output size hint.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > ---
> > > tools/testing/selftests/bpf/test_progs.c | 34 ++++++++++++++++++++++++
> > > 1 file changed, 34 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index 560d7527b86b..6ab98e10e86f 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -124,6 +124,39 @@ static void test_pkt_access(void)
> > > bpf_object__close(obj);
> > > }
> > >
> > > +static void test_output_size_hint(void)
> > > +{
> > > + const char *file = "./test_pkt_access.o";
> > > + struct bpf_object *obj;
> > > + __u32 retval, size, duration;
> > > + int err, prog_fd;
> > > + char buf[10];
> > > +
> > > + err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > > + if (err) {
> > > + error_cnt++;
> > > + return;
> > > + }
> > CHECK can also be used here.
> > if (CHECK(...)) {
> > goto done;
> > }
> > where label "done" is right before bpf_object__close.
>
> I just copied this part from test_pkt_access, happy to change it though.
> However, I think "goto done" would lead to freeing an unallocated
> object in this case?
Right, you can just return here.
>
> --
> Lorenz Bauer | Systems Engineer
> 25 Lavington St., London SE1 0NZ
>
> www.cloudflare.com
^ permalink raw reply
* Re: [PATCH v2 0/4] Fix unsafe BPF_PROG_TEST_RUN interface
From: Y Song @ 2018-11-20 17:18 UTC (permalink / raw)
To: lmb; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <20181120154306.16657-1-lmb@cloudflare.com>
On Tue, Nov 20, 2018 at 7:43 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
> This is because bpf_test_finish copies the output buffer to user space
> without checking its size. This can lead to the kernel overwriting
> data in user space after the buffer if xdp_adjust_head and friends are
> in play.
>
> Changes in v2:
> * Make the syscall return ENOSPC if data_size_out is too small
> * Make bpf_prog_test_run return EINVAL if size_out is missing
> * Document the new behaviour of data_size_out
>
> Lorenz Bauer (4):
> bpf: respect size hint to BPF_PROG_TEST_RUN if present
> tools: sync uapi/linux/bpf.h
> libbpf: require size hint in bpf_prog_test_run
> selftests: add a test for bpf_prog_test_run output size
For the series, if we decided to take this approach rather than
amending another field
in the uapi as described in https://www.spinics.net/lists/netdev/msg534277.html,
then
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-20 18:09 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Dave Hansen, Jethro Beekman, Florian Weimer,
Linux API, Jann Horn, Linus Torvalds, X86 ML, linux-arch, LKML,
Peter Zijlstra, Rich Felker, nhorman, npmccallum, Ayoun, Serge,
shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner,
Ingo Molnar, Borislav Petkov
In-Reply-To: <20181120101133.GA7319@linux.intel.com>
On Tue, Nov 20, 2018 at 12:11:33PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 19, 2018 at 09:00:08AM -0800, Andy Lutomirski wrote:
> > On Mon, Nov 19, 2018 at 8:02 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, Nov 19, 2018 at 07:29:36AM -0800, Andy Lutomirski wrote:
> > > > 1. The kernel needs some way to know *when* to apply this fixup.
> > > > Decoding the instruction stream and doing it to all exceptions that
> > > > hit an ENCLU instruction seems like a poor design.
> > >
> > > I'm not sure why you would ever need to do any type of fixup as the idea
> > > is to just return to AEP i.e. from chosen exceptions (EPCM, #UD) the AEP
> > > would work the same way as for exceptions that the kernel can deal with
> > > except filling the exception information to registers.
> >
> > Sure, but how does the kernel know when to do that and when to send a
> > signal? I don't really like decoding the instruction stream to figure
> > it out.
>
> Hmm... why you have to decode instruction stream to find that out? Would
> just depend on exception type (#GP with EPCM, #UD).
#PF w/ PFEC_SGX is the only exception that indicates a fault is related
to SGX. Theoretically we could avoid decoding by using a magic value
for the AEP itself and doing even more magic fixup, but that wouldn't
help for faults that occur on EENTER, which can be generic #GPs due to
loss of EPC on SGX1 systems.
> Or are you saying
> that kernel should need to SIGSEGV if there is in fact ENCLU so that
> there is no infinite trap loop? Sorry, I'm a bit lost here that where
> does this decoding requirement comes from in the first place. I
> understand how it is used in Sean's proposal...
>
> Anyway, this option can be probably discarded without further
> consideration because apparently single stepping can cause #DB SS fault
> if AEP handler is anything else than a single instruction.
Not that it matters, but we could satisfy the "one instruction"
requirement if the fixup changed RIP to point at an ENCLU for #DBs.
> For me it seems that by ruling out options, vDSO option is what is
> left. I don't like it but at least it works...
>
> /Jarkko
^ permalink raw reply
* Re: [RFC PATCH 1/3] mm, proc: be more verbose about unstable VMA flags in /proc/<pid>/smaps
From: Dan Williams @ 2018-11-20 18:32 UTC (permalink / raw)
To: Michal Hocko
Cc: Linux API, Andrew Morton, adobriyan, Linux MM,
Linux Kernel Mailing List, Michal Hocko, Jan Kara, David Rientjes
In-Reply-To: <20181120103515.25280-2-mhocko@kernel.org>
On Tue, Nov 20, 2018 at 2:35 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> From: Michal Hocko <mhocko@suse.com>
>
> Even though vma flags exported via /proc/<pid>/smaps are explicitly
> documented to be not guaranteed for future compatibility the warning
> doesn't go far enough because it doesn't mention semantic changes to
> those flags. And they are important as well because these flags are
> a deep implementation internal to the MM code and the semantic might
> change at any time.
>
> Let's consider two recent examples:
> http://lkml.kernel.org/r/20181002100531.GC4135@quack2.suse.cz
> : commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
> : removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
> : mean time certain customer of ours started poking into /proc/<pid>/smaps
> : and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
> : flags, the application just fails to start complaining that DAX support is
> : missing in the kernel.
>
> http://lkml.kernel.org/r/alpine.DEB.2.21.1809241054050.224429@chino.kir.corp.google.com
> : Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> : introduced a regression in that userspace cannot always determine the set
> : of vmas where thp is ineligible.
> : Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
> : to determine if a vma is eligible to be backed by hugepages.
> : Previous to this commit, prctl(PR_SET_THP_DISABLE, 1) would cause thp to
> : be disabled and emit "nh" as a flag for the corresponding vmas as part of
> : /proc/pid/smaps. After the commit, thp is disabled by means of an mm
> : flag and "nh" is not emitted.
> : This causes smaps parsing libraries to assume a vma is eligible for thp
> : and ends up puzzling the user on why its memory is not backed by thp.
>
> In both cases userspace was relying on a semantic of a specific VMA
> flag. The primary reason why that happened is a lack of a proper
> internface. While this has been worked on and it will be fixed properly,
> it seems that our wording could see some refinement and be more vocal
> about semantic aspect of these flags as well.
>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Rientjes <rientjes@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> Documentation/filesystems/proc.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index 12a5e6e693b6..b1fda309f067 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -496,7 +496,9 @@ flags associated with the particular virtual memory area in two letter encoded
>
> Note that there is no guarantee that every flag and associated mnemonic will
> be present in all further kernel releases. Things get changed, the flags may
> -be vanished or the reverse -- new added.
> +be vanished or the reverse -- new added. Interpretatation of their meaning
> +might change in future as well. So each consumnent of these flags have to
> +follow each specific kernel version for the exact semantic.
Can we start to claw some of this back? Perhaps with a config option
to hide the flags to put applications on notice? I recall that when I
introduced CONFIG_IO_STRICT_DEVMEM it caused enough regressions that
distros did not enable it, but now a few years out I'm finding that it
is enabled in more places.
In any event,
Acked-by: Dan Williams <dan.j.williams@intel.com>
^ permalink raw reply
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