[PATCH] mm_access: simplify the security checks

[PATCH] mm_access: simplify the security checks

[Oleg Nesterov]

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.

   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
   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() suceeds.

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@redhat.com>
---
 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);
 
-	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

Re: [PATCH] mm_access: simplify the security checks

[Oleg Nesterov]

On 05/30, Oleg Nesterov wrote:
>
> 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() suceeds.
>
> 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.

OOPS. Somehow I wrote the same twice. Will fix the changelog and send V2
in reply to this patch...

Oleg.

[PATCH v2] mm_access: simplify the security checks

[Oleg Nesterov]

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.

   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
   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@redhat.com>
---
 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);
 
-	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

Re: [PATCH] mm_access: simplify the security checks

[Oleg Nesterov]

On 05/30, Oleg Nesterov wrote:
>
> 1. Shift the fast-path "mm == current->mm" check from may_access_mm()
>    to mm_access(), and do it locklessly.

Heh ;) sashiko has some concerns, see
https://sashiko.dev/#/patchset/ahrsfJE3NkKjShEX%40redhat.com

I still think this is fine, but I'll recheck tomorrow.

Oleg.

Re: [PATCH] mm_access: simplify the security checks

[Lorenzo Stoakes]

On Sat, May 30, 2026 at 04:10:07PM +0200, Oleg Nesterov wrote:
> On 05/30, Oleg Nesterov wrote:
> >
> > 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() suceeds.
> >
> > 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.
>
> OOPS. Somehow I wrote the same twice. Will fix the changelog and send V2
> in reply to this patch...

Sorry, pedantic side-note here but - in general in mm we prefer you don't send
new revision in-reply-to the prior one, as it makes them easier to miss! :)

>
> Oleg.
>

Cheers, Lorenzo

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

[David Hildenbrand (Arm)]

On 5/30/26 16:12, Oleg Nesterov wrote:

I am not sure I spot the "simplification" here?

Looks more like an optimization that makes the code slightly more complicated.

> 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.
> 
>    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
>    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@redhat.com>
> ---
>  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)

We now have a function named "may_access_mm" and not providing an mm struct
pointer ... it now looks more like an extended, mm-independent ptrace check.

So probably we should rename that function.

>  {
> -	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);
>  
> -	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;


-- 
Cheers,

David

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@redhat.com>
> ---

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

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

[Oleg Nesterov]

On 06/01, David Hildenbrand (Arm) wrote:
>
> On 5/30/26 16:12, Oleg Nesterov wrote:
>
> I am not sure I spot the "simplification" here?

Ah, then lets forget this patch ;)

Cleanups are always subjective, if maintainer doesn't agree - forget it.

> Looks more like an optimization that makes the code slightly more complicated.

I don't think that !task->mm or current->mm == task->mm is the common case.
Just the code looks more clear to me if it takes exec_update_lock only when
necessary.

But see above, please forget.

Oleg.

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

[Oleg Nesterov]

On 06/01, Lorenzo Stoakes wrote:
>
> +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.

OK. See my reply to David. If it doesn't look like a simplification -
lets forget this patch ;)

> 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.

I don't really follow... SIGKILL from de_thread() or anything else can
come right after down_read_killable().

> >    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 :)

If I understand correctly KCSAN will complain if (say) we race with the
exiting task which does current->mm = NULL without WRITE_ONCE in exit_mm().

> 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).

Yes... I didn't do it this time because V2 doesn't differ from V1, I just removed
the duplicated paragraph from the changelog.

> 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?

Thanks for review! lets forget this patch then.

Oleg.