Now, we have MADV_PAGEOUT and MADV_COLD as madvise hinting API. With that,
application could give hints to kernel what memory range are preferred to be
reclaimed. However, in some platform(e.g., Android), the information
required to make the hinting decision is not known to the app.
Instead, it is known to a centralized userspace daemon(e.g., ActivityManagerService),
and that daemon must be able to initiate reclaim on its own without any app
involvement.
To solve the concern, this patch introduces new syscall - process_madvise(2).
Bascially, it's same with madvise(2) syscall but it has some differences.
1. It needs pidfd of target process to provide the hint
2. It supports only MADV_{COLD|PAGEOUT|MERGEABLE|UNMEREABLE} at this moment.
Other hints in madvise will be opened when there are explicit requests from
community to prevent unexpected bugs we couldn't support.
3. Only privileged processes can do something for other process's address
space.
For more detail of the new API, please see "mm: introduce external memory hinting API"
description in this patchset.
Minchan Kim (6):
mm: pass task to do_madvise
mm: introduce external memory hinting API
mm: validate mm in do_madvise
mm: check fatal signal pending of target process
pid: export pidfd_get_pid
mm: support both pid and pidfd for process_madvise
Oleksandr Natalenko (2):
mm/madvise: employ mmget_still_valid for write lock
mm/madvise: allow KSM hints for remote API
* from v3 - https://lore.kernel.org/linux-mm/[email protected]/
* verify task->mm aftere access_mm - Oleg
* split some patches for easy review - Alexander
* clean up fatal signal checking - Suren
* from v2 - https://lore.kernel.org/linux-mm/[email protected]/
* check signal callee and caller to bail out - Kirill Tkhai
* put more clarification for justification of new API
* from v1 - https://lore.kernel.org/linux-mm/[email protected]/
* fix syscall number - SeongJae
* use get_pid_task - Kirill Tkhai
* extend API to support pid as well as pidfd - Kirill Tkhai
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/io_uring.c | 2 +-
include/linux/mm.h | 3 +-
include/linux/pid.h | 1 +
include/linux/syscalls.h | 3 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/exit.c | 17 ---
kernel/pid.c | 17 +++
kernel/sys_ni.c | 1 +
mm/madvise.c | 149 ++++++++++++++++----
26 files changed, 170 insertions(+), 46 deletions(-)
--
2.25.0.225.g125e21ebc7-goog
In upcoming patches, do_madvise will be called from external process
context so it shouldn't asssume "current" is always hinted process's
task_struct. Thus, let's get the mm_struct from vma->vm_mm, not
current because vma is always hinted process's one. And let's pass
*current* as new task argument of do_madvise so it shouldn't change
existing behavior.
Signed-off-by: Minchan Kim <[email protected]>
---
fs/io_uring.c | 2 +-
include/linux/mm.h | 3 ++-
mm/madvise.c | 37 ++++++++++++++++++++-----------------
3 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 63beda9bafc5..6307206b970f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2736,7 +2736,7 @@ static int io_madvise(struct io_kiocb *req, struct io_kiocb **nxt,
if (force_nonblock)
return -EAGAIN;
- ret = do_madvise(ma->addr, ma->len, ma->advice);
+ ret = do_madvise(current, ma->addr, ma->len, ma->advice);
if (ret < 0)
req_set_fail_links(req);
io_cqring_add_event(req, ret);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 52269e56c514..8cb41131ec96 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2323,7 +2323,8 @@ extern int __do_munmap(struct mm_struct *, unsigned long, size_t,
struct list_head *uf, bool downgrade);
extern int do_munmap(struct mm_struct *, unsigned long, size_t,
struct list_head *uf);
-extern int do_madvise(unsigned long start, size_t len_in, int behavior);
+extern int do_madvise(struct task_struct *task, unsigned long start,
+ size_t len_in, int behavior);
static inline unsigned long
do_mmap_pgoff(struct file *file, unsigned long addr,
diff --git a/mm/madvise.c b/mm/madvise.c
index 43b47d3fae02..ab4011ba2d9e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -256,6 +256,7 @@ static long madvise_willneed(struct vm_area_struct *vma,
{
struct file *file = vma->vm_file;
loff_t offset;
+ struct mm_struct *mm = vma->vm_mm;
*prev = vma;
#ifdef CONFIG_SWAP
@@ -288,12 +289,12 @@ static long madvise_willneed(struct vm_area_struct *vma,
*/
*prev = NULL; /* tell sys_madvise we drop mmap_sem */
get_file(file);
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
offset = (loff_t)(start - vma->vm_start)
+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
vfs_fadvise(file, offset, end - start, POSIX_FADV_WILLNEED);
fput(file);
- down_read(¤t->mm->mmap_sem);
+ down_read(&mm->mmap_sem);
return 0;
}
@@ -674,9 +675,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
}
out:
if (nr_swap) {
- if (current->mm == mm)
- sync_mm_rss(mm);
-
+ sync_mm_rss(mm);
add_mm_counter(mm, MM_SWAPENTS, nr_swap);
}
arch_leave_lazy_mmu_mode();
@@ -756,6 +755,7 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
unsigned long start, unsigned long end,
int behavior)
{
+ struct mm_struct *mm = vma->vm_mm;
*prev = vma;
if (!can_madv_lru_vma(vma))
return -EINVAL;
@@ -763,8 +763,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
if (!userfaultfd_remove(vma, start, end)) {
*prev = NULL; /* mmap_sem has been dropped, prev is stale */
- down_read(¤t->mm->mmap_sem);
- vma = find_vma(current->mm, start);
+ down_read(&mm->mmap_sem);
+ vma = find_vma(mm, start);
if (!vma)
return -ENOMEM;
if (start < vma->vm_start) {
@@ -818,6 +818,7 @@ static long madvise_remove(struct vm_area_struct *vma,
loff_t offset;
int error;
struct file *f;
+ struct mm_struct *mm = vma->vm_mm;
*prev = NULL; /* tell sys_madvise we drop mmap_sem */
@@ -845,13 +846,13 @@ static long madvise_remove(struct vm_area_struct *vma,
get_file(f);
if (userfaultfd_remove(vma, start, end)) {
/* mmap_sem was not released by userfaultfd_remove() */
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
}
error = vfs_fallocate(f,
FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
offset, end - start);
fput(f);
- down_read(¤t->mm->mmap_sem);
+ down_read(&mm->mmap_sem);
return error;
}
@@ -1044,7 +1045,8 @@ madvise_behavior_valid(int behavior)
* -EBADF - map exists, but area maps something that isn't a file.
* -EAGAIN - a kernel resource was temporarily unavailable.
*/
-int do_madvise(unsigned long start, size_t len_in, int behavior)
+int do_madvise(struct task_struct *task, unsigned long start,
+ size_t len_in, int behavior)
{
unsigned long end, tmp;
struct vm_area_struct *vma, *prev;
@@ -1053,6 +1055,7 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
int write;
size_t len;
struct blk_plug plug;
+ struct mm_struct *mm = task->mm;
start = untagged_addr(start);
@@ -1082,10 +1085,10 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
write = madvise_need_mmap_write(behavior);
if (write) {
- if (down_write_killable(¤t->mm->mmap_sem))
+ if (down_write_killable(&mm->mmap_sem))
return -EINTR;
} else {
- down_read(¤t->mm->mmap_sem);
+ down_read(&mm->mmap_sem);
}
/*
@@ -1093,7 +1096,7 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
* ranges, just ignore them, but return -ENOMEM at the end.
* - different from the way of handling in mlock etc.
*/
- vma = find_vma_prev(current->mm, start, &prev);
+ vma = find_vma_prev(mm, start, &prev);
if (vma && start > vma->vm_start)
prev = vma;
@@ -1130,19 +1133,19 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
if (prev)
vma = prev->vm_next;
else /* madvise_remove dropped mmap_sem */
- vma = find_vma(current->mm, start);
+ vma = find_vma(mm, start);
}
out:
blk_finish_plug(&plug);
if (write)
- up_write(¤t->mm->mmap_sem);
+ up_write(&mm->mmap_sem);
else
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
return error;
}
SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
{
- return do_madvise(start, len_in, behavior);
+ return do_madvise(current, start, len_in, behavior);
}
--
2.25.0.225.g125e21ebc7-goog
Oleg pointed out mm could be nulllified right after mm_access succeeds.
This patch validates it before the using.
Cc: Oleg Nesterov <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
mm/madvise.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index 8611f1d39289..bb04c7897eb9 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1073,7 +1073,11 @@ int do_madvise(struct task_struct *task, unsigned long start,
int write;
size_t len;
struct blk_plug plug;
- struct mm_struct *mm = task->mm;
+ struct mm_struct *mm = READ_ONCE(task->mm);
+
+ /* task can exit and nullify its ->mm right after mm_access() */
+ if (!mm)
+ return -ESRCH;
start = untagged_addr(start);
--
2.25.0.225.g125e21ebc7-goog
Bail out to prevent unnecessary CPU overhead if target process ha
pending fatal signal during MADV_COLD| MADV_PAGEOUT operation.
Signed-off-by: Minchan Kim <[email protected]>
---
mm/madvise.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index bb04c7897eb9..276b9d81c1dd 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -36,6 +36,7 @@
struct madvise_walk_private {
struct mmu_gather *tlb;
bool pageout;
+ struct task_struct *task;
};
/*
@@ -316,6 +317,9 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
if (fatal_signal_pending(current))
return -EINTR;
+ if (fatal_signal_pending(private->task))
+ return -EINTR;
+
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
if (pmd_trans_huge(*pmd)) {
pmd_t orig_pmd;
@@ -471,12 +475,14 @@ static const struct mm_walk_ops cold_walk_ops = {
};
static void madvise_cold_page_range(struct mmu_gather *tlb,
+ struct task_struct *task,
struct vm_area_struct *vma,
unsigned long addr, unsigned long end)
{
struct madvise_walk_private walk_private = {
.pageout = false,
.tlb = tlb,
+ .task = task,
};
tlb_start_vma(tlb, vma);
@@ -484,7 +490,8 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
tlb_end_vma(tlb, vma);
}
-static long madvise_cold(struct vm_area_struct *vma,
+static long madvise_cold(struct task_struct *task,
+ struct vm_area_struct *vma,
struct vm_area_struct **prev,
unsigned long start_addr, unsigned long end_addr)
{
@@ -497,19 +504,21 @@ static long madvise_cold(struct vm_area_struct *vma,
lru_add_drain();
tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
- madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
+ madvise_cold_page_range(&tlb, task, vma, start_addr, end_addr);
tlb_finish_mmu(&tlb, start_addr, end_addr);
return 0;
}
static void madvise_pageout_page_range(struct mmu_gather *tlb,
+ struct task_struct *task,
struct vm_area_struct *vma,
unsigned long addr, unsigned long end)
{
struct madvise_walk_private walk_private = {
.pageout = true,
.tlb = tlb,
+ .task = task,
};
tlb_start_vma(tlb, vma);
@@ -533,7 +542,8 @@ static inline bool can_do_pageout(struct vm_area_struct *vma)
inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
}
-static long madvise_pageout(struct vm_area_struct *vma,
+static long madvise_pageout(struct task_struct *task,
+ struct vm_area_struct *vma,
struct vm_area_struct **prev,
unsigned long start_addr, unsigned long end_addr)
{
@@ -549,7 +559,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
lru_add_drain();
tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
- madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
+ madvise_pageout_page_range(&tlb, task, vma, start_addr, end_addr);
tlb_finish_mmu(&tlb, start_addr, end_addr);
return 0;
@@ -927,7 +937,8 @@ static int madvise_inject_error(int behavior,
#endif
static long
-madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
+madvise_vma(struct task_struct *task, struct vm_area_struct *vma,
+ struct vm_area_struct **prev,
unsigned long start, unsigned long end, int behavior)
{
switch (behavior) {
@@ -936,9 +947,9 @@ madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
case MADV_WILLNEED:
return madvise_willneed(vma, prev, start, end);
case MADV_COLD:
- return madvise_cold(vma, prev, start, end);
+ return madvise_cold(task, vma, prev, start, end);
case MADV_PAGEOUT:
- return madvise_pageout(vma, prev, start, end);
+ return madvise_pageout(task, vma, prev, start, end);
case MADV_FREE:
case MADV_DONTNEED:
return madvise_dontneed_free(vma, prev, start, end, behavior);
@@ -1143,7 +1154,7 @@ int do_madvise(struct task_struct *task, unsigned long start,
tmp = end;
/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
- error = madvise_vma(vma, &prev, start, tmp, behavior);
+ error = madvise_vma(task, vma, &prev, start, tmp, behavior);
if (error)
goto out;
start = tmp;
--
2.25.0.225.g125e21ebc7-goog
From: Oleksandr Natalenko <[email protected]>
Do the very same trick as we already do since 04f5866e41fb. KSM hints
will require locking mmap_sem for write since they modify vm_flags, so
for remote KSM hinting this additional check is needed.
Signed-off-by: Oleksandr Natalenko <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
mm/madvise.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/madvise.c b/mm/madvise.c
index 276b9d81c1dd..71f0ba199ae8 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1120,6 +1120,8 @@ int do_madvise(struct task_struct *task, unsigned long start,
if (write) {
if (down_write_killable(&mm->mmap_sem))
return -EINTR;
+ if (current->mm != mm && !mmget_still_valid(mm))
+ goto skip_mm;
} else {
down_read(&mm->mmap_sem);
}
@@ -1170,6 +1172,7 @@ int do_madvise(struct task_struct *task, unsigned long start,
}
out:
blk_finish_plug(&plug);
+skip_mm:
if (write)
up_write(&mm->mmap_sem);
else
--
2.25.0.225.g125e21ebc7-goog
From: Oleksandr Natalenko <[email protected]>
It all began with the fact that KSM works only on memory that is marked
by madvise(). And the only way to get around that is to either:
* use LD_PRELOAD; or
* patch the kernel with something like UKSM or PKSM.
(i skip ptrace can of worms here intentionally)
To overcome this restriction, lets employ a new remote madvise API. This
can be used by some small userspace helper daemon that will do auto-KSM
job for us.
I think of two major consumers of remote KSM hints:
* hosts, that run containers, especially similar ones and especially in
a trusted environment, sharing the same runtime like Node.js;
* heavy applications, that can be run in multiple instances, not
limited to opensource ones like Firefox, but also those that cannot be
modified since they are binary-only and, maybe, statically linked.
Speaking of statistics, more numbers can be found in the very first
submission, that is related to this one [1]. For my current setup with
two Firefox instances I get 100 to 200 MiB saved for the second instance
depending on the amount of tabs.
1 FF instance with 15 tabs:
$ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
410
2 FF instances, second one has 12 tabs (all the tabs are different):
$ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
592
At the very moment I do not have specific numbers for containerised
workload, but those should be comparable in case the containers share
similar/same runtime.
[1] https://lore.kernel.org/patchwork/patch/1012142/
Signed-off-by: Oleksandr Natalenko <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
mm/madvise.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/mm/madvise.c b/mm/madvise.c
index 71f0ba199ae8..b1237466657e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1002,6 +1002,10 @@ process_madvise_behavior_valid(int behavior)
switch (behavior) {
case MADV_COLD:
case MADV_PAGEOUT:
+#ifdef CONFIG_KSM
+ case MADV_MERGEABLE:
+ case MADV_UNMERGEABLE:
+#endif
return true;
default:
return false;
--
2.25.0.225.g125e21ebc7-goog
process_madvise syscall needs pidfd_get_pid function to translate
pidfd to pid so this patch exports the function.
Cc: Christian Brauner <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
include/linux/pid.h | 1 +
kernel/exit.c | 17 -----------------
kernel/pid.c | 17 +++++++++++++++++
3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 998ae7d24450..023d9c3a8edc 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
struct file;
extern struct pid *pidfd_pid(const struct file *file);
+extern struct pid *pidfd_get_pid(unsigned int fd);
static inline struct pid *get_pid(struct pid *pid)
{
diff --git a/kernel/exit.c b/kernel/exit.c
index 0b81b26a872a..43375f9d8bbc 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1470,23 +1470,6 @@ static long do_wait(struct wait_opts *wo)
return retval;
}
-static struct pid *pidfd_get_pid(unsigned int fd)
-{
- struct fd f;
- struct pid *pid;
-
- f = fdget(fd);
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- pid = pidfd_pid(f.file);
- if (!IS_ERR(pid))
- get_pid(pid);
-
- fdput(f);
- return pid;
-}
-
static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
int options, struct rusage *ru)
{
diff --git a/kernel/pid.c b/kernel/pid.c
index 0f4ecb57214c..360ba480a2a9 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -496,6 +496,23 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
return idr_get_next(&ns->idr, &nr);
}
+struct pid *pidfd_get_pid(unsigned int fd)
+{
+ struct fd f;
+ struct pid *pid;
+
+ f = fdget(fd);
+ if (!f.file)
+ return ERR_PTR(-EBADF);
+
+ pid = pidfd_pid(f.file);
+ if (!IS_ERR(pid))
+ get_pid(pid);
+
+ fdput(f);
+ return pid;
+}
+
/**
* pidfd_create() - Create a new pid file descriptor.
*
--
2.25.0.225.g125e21ebc7-goog
There is usecase that System Management Software(SMS) want to give
a memory hint like MADV_[COLD|PAGEEOUT] to other processes and
in the case of Android, it is the ActivityManagerService.
It's similar in spirit to madvise(MADV_WONTNEED), but the information
required to make the reclaim decision is not known to the app. Instead,
it is known to the centralized userspace daemon(ActivityManagerService),
and that daemon must be able to initiate reclaim on its own without
any app involvement.
To solve the issue, this patch introduces a new syscall process_madvise(2).
It uses pidfd of an external process to give the hint.
int process_madvise(int pidfd, void *addr, size_t length, int advise,
unsigned long flag);
Since it could affect other process's address range, only privileged
process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
gives it the right to ptrace the process could use it successfully.
The flag argument is reserved for future use if we need to extend the
API.
I think supporting all hints madvise has/will supported/support to
process_madvise is rather risky. Because we are not sure all hints make
sense from external process and implementation for the hint may rely on
the caller being in the current context so it could be error-prone.
Thus, I just limited hints as MADV_[COLD|PAGEOUT] in this patch.
If someone want to add other hints, we could hear hear the usecase and
review it for each hint. It's safer for maintenance rather than
introducing a buggy syscall but hard to fix it later.
Q.1 - Why does any external entity have better knowledge?
Quote from Sandeep
"For Android, every application (including the special SystemServer) are forked
from Zygote. The reason of course is to share as many libraries and classes between
the two as possible to benefit from the preloading during boot.
After applications start, (almost) all of the APIs end up calling into this
SystemServer process over IPC (binder) and back to the application.
In a fully running system, the SystemServer monitors every single process
periodically to calculate their PSS / RSS and also decides which process is
"important" to the user for interactivity.
So, because of how these processes start _and_ the fact that the SystemServer
is looping to monitor each process, it does tend to *know* which address
range of the application is not used / useful.
Besides, we can never rely on applications to clean things up themselves.
We've had the "hey app1, the system is low on memory, please trim your
memory usage down" notifications for a long time[1]. They rely on
applications honoring the broadcasts and very few do.
So, if we want to avoid the inevitable killing of the application and
restarting it, some way to be able to tell the OS about unimportant memory in
these applications will be useful.
- ssp
Q.2 - How to guarantee the race(i.e., object validation) between when giving a
hint from an external process and get the hint from the target process?
process_madvise operates on the target process's address space as it exists
at the instant that process_madvise is called. If the space target process
can run between the time the process_madvise process inspects the target
process address space and the time that process_madvise is actually called,
process_madvise may operate on memory regions that the calling process does
not expect. It's the responsibility of the process calling process_madvise
to close this race condition. For example, the calling process can suspend
the target process with ptrace, SIGSTOP, or the freezer cgroup so that it
doesn't have an opportunity to change its own address space before
process_madvise is called. Another option is to operate on memory regions
that the caller knows a priori will be unchanged in the target process.
Yet another option is to accept the race for certain process_madvise calls
after reasoning that mistargeting will do no harm. The suggested API itself
does not provide synchronization. It also apply other APIs like move_pages,
process_vm_write.
The race isn't really a problem though. Why is it so wrong to require
that callers do their own synchronization in some manner? Nobody objects
to write(2) merely because it's possible for two processes to open the same
file and clobber each other's writes --- instead, we tell people to use
flock or something. Think about mmap. It never guarantees newly allocated
address space is still valid when the user tries to access it because other
threads could unmap the memory right before. That's where we need
synchronization by using other API or design from userside. It shouldn't
be part of API itself. If someone needs more fine-grained synchronization
rather than process level, there were two ideas suggested - cookie[2] and
anon-fd[3]. Both are applicable via using last reserved argument of the API
but I don't think it's necessary right now since we have already ways to
prevent the race so don't want to add additional complexity with more
fine-grained optimization model.
To make the API extend, it reserved an unsigned long as last argument
so we could support it in future if someone really needs it.
Q.3 - Why doesn't ptrace work?
Injecting an madvise in the target process using ptrace would not work for us
because such injected madvise would have to be executed by the target process,
which means that process would have to be runnable and that creates the risk
of the abovementioned race and hinting a wrong VMA. Furthermore, we want to
act the hint in caller's context, not calle because calle is usually limited
in cpuset/cgroups or even freezed state so they can't act by themselves
quick enough, which causes more thrashing/kill. It doesn't work if the
target process are ptraced(e.g., strace, debugger, minidump) because a
process can have at most one ptracer.
[1] https://developer.android.com/topic/performance/memory"
[2] process_getinfo for getting the cookie which is updated whenever
vma of process address layout are changed - Daniel Colascione
- https://lore.kernel.org/lkml/[email protected]/T/#m7694416fd179b2066a2c62b5b139b14e3894e224
[3] anonymous fd which is used for the object(i.e., address range)
validation - Michal Hocko
- https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Minchan Kim <[email protected]>
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/syscalls.h | 2 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/sys_ni.c | 1 +
mm/madvise.c | 64 +++++++++++++++++++++
21 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 36d42da7466a..c82952e6fb80 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -477,3 +477,4 @@
# 545 reserved for clone3
547 common openat2 sys_openat2
548 common pidfd_getfd sys_pidfd_getfd
+549 common process_madvise sys_process_madvise
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 4d1cf74a2caa..54c2719fec46 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -451,3 +451,4 @@
435 common clone3 sys_clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 1dd22da1c3a9..75f04a1023be 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 439
+#define __NR_compat_syscalls 440
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index c1c61635f89c..2a27be7a1f91 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
__SYSCALL(__NR_openat2, sys_openat2)
#define __NR_pidfd_getfd 438
__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
+#define __NR_process_madvise 439
+__SYSCALL(__NR_process_madvise, process_madvise)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index 042911e670b8..9524af1c318c 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -358,3 +358,4 @@
# 435 reserved for clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index f4f49fcb76d0..8197050c097c 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -437,3 +437,4 @@
435 common clone3 __sys_clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 4c67b11f9c9e..c5b6c8afe445 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -443,3 +443,4 @@
435 common clone3 sys_clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 1f9e8ad636cc..8ec8c558aa9c 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -376,3 +376,4 @@
435 n32 clone3 __sys_clone3
437 n32 openat2 sys_openat2
438 n32 pidfd_getfd sys_pidfd_getfd
+439 n32 process_madvise sys_process_madvise
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index c0b9d802dbf6..0078f891bb92 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -352,3 +352,4 @@
435 n64 clone3 __sys_clone3
437 n64 openat2 sys_openat2
438 n64 pidfd_getfd sys_pidfd_getfd
+439 n64 process_madvise sys_process_madvise
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 52a15f5cd130..09c3b5dc6855 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -435,3 +435,4 @@
435 common clone3 sys_clone3_wrapper
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 35b61bfc1b1a..97eac48c2937 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -519,3 +519,4 @@
435 nospu clone3 ppc_clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index bd7bd3581a0f..8dc8bfd958ea 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -440,3 +440,4 @@
435 common clone3 sys_clone3 sys_clone3
437 common openat2 sys_openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise sys_process_madvise
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index c7a30fcd135f..e69d98040777 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -440,3 +440,4 @@
# 435 reserved for clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index f13615ecdecc..6f6e66dd51f9 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -483,3 +483,4 @@
# 435 reserved for clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index c17cb77eb150..1b2184549e27 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -442,3 +442,4 @@
435 i386 clone3 sys_clone3 __ia32_sys_clone3
437 i386 openat2 sys_openat2 __ia32_sys_openat2
438 i386 pidfd_getfd sys_pidfd_getfd __ia32_sys_pidfd_getfd
+439 i386 process_madvise sys_process_madvise __ia32_sys_process_madvise
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 44d510bc9b78..82d60eb1e00d 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -359,6 +359,7 @@
435 common clone3 __x64_sys_clone3/ptregs
437 common openat2 __x64_sys_openat2
438 common pidfd_getfd __x64_sys_pidfd_getfd
+439 common process_madvise __x64_sys_process_madvise
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 85a9ab1bc04d..165cae047770 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -408,3 +408,4 @@
435 common clone3 sys_clone3
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
+439 common process_madvise sys_process_madvise
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 1815065d52f3..e4cd2c2f8bb4 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -876,6 +876,8 @@ asmlinkage long sys_munlockall(void);
asmlinkage long sys_mincore(unsigned long start, size_t len,
unsigned char __user * vec);
asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
+asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
+ size_t len, int behavior, unsigned long flags);
asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
unsigned long prot, unsigned long pgoff,
unsigned long flags);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 3a3201e4618e..85d8c9376a63 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -855,9 +855,11 @@ __SYSCALL(__NR_clone3, sys_clone3)
__SYSCALL(__NR_openat2, sys_openat2)
#define __NR_pidfd_getfd 438
__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
+#define __NR_pidfd_getfd 439
+__SYSCALL(__NR_process_madvise, sys_process_madvise)
#undef __NR_syscalls
-#define __NR_syscalls 439
+#define __NR_syscalls 440
/*
* 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 3b69a560a7ac..6c7332776e8e 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -280,6 +280,7 @@ COND_SYSCALL(mlockall);
COND_SYSCALL(munlockall);
COND_SYSCALL(mincore);
COND_SYSCALL(madvise);
+COND_SYSCALL(process_madvise);
COND_SYSCALL(remap_file_pages);
COND_SYSCALL(mbind);
COND_SYSCALL_COMPAT(mbind);
diff --git a/mm/madvise.c b/mm/madvise.c
index ab4011ba2d9e..8611f1d39289 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -17,6 +17,7 @@
#include <linux/falloc.h>
#include <linux/fadvise.h>
#include <linux/sched.h>
+#include <linux/sched/mm.h>
#include <linux/ksm.h>
#include <linux/fs.h>
#include <linux/file.h>
@@ -984,6 +985,18 @@ madvise_behavior_valid(int behavior)
}
}
+static bool
+process_madvise_behavior_valid(int behavior)
+{
+ switch (behavior) {
+ case MADV_COLD:
+ case MADV_PAGEOUT:
+ return true;
+ default:
+ return false;
+ }
+}
+
/*
* The madvise(2) system call.
*
@@ -1031,6 +1044,11 @@ madvise_behavior_valid(int behavior)
* MADV_DONTDUMP - the application wants to prevent pages in the given range
* from being included in its core dump.
* MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
+ * MADV_COLD - the application uses the memory less so the kernel can
+ * deactivate the memory to evict them quickly when the memory
+ * pressure happen.
+ * MADV_PAGEOUT - the application uses the memroy very rarely so kernel can
+ * page out the memory instantly.
*
* return values:
* zero - success
@@ -1149,3 +1167,49 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
{
return do_madvise(current, start, len_in, behavior);
}
+
+SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
+ size_t, len_in, int, behavior, unsigned long, flags)
+{
+ int ret;
+ struct fd f;
+ struct pid *pid;
+ struct task_struct *task;
+ struct mm_struct *mm;
+
+ if (flags != 0)
+ return -EINVAL;
+
+ if (!process_madvise_behavior_valid(behavior))
+ return -EINVAL;
+
+ f = fdget(pidfd);
+ if (!f.file)
+ return -EBADF;
+
+ pid = pidfd_pid(f.file);
+ if (IS_ERR(pid)) {
+ ret = PTR_ERR(pid);
+ goto fdput;
+ }
+
+ task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task) {
+ ret = -ESRCH;
+ goto fdput;
+ }
+
+ mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
+ if (IS_ERR_OR_NULL(mm)) {
+ ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
+ goto release_task;
+ }
+
+ ret = do_madvise(task, start, len_in, behavior);
+ mmput(mm);
+release_task:
+ put_task_struct(task);
+fdput:
+ fdput(f);
+ return ret;
+}
--
2.25.0.225.g125e21ebc7-goog
There is a demand[1] to support pid as well pidfd for process_madvise
to reduce unnecessary syscall to get pidfd if the user has control of
the target process(ie, they could guarantee the process is not gone
or pid is not reused. Or, it might be okay to give a hint to wrong
process).
This patch aims for supporting both options like waitid(2). So, the
syscall is currently,
int process_madvise(int which, pid_t pid, void *addr,
size_t length, int advise, unsigned long flag);
@which is actually idtype_t for userspace libray and currently,
it supports P_PID and P_PIDFD.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Cc: Christian Brauner <[email protected]>
Suggested-by: Kirill Tkhai <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
include/linux/syscalls.h | 3 ++-
mm/madvise.c | 34 ++++++++++++++++++++++------------
2 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e4cd2c2f8bb4..f5ada20e2943 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -876,7 +876,8 @@ asmlinkage long sys_munlockall(void);
asmlinkage long sys_mincore(unsigned long start, size_t len,
unsigned char __user * vec);
asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
-asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
+
+asmlinkage long sys_process_madvise(int which, pid_t pid, unsigned long start,
size_t len, int behavior, unsigned long flags);
asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
unsigned long prot, unsigned long pgoff,
diff --git a/mm/madvise.c b/mm/madvise.c
index b1237466657e..32833d0ba574 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1190,11 +1190,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
return do_madvise(current, start, len_in, behavior);
}
-SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
+SYSCALL_DEFINE6(process_madvise, int, which, pid_t, upid, unsigned long, start,
size_t, len_in, int, behavior, unsigned long, flags)
{
int ret;
- struct fd f;
struct pid *pid;
struct task_struct *task;
struct mm_struct *mm;
@@ -1205,20 +1204,31 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
if (!process_madvise_behavior_valid(behavior))
return -EINVAL;
- f = fdget(pidfd);
- if (!f.file)
- return -EBADF;
+ switch (which) {
+ case P_PID:
+ if (upid <= 0)
+ return -EINVAL;
+
+ pid = find_get_pid(upid);
+ if (!pid)
+ return -ESRCH;
+ break;
+ case P_PIDFD:
+ if (upid < 0)
+ return -EINVAL;
- pid = pidfd_pid(f.file);
- if (IS_ERR(pid)) {
- ret = PTR_ERR(pid);
- goto fdput;
+ pid = pidfd_get_pid(upid);
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
+ break;
+ default:
+ return -EINVAL;
}
task = get_pid_task(pid, PIDTYPE_PID);
if (!task) {
ret = -ESRCH;
- goto fdput;
+ goto put_pid;
}
mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
@@ -1231,7 +1241,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
mmput(mm);
release_task:
put_task_struct(task);
-fdput:
- fdput(f);
+put_pid:
+ put_pid(pid);
return ret;
}
--
2.25.0.225.g125e21ebc7-goog
On Wed, 2020-02-12 at 15:39 -0800, Minchan Kim wrote:
> In upcoming patches, do_madvise will be called from external process
> context so it shouldn't asssume "current" is always hinted process's
> task_struct. Thus, let's get the mm_struct from vma->vm_mm, not
> current because vma is always hinted process's one. And let's pass
> *current* as new task argument of do_madvise so it shouldn't change
> existing behavior.
>
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> fs/io_uring.c | 2 +-
> include/linux/mm.h | 3 ++-
> mm/madvise.c | 37 ++++++++++++++++++++-----------------
> 3 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 63beda9bafc5..6307206b970f 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2736,7 +2736,7 @@ static int io_madvise(struct io_kiocb *req, struct io_kiocb **nxt,
> if (force_nonblock)
> return -EAGAIN;
>
> - ret = do_madvise(ma->addr, ma->len, ma->advice);
> + ret = do_madvise(current, ma->addr, ma->len, ma->advice);
> if (ret < 0)
> req_set_fail_links(req);
> io_cqring_add_event(req, ret);
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 52269e56c514..8cb41131ec96 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2323,7 +2323,8 @@ extern int __do_munmap(struct mm_struct *, unsigned long, size_t,
> struct list_head *uf, bool downgrade);
> extern int do_munmap(struct mm_struct *, unsigned long, size_t,
> struct list_head *uf);
> -extern int do_madvise(unsigned long start, size_t len_in, int behavior);
> +extern int do_madvise(struct task_struct *task, unsigned long start,
> + size_t len_in, int behavior);
>
> static inline unsigned long
> do_mmap_pgoff(struct file *file, unsigned long addr,
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 43b47d3fae02..ab4011ba2d9e 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -256,6 +256,7 @@ static long madvise_willneed(struct vm_area_struct *vma,
> {
> struct file *file = vma->vm_file;
> loff_t offset;
> + struct mm_struct *mm = vma->vm_mm;
>
> *prev = vma;
> #ifdef CONFIG_SWAP
I would probably move the declaration of the mm variable to the top just
so you don't have the large "offset" valley between the two long variable
declarations.
> @@ -288,12 +289,12 @@ static long madvise_willneed(struct vm_area_struct *vma,
> */
> *prev = NULL; /* tell sys_madvise we drop mmap_sem */
> get_file(file);
> - up_read(¤t->mm->mmap_sem);
> + up_read(&mm->mmap_sem);
> offset = (loff_t)(start - vma->vm_start)
> + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> vfs_fadvise(file, offset, end - start, POSIX_FADV_WILLNEED);
> fput(file);
> - down_read(¤t->mm->mmap_sem);
> + down_read(&mm->mmap_sem);
> return 0;
> }
>
> @@ -674,9 +675,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> }
> out:
> if (nr_swap) {
> - if (current->mm == mm)
> - sync_mm_rss(mm);
> -
> + sync_mm_rss(mm);
> add_mm_counter(mm, MM_SWAPENTS, nr_swap);
> }
> arch_leave_lazy_mmu_mode();
This seems like it is taking things in the opposite direction of the other
changes. sync_mm_rss will operate on current if I am not mistaken. I don't
think you would want to add the stats from current to the stats of the
task you are updating.
It might make sense to add a new function that would allow you to sync the
remote task stats by creaing a version of sync_mm_rss that also takes a
task pointer.
> @@ -756,6 +755,7 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> unsigned long start, unsigned long end,
> int behavior)
> {
> + struct mm_struct *mm = vma->vm_mm;
> *prev = vma;
> if (!can_madv_lru_vma(vma))
> return -EINVAL;
> @@ -763,8 +763,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> if (!userfaultfd_remove(vma, start, end)) {
> *prev = NULL; /* mmap_sem has been dropped, prev is stale */
>
> - down_read(¤t->mm->mmap_sem);
> - vma = find_vma(current->mm, start);
> + down_read(&mm->mmap_sem);
> + vma = find_vma(mm, start);
> if (!vma)
> return -ENOMEM;
> if (start < vma->vm_start) {
This piece of code has me wondering if it is valid to be using vma->mm at
the start of the function. I assume we are probably safe since we read the
mm value before the semaphore was released in userfaultfd_remove. It might
make more sense to just pass the task to the function and use task->mm-
>mmap_sem instead.
It might be simpler, safer, and easier to review to just go through and
add the task struct as needed and then simply replace references to
current->mm with task->mm.
> @@ -818,6 +818,7 @@ static long madvise_remove(struct vm_area_struct *vma,
> loff_t offset;
> int error;
> struct file *f;
> + struct mm_struct *mm = vma->vm_mm;
>
> *prev = NULL; /* tell sys_madvise we drop mmap_sem */
>
> @@ -845,13 +846,13 @@ static long madvise_remove(struct vm_area_struct *vma,
> get_file(f);
> if (userfaultfd_remove(vma, start, end)) {
> /* mmap_sem was not released by userfaultfd_remove() */
> - up_read(¤t->mm->mmap_sem);
> + up_read(&mm->mmap_sem);
> }
> error = vfs_fallocate(f,
> FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> offset, end - start);
> fput(f);
> - down_read(¤t->mm->mmap_sem);
> + down_read(&mm->mmap_sem);
> return error;
> }
>
> @@ -1044,7 +1045,8 @@ madvise_behavior_valid(int behavior)
> * -EBADF - map exists, but area maps something that isn't a file.
> * -EAGAIN - a kernel resource was temporarily unavailable.
> */
> -int do_madvise(unsigned long start, size_t len_in, int behavior)
> +int do_madvise(struct task_struct *task, unsigned long start,
> + size_t len_in, int behavior)
> {
> unsigned long end, tmp;
> struct vm_area_struct *vma, *prev;
> @@ -1053,6 +1055,7 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> int write;
> size_t len;
> struct blk_plug plug;
> + struct mm_struct *mm = task->mm;
>
> start = untagged_addr(start);
>
> @@ -1082,10 +1085,10 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
>
> write = madvise_need_mmap_write(behavior);
> if (write) {
> - if (down_write_killable(¤t->mm->mmap_sem))
> + if (down_write_killable(&mm->mmap_sem))
> return -EINTR;
> } else {
> - down_read(¤t->mm->mmap_sem);
> + down_read(&mm->mmap_sem);
> }
>
> /*
> @@ -1093,7 +1096,7 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> * ranges, just ignore them, but return -ENOMEM at the end.
> * - different from the way of handling in mlock etc.
> */
> - vma = find_vma_prev(current->mm, start, &prev);
> + vma = find_vma_prev(mm, start, &prev);
> if (vma && start > vma->vm_start)
> prev = vma;
>
> @@ -1130,19 +1133,19 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> if (prev)
> vma = prev->vm_next;
> else /* madvise_remove dropped mmap_sem */
> - vma = find_vma(current->mm, start);
> + vma = find_vma(mm, start);
> }
> out:
> blk_finish_plug(&plug);
> if (write)
> - up_write(¤t->mm->mmap_sem);
> + up_write(&mm->mmap_sem);
> else
> - up_read(¤t->mm->mmap_sem);
> + up_read(&mm->mmap_sem);
>
> return error;
> }
>
> SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> {
> - return do_madvise(start, len_in, behavior);
> + return do_madvise(current, start, len_in, behavior);
> }
On Wed, 2020-02-12 at 15:39 -0800, Minchan Kim wrote:
> process_madvise syscall needs pidfd_get_pid function to translate
> pidfd to pid so this patch exports the function.
>
> Cc: Christian Brauner <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
I think you might have misunderstood my earlier comments. This should be
patch 2 in your set. What is patch 8 should be folded into you existing
patch 2 and become patch 3 with the rest of your patches shifted by 1
since you are reordering them.
Otherwise the code itself appears to not have changed anything so it looks
fine to me.
Reviewed-by: Alexander Duyck <[email protected]>
> ---
> include/linux/pid.h | 1 +
> kernel/exit.c | 17 -----------------
> kernel/pid.c | 17 +++++++++++++++++
> 3 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index 998ae7d24450..023d9c3a8edc 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
> struct file;
>
> extern struct pid *pidfd_pid(const struct file *file);
> +extern struct pid *pidfd_get_pid(unsigned int fd);
>
> static inline struct pid *get_pid(struct pid *pid)
> {
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 0b81b26a872a..43375f9d8bbc 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1470,23 +1470,6 @@ static long do_wait(struct wait_opts *wo)
> return retval;
> }
>
> -static struct pid *pidfd_get_pid(unsigned int fd)
> -{
> - struct fd f;
> - struct pid *pid;
> -
> - f = fdget(fd);
> - if (!f.file)
> - return ERR_PTR(-EBADF);
> -
> - pid = pidfd_pid(f.file);
> - if (!IS_ERR(pid))
> - get_pid(pid);
> -
> - fdput(f);
> - return pid;
> -}
> -
> static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
> int options, struct rusage *ru)
> {
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 0f4ecb57214c..360ba480a2a9 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -496,6 +496,23 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> return idr_get_next(&ns->idr, &nr);
> }
>
> +struct pid *pidfd_get_pid(unsigned int fd)
> +{
> + struct fd f;
> + struct pid *pid;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return ERR_PTR(-EBADF);
> +
> + pid = pidfd_pid(f.file);
> + if (!IS_ERR(pid))
> + get_pid(pid);
> +
> + fdput(f);
> + return pid;
> +}
> +
> /**
> * pidfd_create() - Create a new pid file descriptor.
> *
On Wed, 2020-02-12 at 15:39 -0800, Minchan Kim wrote:
> There is a demand[1] to support pid as well pidfd for process_madvise
> to reduce unnecessary syscall to get pidfd if the user has control of
> the target process(ie, they could guarantee the process is not gone
> or pid is not reused. Or, it might be okay to give a hint to wrong
> process).
>
> This patch aims for supporting both options like waitid(2). So, the
> syscall is currently,
>
> int process_madvise(int which, pid_t pid, void *addr,
> size_t length, int advise, unsigned long flag);
>
> @which is actually idtype_t for userspace libray and currently,
> it supports P_PID and P_PIDFD.
>
> [1] https://lore.kernel.org/linux-mm/[email protected]/
>
> Cc: Christian Brauner <[email protected]>
> Suggested-by: Kirill Tkhai <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
So if you move patch 7 up before patch 2 you could squash this patch with
your current patch 2 and drop one patch from your series. It would
probably help to reduce the review overhead as well.
On Thu, Feb 13, 2020 at 12:40 AM Minchan Kim <[email protected]> wrote:
> To solve the issue, this patch introduces a new syscall process_madvise(2).
> It uses pidfd of an external process to give the hint.
[...]
> + mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
> + if (IS_ERR_OR_NULL(mm)) {
> + ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> + goto release_task;
> + }
> +
> + ret = do_madvise(task, start, len_in, behavior);
When you're accessing another task, you should ensure that the other
task doesn't gain new privileges by executing a setuid binary in the
middle of being accessed. mm_access() does that for you; it holds the
->cred_guard_mutex while it is looking up the task's ->mm and doing
the security check. mm_access() then returns you an mm pointer that
you're allowed to access without worrying about such things; an
mm_struct never gains privileges, since a setuid execution creates a
fresh mm_struct. However, the task may still execute setuid binaries
and such things.
This means that after you've looked up the mm with mm_access(), you
have to actually *use* that pointer. You're not allowed to simply read
task->mm yourself.
Therefore, I think you should:
- change patch 1/8 ("mm: pass task to do_madvise") to also pass an
mm_struct* to do_madvise (but keep the task_struct* for patch 4/8)
- in this patch, pass the mm_struct* from mm_access() into do_madvise()
- drop patch 3/8 ("mm: validate mm in do_madvise"); it just papers
over a symptom without addressing the underlying problem
Hi Jann,
On Thu, Feb 13, 2020 at 03:08:59PM +0100, Jann Horn wrote:
> On Thu, Feb 13, 2020 at 12:40 AM Minchan Kim <[email protected]> wrote:
> > To solve the issue, this patch introduces a new syscall process_madvise(2).
> > It uses pidfd of an external process to give the hint.
> [...]
> > + mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
> > + if (IS_ERR_OR_NULL(mm)) {
> > + ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> > + goto release_task;
> > + }
> > +
> > + ret = do_madvise(task, start, len_in, behavior);
>
> When you're accessing another task, you should ensure that the other
> task doesn't gain new privileges by executing a setuid binary in the
> middle of being accessed. mm_access() does that for you; it holds the
> ->cred_guard_mutex while it is looking up the task's ->mm and doing
> the security check. mm_access() then returns you an mm pointer that
> you're allowed to access without worrying about such things; an
> mm_struct never gains privileges, since a setuid execution creates a
> fresh mm_struct. However, the task may still execute setuid binaries
> and such things.
>
> This means that after you've looked up the mm with mm_access(), you
> have to actually *use* that pointer. You're not allowed to simply read
> task->mm yourself.
>
> Therefore, I think you should:
>
> - change patch 1/8 ("mm: pass task to do_madvise") to also pass an
> mm_struct* to do_madvise (but keep the task_struct* for patch 4/8)
> - in this patch, pass the mm_struct* from mm_access() into do_madvise()
> - drop patch 3/8 ("mm: validate mm in do_madvise"); it just papers
> over a symptom without addressing the underlying problem
Actually, it was what this patch series was doing until last version
but I changed it to reduce just *a parameter* to do_madvise.
And then, this time, I got a good advise I was not familiar.
I will fix it again.
Thanks for the review!
On Wed, Feb 12, 2020 at 04:21:59PM -0800, Alexander Duyck wrote:
> On Wed, 2020-02-12 at 15:39 -0800, Minchan Kim wrote:
> > In upcoming patches, do_madvise will be called from external process
> > context so it shouldn't asssume "current" is always hinted process's
> > task_struct. Thus, let's get the mm_struct from vma->vm_mm, not
> > current because vma is always hinted process's one. And let's pass
> > *current* as new task argument of do_madvise so it shouldn't change
> > existing behavior.
> >
> > Signed-off-by: Minchan Kim <[email protected]>
> > ---
> > fs/io_uring.c | 2 +-
> > include/linux/mm.h | 3 ++-
> > mm/madvise.c | 37 ++++++++++++++++++++-----------------
> > 3 files changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 63beda9bafc5..6307206b970f 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -2736,7 +2736,7 @@ static int io_madvise(struct io_kiocb *req, struct io_kiocb **nxt,
> > if (force_nonblock)
> > return -EAGAIN;
> >
> > - ret = do_madvise(ma->addr, ma->len, ma->advice);
> > + ret = do_madvise(current, ma->addr, ma->len, ma->advice);
> > if (ret < 0)
> > req_set_fail_links(req);
> > io_cqring_add_event(req, ret);
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 52269e56c514..8cb41131ec96 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2323,7 +2323,8 @@ extern int __do_munmap(struct mm_struct *, unsigned long, size_t,
> > struct list_head *uf, bool downgrade);
> > extern int do_munmap(struct mm_struct *, unsigned long, size_t,
> > struct list_head *uf);
> > -extern int do_madvise(unsigned long start, size_t len_in, int behavior);
> > +extern int do_madvise(struct task_struct *task, unsigned long start,
> > + size_t len_in, int behavior);
> >
> > static inline unsigned long
> > do_mmap_pgoff(struct file *file, unsigned long addr,
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 43b47d3fae02..ab4011ba2d9e 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -256,6 +256,7 @@ static long madvise_willneed(struct vm_area_struct *vma,
> > {
> > struct file *file = vma->vm_file;
> > loff_t offset;
> > + struct mm_struct *mm = vma->vm_mm;
> >
> > *prev = vma;
> > #ifdef CONFIG_SWAP
>
> I would probably move the declaration of the mm variable to the top just
> so you don't have the large "offset" valley between the two long variable
> declarations.
No problem.
>
> > @@ -288,12 +289,12 @@ static long madvise_willneed(struct vm_area_struct *vma,
> > */
> > *prev = NULL; /* tell sys_madvise we drop mmap_sem */
> > get_file(file);
> > - up_read(¤t->mm->mmap_sem);
> > + up_read(&mm->mmap_sem);
> > offset = (loff_t)(start - vma->vm_start)
> > + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> > vfs_fadvise(file, offset, end - start, POSIX_FADV_WILLNEED);
> > fput(file);
> > - down_read(¤t->mm->mmap_sem);
> > + down_read(&mm->mmap_sem);
> > return 0;
> > }
> >
> > @@ -674,9 +675,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> > }
> > out:
> > if (nr_swap) {
> > - if (current->mm == mm)
> > - sync_mm_rss(mm);
> > -
> > + sync_mm_rss(mm);
> > add_mm_counter(mm, MM_SWAPENTS, nr_swap);
> > }
> > arch_leave_lazy_mmu_mode();
>
> This seems like it is taking things in the opposite direction of the other
> changes. sync_mm_rss will operate on current if I am not mistaken. I don't
> think you would want to add the stats from current to the stats of the
> task you are updating.
>
> It might make sense to add a new function that would allow you to sync the
> remote task stats by creaing a version of sync_mm_rss that also takes a
> task pointer.
Sorry, that part was intended for other patch regardless of this patchset but
squeezed into this version by my mistake. I wanted to remove those part
from madvise_free because currently madvise_free is always done in current context.
I will drop it in next revision since it's not related to this patchset.
>
> > @@ -756,6 +755,7 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> > unsigned long start, unsigned long end,
> > int behavior)
> > {
> > + struct mm_struct *mm = vma->vm_mm;
> > *prev = vma;
> > if (!can_madv_lru_vma(vma))
> > return -EINVAL;
> > @@ -763,8 +763,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> > if (!userfaultfd_remove(vma, start, end)) {
> > *prev = NULL; /* mmap_sem has been dropped, prev is stale */
> >
> > - down_read(¤t->mm->mmap_sem);
> > - vma = find_vma(current->mm, start);
> > + down_read(&mm->mmap_sem);
> > + vma = find_vma(mm, start);
> > if (!vma)
> > return -ENOMEM;
> > if (start < vma->vm_start) {
>
> This piece of code has me wondering if it is valid to be using vma->mm at
> the start of the function. I assume we are probably safe since we read the
> mm value before the semaphore was released in userfaultfd_remove. It might
> make more sense to just pass the task to the function and use task->mm-
> >mmap_sem instead.
As Jann pointed out, we couldn't use task->mm once we verified it via
access_mm. However, I believe vma->vm_mm is safe(Ccing Jann for double
check).
>
> It might be simpler, safer, and easier to review to just go through and
> add the task struct as needed and then simply replace references to
> current->mm with task->mm.
>
> > @@ -818,6 +818,7 @@ static long madvise_remove(struct vm_area_struct *vma,
> > loff_t offset;
> > int error;
> > struct file *f;
> > + struct mm_struct *mm = vma->vm_mm;
> >
> > *prev = NULL; /* tell sys_madvise we drop mmap_sem */
> >
> > @@ -845,13 +846,13 @@ static long madvise_remove(struct vm_area_struct *vma,
> > get_file(f);
> > if (userfaultfd_remove(vma, start, end)) {
> > /* mmap_sem was not released by userfaultfd_remove() */
> > - up_read(¤t->mm->mmap_sem);
> > + up_read(&mm->mmap_sem);
> > }
> > error = vfs_fallocate(f,
> > FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> > offset, end - start);
> > fput(f);
> > - down_read(¤t->mm->mmap_sem);
> > + down_read(&mm->mmap_sem);
> > return error;
> > }
> >
> > @@ -1044,7 +1045,8 @@ madvise_behavior_valid(int behavior)
> > * -EBADF - map exists, but area maps something that isn't a file.
> > * -EAGAIN - a kernel resource was temporarily unavailable.
> > */
> > -int do_madvise(unsigned long start, size_t len_in, int behavior)
> > +int do_madvise(struct task_struct *task, unsigned long start,
> > + size_t len_in, int behavior)
> > {
> > unsigned long end, tmp;
> > struct vm_area_struct *vma, *prev;
> > @@ -1053,6 +1055,7 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> > int write;
> > size_t len;
> > struct blk_plug plug;
> > + struct mm_struct *mm = task->mm;
> >
> > start = untagged_addr(start);
> >
> > @@ -1082,10 +1085,10 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> >
> > write = madvise_need_mmap_write(behavior);
> > if (write) {
> > - if (down_write_killable(¤t->mm->mmap_sem))
> > + if (down_write_killable(&mm->mmap_sem))
> > return -EINTR;
> > } else {
> > - down_read(¤t->mm->mmap_sem);
> > + down_read(&mm->mmap_sem);
> > }
> >
> > /*
> > @@ -1093,7 +1096,7 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> > * ranges, just ignore them, but return -ENOMEM at the end.
> > * - different from the way of handling in mlock etc.
> > */
> > - vma = find_vma_prev(current->mm, start, &prev);
> > + vma = find_vma_prev(mm, start, &prev);
> > if (vma && start > vma->vm_start)
> > prev = vma;
> >
> > @@ -1130,19 +1133,19 @@ int do_madvise(unsigned long start, size_t len_in, int behavior)
> > if (prev)
> > vma = prev->vm_next;
> > else /* madvise_remove dropped mmap_sem */
> > - vma = find_vma(current->mm, start);
> > + vma = find_vma(mm, start);
> > }
> > out:
> > blk_finish_plug(&plug);
> > if (write)
> > - up_write(¤t->mm->mmap_sem);
> > + up_write(&mm->mmap_sem);
> > else
> > - up_read(¤t->mm->mmap_sem);
> > + up_read(&mm->mmap_sem);
> >
> > return error;
> > }
> >
> > SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > {
> > - return do_madvise(start, len_in, behavior);
> > + return do_madvise(current, start, len_in, behavior);
> > }
>
>
Hi Alexander,
On Wed, Feb 12, 2020 at 04:25:31PM -0800, Alexander Duyck wrote:
> On Wed, 2020-02-12 at 15:39 -0800, Minchan Kim wrote:
> > process_madvise syscall needs pidfd_get_pid function to translate
> > pidfd to pid so this patch exports the function.
> >
> > Cc: Christian Brauner <[email protected]>
> > Signed-off-by: Minchan Kim <[email protected]>
>
> I think you might have misunderstood my earlier comments. This should be
> patch 2 in your set. What is patch 8 should be folded into you existing
> patch 2 and become patch 3 with the rest of your patches shifted by 1
> since you are reordering them.
>
> Otherwise the code itself appears to not have changed anything so it looks
> fine to me.
>
> Reviewed-by: Alexander Duyck <[email protected]>
It was my intention because I expect supporting both pid and pidfd would be
controversial. It would make easy to revert.
Thanks for the review!
On Thu, Feb 13, 2020 at 6:02 PM Minchan Kim <[email protected]> wrote:
> On Wed, Feb 12, 2020 at 04:21:59PM -0800, Alexander Duyck wrote:
> > On Wed, 2020-02-12 at 15:39 -0800, Minchan Kim wrote:
> > > In upcoming patches, do_madvise will be called from external process
> > > context so it shouldn't asssume "current" is always hinted process's
> > > task_struct. Thus, let's get the mm_struct from vma->vm_mm, not
> > > current because vma is always hinted process's one. And let's pass
> > > *current* as new task argument of do_madvise so it shouldn't change
> > > existing behavior.
[...]
> > > @@ -763,8 +763,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> > > if (!userfaultfd_remove(vma, start, end)) {
> > > *prev = NULL; /* mmap_sem has been dropped, prev is stale */
> > >
> > > - down_read(¤t->mm->mmap_sem);
> > > - vma = find_vma(current->mm, start);
> > > + down_read(&mm->mmap_sem);
> > > + vma = find_vma(mm, start);
> > > if (!vma)
> > > return -ENOMEM;
> > > if (start < vma->vm_start) {
> >
> > This piece of code has me wondering if it is valid to be using vma->mm at
> > the start of the function. I assume we are probably safe since we read the
> > mm value before the semaphore was released in userfaultfd_remove. It might
> > make more sense to just pass the task to the function and use task->mm-
> > >mmap_sem instead.
>
> As Jann pointed out, we couldn't use task->mm once we verified it via
> access_mm. However, I believe vma->vm_mm is safe(Ccing Jann for double
> check).
Looks safe to me, too.