Message-ID: <203548a6-cf70-30ce-6756-f6c909e7ef21@redhat.com>
Date:   Fri, 1 Jul 2022 14:09:24 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     cgel.zte@...il.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, vbabka@...e.cz, minchan@...nel.org,
        oleksandr@...hat.com, xu xin <xu.xin16@....com.cn>,
        Jann Horn <jannh@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH linux-next] mm/madvise: allow KSM hints for
 process_madvise

On 01.07.22 14:02, Michal Hocko wrote:
> On Fri 01-07-22 12:50:59, David Hildenbrand wrote:
>> On 01.07.22 12:32, David Hildenbrand wrote:
>>> On 01.07.22 11:11, Michal Hocko wrote:
>>>> [Cc Jann]
>>>>
>>>> On Fri 01-07-22 08:43:23, cgel.zte@...il.com wrote:
>>>>> From: xu xin <xu.xin16@....com.cn>
>>>>>
>>>>> The benefits of doing this are obvious because using madvise in user code
>>>>> is the only current way to enable KSM, which is inconvenient for those
>>>>> compiled app without marking MERGEABLE wanting to enable KSM.
>>>>
>>>> I would rephrase:
>>>> "
>>>> KSM functionality is currently available only to processes which are
>>>> using MADV_MERGEABLE directly. This is limiting because there are
>>>> usecases which will benefit from enabling KSM on a remote process. One
>>>> example would be an application which cannot be modified (e.g. because
>>>> it is only distributed as a binary). MORE EXAMPLES WOULD BE REALLY
>>>> BENEFICIAL.
>>>> "
>>>>
>>>>> Since we already have the syscall of process_madvise(), then reusing the
>>>>> interface to allow external KSM hints is more acceptable [1].
>>>>>
>>>>> Although this patch was released by Oleksandr Natalenko, but it was
>>>>> unfortunately terminated without any conclusions because there was debate
>>>>> on whether it should use signal_pending() to check the target task besides
>>>>> the task of current() when calling unmerge_ksm_pages of other task [2].
>>>>
>>>> I am not sure this is particularly interesting. I do not remember
>>>> details of that discussion but checking signal_pending on a different
>>>> task is rarely the right thing to do. In this case the check is meant to
>>>> allow bailing out from the operation so that the caller could be
>>>> terminated for example.
>>>>
>>>>> I think it's unneeded to check the target task. For example, when we set
>>>>> the klob /sys/kernel/mm/ksm/run from 1 to 2,
>>>>> unmerge_and_remove_all_rmap_items() doesn't use signal_pending() to check
>>>>> all other target tasks either.
>>>>>
>>>>> I hope this patch can get attention again.
>>>>
>>>> One thing that the changelog is missing and it is quite important IMHO
>>>> is the permission model. As we have discussed in previous incarnations
>>>> of the remote KSM functionality that KSM has some security implications.
>>>> It would be really great to refer to that in the changelog for the
>>>> future reference (http://lkml.kernel.org/r/CAG48ez0riS60zcA9CC9rUDV=kLS0326Rr23OKv1_RHaTkOOj7A@mail.gmail.com)
>>>>
>>>> So this implementation requires PTRACE_MODE_READ_FSCREDS and
>>>> CAP_SYS_NICE so the remote process would need to be allowed to
>>>> introspect the address space. This is the same constrain applied to the
>>>> remote momory reclaim. Is this sufficient?
>>>>
>>>> I would say yes because to some degree KSM mergning can have very
>>>> similar effect to memory reclaim from the side channel POV. But it
>>>> should be really documented in the changelog so that it is clear that
>>>> this has been a deliberate decision and thought through.
>>>>
>>>> Other than that this looks like the most reasonable approach to me.
>>>>
>>>>> [1] https://lore.kernel.org/lkml/YoOrdh85+AqJH8w1@dhcp22.suse.cz/
>>>>> [2] https://lore.kernel.org/lkml/2a66abd8-4103-f11b-06d1-07762667eee6@suse.cz/
>>>>>
>>>
>>> I have various concerns, but the biggest concern is that this modifies
>>> VMA flags and can possibly break applications.
>>>
>>> process_madvise must not modify remote process state.
>>>
>>> That's why we only allow a very limited selection that are merely hints.
>>>
>>> So nack from my side.
>>>
>>
>> [I'm quit ebusy, but I think some more explanation might be of value]
>>
>> One COW example where I think force-enabling KSM for processes is
>> *currently* not a good idea (besides the side channel discussions, which
>> is also why Windows stopped to enable KSM system wide a while ago):
>>
>> App:
>>
>> a) memset(page, 0);
>> b) trigger R/O long-term pin on page (e.g., vfio)
>>
>> If between a) and b) KSM replaces the page by the shared zeropage you'll
>> get an unreliable pin because we don't break yet COW when taking a R/O
>> pin on the shared zeropage. And in the traditional sense, the app did
>> everything right to guarantee that the pin will stay reliable.
> 
> Isn't this a bug in the existing implementation of the CoW?

One the one hand yes (pinning the shared zeropage is questionable), on
the other hand no (user space did modify that memory ahead of time and
filled it with something reasonable, that's how why always worked
correctly in the absence of KSM).

> 
>> Further, if an app explicitly decides to disable KSM one some region, we
>> should not overwrite that.
> 
> Well, the interface is rather spartan. You cannot really tell "disable
> KSM on some reqion". You can only tell "KSM can be applied to this
> region" and later change your mind. Maybe this is what you had in
> mind though.

That's what I meant. The hugepage interface has different semantics and
you get three possible states:

1: yes please: MADV_HUGEPAGE
2: don't care -- don't set anything
3. please no: MADV_NOHUGEPAGE

Currently for KSM we only have 1 and 2 internally I think (single
flag), because it didn't matter in the past ebcause there was no
force-enablement. One could convert it into all 3 states, changing the
semantics of MADV_UNMERGEABLE slightly from


1: yes please: MADV_MERGEABLE
2: don't care: MADV_UNMERGEABLE

to

1: yes please: MADV_MERGEABLE
2: don't care -- don't set anything
3. please no: MADV_UNMERGEABLE


-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists