Re: [PATCH linux-next] mm/madvise: allow KSM hints for process_madvise

[David Hildenbrand]

On 01.07.22 11:11, Michal Hocko wrote:
> [Cc Jann]
> 
> On Fri 01-07-22 08:43:23, cgel.zte@xxxxxxxxx wrote:
>> From: xu xin <xu.xin16@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx)
> 
> 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@xxxxxxxxxxxxxx/
>> [2] https://lore.kernel.org/lkml/2a66abd8-4103-f11b-06d1-07762667eee6@xxxxxxx/
>>

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.

-- 
Thanks,

David / dhildenb