Re: [PATCH v2] mm_access: simplify the security checks

[Lorenzo Stoakes]

+cc Liam for mm lifecycle stuff :)

The subject here seems not quite right - you're adding complexity here in that
now there's a racey fast path.

On Sat, May 30, 2026 at 04:12:32PM +0200, Oleg Nesterov wrote:
> 1. Shift the fast-path "mm == current->mm" check from may_access_mm()
>    to mm_access(), and do it locklessly.
>
>    task->mm is not stable but we do not care. We can race with exec,
>    but in this case we pin/return current->mm. This doesn't differ
>    from the case where the target execs after we drop exec_update_lock.

Well it does differ in that previously we increment a reference counter
on the mm with the exec lock held, and now we don't?

Also how often are we invoking this where mm == current->mm?

I think the reasoning here is more so that current is guaranteed to hold a
reference on current->mm (de_thread() will have issued a fatal signal not
yet processed).

So the commit message should say that I think.

One behavioural change here though is that down_read_killable() was used
previously, so such a situation would return -EINTR, but now would instead
succeed.

>
>    All we need for correctness is READ_ONCE() to ensure the compiler
>    won't reload task->mm. This is not enough for KCSAN, but we already

I'm not sure 'this is not enough for KCSAN' is really reassuring :)

>    have other lockless ->mm LOAD's. We should probably change exec_mmap/
>    exit_mm to use WRITE_ONCE().
>
> 2. With the change above, may_access_mm() doesn't need the "mm" argument,
>    so we do not need to call get_task_mm() beforehand. We can call it
>    only if may_access_mm() succeeds.
>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---

It's useful to put a revision history (ideally with links to prior revisions)
below the --- line to explain how vN differs from v(N-1).

>  kernel/fork.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b8b651abce8b..3239380ab93b 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1381,10 +1381,8 @@ struct mm_struct *get_task_mm(struct task_struct *task)
>  }
>  EXPORT_SYMBOL_GPL(get_task_mm);
>
> -static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode)
> +static bool may_access_mm(struct task_struct *task, unsigned int mode)
>  {
> -	if (mm == current->mm)
> -		return true;
>  	if (ptrace_may_access(task, mode))
>  		return true;
>  	if ((mode & PTRACE_MODE_READ) && perfmon_capable())
> @@ -1394,20 +1392,24 @@ static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsign
>
>  struct mm_struct *mm_access(struct task_struct *task, unsigned int mode)
>  {
> -	struct mm_struct *mm;
> -	int err;
> +	struct mm_struct *mm = READ_ONCE(task->mm);
>
> -	err =  down_read_killable(&task->signal->exec_update_lock);
> -	if (err)
> -		return ERR_PTR(err);
> +	if (!mm || (task->flags & PF_KTHREAD))
> +		return ERR_PTR(-ESRCH);

This really needs a comment to explain your reasoning for while READ_ONCE()
suffices here.

>
> -	mm = get_task_mm(task);
> -	if (!mm) {
> -		mm = ERR_PTR(-ESRCH);
> -	} else if (!may_access_mm(mm, task, mode)) {
> -		mmput(mm);
> -		mm = ERR_PTR(-EACCES);
> +	if (mm == current->mm) {
> +		mmget(mm);
> +		return mm;
>  	}
> +
> +	if (down_read_killable(&task->signal->exec_update_lock))
> +		return ERR_PTR(-EINTR);
> +
> +	if (may_access_mm(task, mode))
> +		mm = get_task_mm(task) ?: ERR_PTR(-ESRCH);
> +	else
> +		mm = ERR_PTR(-EACCES);
> +
>  	up_read(&task->signal->exec_update_lock);
>
>  	return mm;
> --
> 2.52.0
>
>

(Side-note - we should really have these functions (and anything else
mm-related) in mm files.)

Overall I'm not really convinced about this patch - this isn't simplifying
things, it's introducing subtle assumptions and I don't really see the
benefit?

So I think it's a no unless you can provide a really solid justification.

And if it's a performance thing - how often are we actually calling
mm_access() for current->mm?

mm lifecycle is a very horrible part of mm and I think we should only make
changes when really necessary.

Thanks, Lorenzo