Message-ID: <c9de1c34-2a39-e4a2-c9b0-9790c5ffab13@redhat.com>
Date:   Fri, 1 Jul 2022 12:50:59 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Michal Hocko <mhocko@...e.com>, cgel.zte@...il.com
Cc:     akpm@...ux-foundation.org, 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>
Subject: Re: [PATCH linux-next] mm/madvise: allow KSM hints for
 process_madvise

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.


Further, if an app explicitly decides to disable KSM one some region, we
should not overwrite that.


I can see that we might want such a (VMA-wide? process-wide?
system-wide?) overwrite, but IMHO we would have to make sure that

a) Any eventual side effects (see above) are handled correctly.
b) The app has an explicit mechanism to certainly disable KSM for a
   region (e.g., storing secrets) -- similarly to MADV_NOHUGEPAGE that
   ensures that there will *not* be huge pages.

IOW, fixes for a) [I'm planning on doing that] and two sets of flags for
b) to distinguish what the app wants and what somebody else might want.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists