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
