Re: [PATCH RFC 4/5] mm/ksm, proc: introduce remote merge

On Thu, May 16, 2019 at 4:20 PM Oleksandr Natalenko
[off-list ref] wrote:

On Thu, May 16, 2019 at 12:00:24PM +0200, Jann Horn wrote:

quoted

On Thu, May 16, 2019 at 11:43 AM Oleksandr Natalenko
[off-list ref] wrote:

quoted

Use previously introduced remote madvise knob to mark task's
anonymous memory as mergeable.

To force merging task's VMAs, "merge" hint is used:

   # echo merge > /proc/<pid>/madvise

Force unmerging is done similarly:

   # echo unmerge > /proc/<pid>/madvise

To achieve this, previously introduced ksm_madvise_*() helpers
are used.

Why does this not require PTRACE_MODE_ATTACH_FSCREDS to the target
process? Enabling KSM on another process is hazardous because it
significantly increases the attack surface for side channels.

(Note that if you change this to require PTRACE_MODE_ATTACH_FSCREDS,
you'll want to use mm_access() in the ->open handler and drop the mm
in ->release. mm_access() from a ->write handler is not permitted.)

Sounds reasonable. So, something similar to what mem_open() & friends do
now:

static int madvise_open(...)
...
        struct task_struct *task = get_proc_task(inode);
...
        if (task) {
                mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
                put_task_struct(task);
                if (!IS_ERR_OR_NULL(mm)) {
                        mmgrab(mm);
                        mmput(mm);
...

Then:

static ssize_t madvise_write(...)
...
        if (!mmget_not_zero(mm))
                goto out;

        down_write(&mm->mmap_sem);
        if (!mmget_still_valid(mm))
                goto skip_mm;
...
skip_mm:
        up_write(&mm->mmap_sem);

        mmput(mm);
out:
        return ...;

And, finally:

static int madvise_release(...)
...
                mmdrop(mm);
...

Right?

Yeah, that looks reasonable.