Message-ID: <Yr66Uhcv+XAPYPwj@dhcp22.suse.cz>
Date:   Fri, 1 Jul 2022 11:11:46 +0200
From:   Michal Hocko <mhocko@...e.com>
To:     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

[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/
> 
> Signed-off-by: Oleksandr Natalenko <oleksandr@...hat.com>
> Signed-off-by: xu xin <xu.xin16@....com.cn>
> ---
>  mm/madvise.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 851fa4e134bc..df915531ad9f 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1173,6 +1173,10 @@ process_madvise_behavior_valid(int behavior)
>  	case MADV_COLD:
>  	case MADV_PAGEOUT:
>  	case MADV_WILLNEED:
> +#ifdef CONFIG_KSM
> +	case MADV_MERGEABLE:
> +	case MADV_UNMERGEABLE:
> +#endif
>  		return true;
>  	default:
>  		return false;
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists