Re: [PATCH] Revert "riscv: Define TASK_SIZE_MAX for __access_ok()"

[Palmer Dabbelt]

On Mon, 23 Jun 2025 02:15:48 PDT (-0700), Alexandre Ghiti wrote:
> Hi Nam,
>
> On 6/19/25 17:58, Nam Cao wrote:
> > This reverts commit ad5643cf2f69 ("riscv: Define TASK_SIZE_MAX for
> > __access_ok()").
> >
> > This commit changes TASK_SIZE_MAX to be LONG_MAX to optimize access_ok(),
> > because the previous TASK_SIZE_MAX (default to TASK_SIZE) requires some
> > computation.
> >
> > The reasoning was that all user addresses are less than LONG_MAX, and all
> > kernel addresses are greater than LONG_MAX. Therefore access_ok() can
> > filter kernel addresses.
> >
> > Addresses between TASK_SIZE and LONG_MAX are not valid user addresses, but
> > access_ok() let them pass. That was thought to be okay, because they are
> > not valid addresses at hardware level.
> >
> > Unfortunately, one case is missed: get_user_pages_fast() happily accepts
> > addresses between TASK_SIZE and LONG_MAX. futex(), for instance, uses
> > get_user_pages_fast(). This causes the problem reported by Robert [1].
> >
> > Therefore, revert this commit. TASK_SIZE_MAX is changed to the default:
> > TASK_SIZE.
> >
> > This unfortunately reduces performance, because TASK_SIZE is more expensive
> > to compute compared to LONG_MAX. But correctness first, we can think about
> > optimization later, if required.
> >
> > Reported-by: <rtm@xxxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/linux-riscv/77605.1750245028@localhost/
> > Signed-off-by: Nam Cao <namcao@xxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >   arch/riscv/include/asm/pgtable.h | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 438ce7df24c39..5bd5aae60d536 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -1075,7 +1075,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >    */
> >   #ifdef CONFIG_64BIT
> >   #define TASK_SIZE_64	(PGDIR_SIZE * PTRS_PER_PGD / 2)
> > -#define TASK_SIZE_MAX	LONG_MAX
> >
> >   #ifdef CONFIG_COMPAT
> >   #define TASK_SIZE_32	(_AC(0x80000000, UL) - PAGE_SIZE)
>
>
> I agree with this revert, the next step is to implement the same
> optimization using alternatives (like x86 does).
>
> Reviewed-by: Alexandre Ghiti <alexghiti@xxxxxxxxxxxx>
>
> It should land into -fixes.

It's not clear if you're picking it up?  I will, it's hitting the 
tester now...

> Thanks,
>
> Alex