Received-SPF: pass (google.com: domain of linux-kernel+bounces-106701-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <88de4a1a-047e-4be9-b5b0-3e53434dc022@sifive.com>
Date: Mon, 18 Mar 2024 16:29:30 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] riscv: Define TASK_SIZE_MAX for __access_ok()
Content-Language: en-US
To: Alexandre Ghiti <alexghiti@rivosinc.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, linux-riscv@lists.infradead.org,
 Albert Ou <aou@eecs.berkeley.edu>, Andrew Morton
 <akpm@linux-foundation.org>, Charlie Jenkins <charlie@rivosinc.com>,
 Guo Ren <guoren@kernel.org>, Jisheng Zhang <jszhang@kernel.org>,
 Kemeng Shi <shikemeng@huaweicloud.com>,
 "Matthew Wilcox (Oracle)" <willy@infradead.org>,
 "Mike Rapoport (IBM)" <rppt@kernel.org>,
 Paul Walmsley <paul.walmsley@sifive.com>, Xiao Wang <xiao.w.wang@intel.com>,
 Yangyu Chen <cyy@cyyself.name>, linux-kernel@vger.kernel.org
References: <20240313180010.295747-1-samuel.holland@sifive.com>
 <CAHVXubjLWZkjSapnsWJzimWg_2swEy7tQ-DQ6ri8yMk8-Qsc-A@mail.gmail.com>
From: Samuel Holland <samuel.holland@sifive.com>
In-Reply-To: <CAHVXubjLWZkjSapnsWJzimWg_2swEy7tQ-DQ6ri8yMk8-Qsc-A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Alex,

On 2024-03-18 3:50 PM, Alexandre Ghiti wrote:
> On Wed, Mar 13, 2024 at 7:00 PM Samuel Holland
> <samuel.holland@sifive.com> wrote:
>>
>> TASK_SIZE_MAX should be set to the largest userspace address under any
>> runtime configuration. This optimizes the check in __access_ok(), which
>> no longer needs to compute the current value of TASK_SIZE. It is still
>> safe because addresses between TASK_SIZE and TASK_SIZE_MAX are invalid
>> at the hardware level.
>>
>> This removes about half of the references to pgtable_l[45]_enabled.
>>
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>>  arch/riscv/include/asm/pgtable-64.h | 1 +
>>  arch/riscv/include/asm/pgtable.h    | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/arch/riscv/include/asm/pgtable-64.h b/arch/riscv/include/asm/pgtable-64.h
>> index b99bd66107a6..a677ef3c0fe2 100644
>> --- a/arch/riscv/include/asm/pgtable-64.h
>> +++ b/arch/riscv/include/asm/pgtable-64.h
>> @@ -17,6 +17,7 @@ extern bool pgtable_l5_enabled;
>>  #define PGDIR_SHIFT_L4  39
>>  #define PGDIR_SHIFT_L5  48
>>  #define PGDIR_SIZE_L3   (_AC(1, UL) << PGDIR_SHIFT_L3)
>> +#define PGDIR_SIZE_L5   (_AC(1, UL) << PGDIR_SHIFT_L5)
>>
>>  #define PGDIR_SHIFT     (pgtable_l5_enabled ? PGDIR_SHIFT_L5 : \
>>                 (pgtable_l4_enabled ? PGDIR_SHIFT_L4 : PGDIR_SHIFT_L3))
>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
>> index 6066822e7396..2032f8ac5fc5 100644
>> --- a/arch/riscv/include/asm/pgtable.h
>> +++ b/arch/riscv/include/asm/pgtable.h
>> @@ -867,6 +867,7 @@ 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_MIN  (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>> +#define TASK_SIZE_MAX  (PGDIR_SIZE_L5 * PTRS_PER_PGD / 2)
>>
>>  #ifdef CONFIG_COMPAT
>>  #define TASK_SIZE_32   (_AC(0x80000000, UL))
>> --
>> 2.43.1
>>
> 
> I think you also need to change the check in handle_page_fault() by
> using TASK_SIZE_MAX instead of TASK_SIZE, otherwise the fixup can't
> happen (https://elixir.bootlin.com/linux/latest/source/arch/riscv/mm/fault.c#L273).

It is not necessary to change that check in fault.c unless we expect to handle
exceptions (outside of userspace access routines) for addresses between
TASK_SIZE and TASK_SIZE_MAX. It looks like the call to fixup_exception() [added
in 416721ff05fd ("riscv, mm: Perform BPF exhandler fixup on page fault")] is
only intended to catch null pointer dereferences. So making the change wouldn't
have any functional impact, but it would still be a valid optimization.

> Or I was wondering if it would not be better to do like x86 and use an
> alternative, it would be more correct (even though I believe your
> solution works)
> https://elixir.bootlin.com/linux/latest/source/arch/x86/include/asm/page_64.h#L82.

What would be the benefit of using an alternative? Any access to an address
between TASK_SIZE and TASK_SIZE_MAX is guaranteed to generate a page fault, so
the only benefit I see is returning -EFAULT slightly faster at the cost of
applying a few hundred alternatives at boot. But it's possible I'm missing
something.

Regards,
Samuel