Re: [PATCH RFC v3 2/6] riscv/mm: add untagged_addr_remote_unlocked()

[Usama Arif]

On Fri, 17 Jul 2026 13:00:32 -0400 Rik van Riel <riel@xxxxxxxxxxx> wrote:

> __access_remote_vm() untags the remote address before looking up the VMA,
> now without holding the mmap lock. riscv defines untagged_addr_remote() but

This describes behavior introduced in a later patch, not by this patch.
At this point, __access_remote_vm() still acquires mmap_lock before
untagging the address.

> not untagged_addr_remote_unlocked(), so it falls back to the generic
> version, which untags with untagged_addr().
> 
> That reads current->mm, not the target mm, so a remote access to a process
> using pointer masking would untag with the wrong mask.
> 
> mm->context.pmlen is set only through PR_SET_TAGGED_ADDR_CTRL and is stable
> afterwards, so it can be read without the mmap lock, as it already is from
> untagged_addr() and mm_untag_mask().
>

I think it might not be stable? set_tagged_addr_ctrl() can change it
repeatedly until a CLONE_VM operation sets MM_CONTEXT_LOCK_PMLEN.

> Add untagged_addr_remote_unlocked(), which untags against the target mm,
> and annotate context.pmlen accesses with READ_ONCE() and WRITE_ONCE() so
> the lockless reads are explicit and KCSAN-clean. untagged_addr_remote()
> keeps its mmap_assert_locked() and shares the code.
> 
> Assisted-by: Claude:claude-opus-4.8
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> ---
>  arch/riscv/include/asm/mmu_context.h |  4 ++--
>  arch/riscv/include/asm/uaccess.h     | 10 +++++++---
>  arch/riscv/kernel/process.c          | 12 +++++++-----
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/mmu_context.h b/arch/riscv/include/asm/mmu_context.h
> index dbf27a78df6c..3ce16796e5a2 100644
> --- a/arch/riscv/include/asm/mmu_context.h
> +++ b/arch/riscv/include/asm/mmu_context.h
> @@ -21,7 +21,7 @@ static inline void activate_mm(struct mm_struct *prev,
>  			       struct mm_struct *next)
>  {
>  #ifdef CONFIG_RISCV_ISA_SUPM
> -	next->context.pmlen = 0;
> +	WRITE_ONCE(next->context.pmlen, 0);
>  #endif
>  	switch_mm(prev, next, NULL);
>  }
> @@ -44,7 +44,7 @@ DECLARE_STATIC_KEY_FALSE(use_asid_allocator);
>  #define mm_untag_mask mm_untag_mask
>  static inline unsigned long mm_untag_mask(struct mm_struct *mm)
>  {
> -	return -1UL >> mm->context.pmlen;
> +	return -1UL >> READ_ONCE(mm->context.pmlen);
>  }
>  #endif
>  
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 5d4ec15584cf..53806e0f7dcf 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -16,7 +16,7 @@
>  static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigned long addr)
>  {
>  	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) {
> -		u8 pmlen = mm->context.pmlen;
> +		u8 pmlen = READ_ONCE(mm->context.pmlen);
>  
>  		/* Virtual addresses are sign-extended; physical addresses are zero-extended. */
>  		if (IS_ENABLED(CONFIG_MMU))
> @@ -33,12 +33,16 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigne
>  	(__force __typeof__(addr))__untagged_addr_remote(current->mm, __addr);	\
>  })
>  
> -#define untagged_addr_remote(mm, addr) ({					\
> +#define untagged_addr_remote_unlocked(mm, addr) ({				\
>  	unsigned long __addr = (__force unsigned long)(addr);			\
> -	mmap_assert_locked(mm);							\
>  	(__force __typeof__(addr))__untagged_addr_remote(mm, __addr);		\
>  })
>  
> +#define untagged_addr_remote(mm, addr) ({					\
> +	mmap_assert_locked(mm);							\
> +	untagged_addr_remote_unlocked(mm, addr);				\
> +})
> +
>  #define access_ok(addr, size) likely(__access_ok(untagged_addr(addr), size))
>  #else
>  #define untagged_addr(addr) (addr)
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index b2df7f72241a..6ae7552fed09 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -357,13 +357,15 @@ long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg)
>  	if (mmap_write_lock_killable(mm))
>  		return -EINTR;
>  
> -	if (test_bit(MM_CONTEXT_LOCK_PMLEN, &mm->context.flags) && mm->context.pmlen != pmlen) {
> -		mmap_write_unlock(mm);
> -		return -EBUSY;
> +	if (test_bit(MM_CONTEXT_LOCK_PMLEN, &mm->context.flags)) {
> +		if (READ_ONCE(mm->context.pmlen) != pmlen) {
> +			mmap_write_unlock(mm);
> +			return -EBUSY;
> +		}
>  	}
>  
>  	envcfg_update_bits(task, ENVCFG_PMM, pmm);
> -	mm->context.pmlen = pmlen;
> +	WRITE_ONCE(mm->context.pmlen, pmlen);
>  
>  	mmap_write_unlock(mm);
>  
> @@ -394,7 +396,7 @@ long get_tagged_addr_ctrl(struct task_struct *task)
>  		break;
>  	}
>  
> -	if (task->mm->context.pmlen)
> +	if (READ_ONCE(task->mm->context.pmlen))
>  		ret |= PR_TAGGED_ADDR_ENABLE;
>  
>  	return ret;
> -- 
> 2.53.0-Meta
> 
>