Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 09 Mar 2021 19:16:45 -0800 (PST)
Subject:     Re: [PATCH v2] riscv: Return -EFAULT if copy_{to,from}_user() failed in signal.c
In-Reply-To: <1615017149-24843-1-git-send-email-yangtiezhu@loongson.cn>
CC:     Paul Walmsley <paul.walmsley@sifive.com>, aou@eecs.berkeley.edu,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     yangtiezhu@loongson.cn
Message-ID: <mhng-33c9663e-90b5-48a8-b089-54f16f6b494a@penguin>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 05 Mar 2021 23:52:29 PST (-0800), yangtiezhu@loongson.cn wrote:
> copy_{to,from}_user() returns the amount left to copy, it should return
> -EFAULT error code if copy {to,from} user failed, just like the return
> value is an error code when {put,get}_user() failed, this is to make the
> return value consistent, no function change.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/riscv/kernel/signal.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index 65942b3..c76d877 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -39,7 +39,7 @@ static long restore_fp_state(struct pt_regs *regs,
>
>  	err = __copy_from_user(&current->thread.fstate, state, sizeof(*state));
>  	if (unlikely(err))
> -		return err;
> +		return -EFAULT;
>
>  	fstate_restore(current, regs);
>
> @@ -67,7 +67,7 @@ static long save_fp_state(struct pt_regs *regs,
>  	fstate_save(current, regs);
>  	err = __copy_to_user(state, &current->thread.fstate, sizeof(*state));
>  	if (unlikely(err))
> -		return err;
> +		return -EFAULT;
>
>  	/* We support no other extension state at this time. */
>  	for (i = 0; i < ARRAY_SIZE(sc_fpregs->q.reserved); i++) {
> @@ -87,8 +87,12 @@ static long restore_sigcontext(struct pt_regs *regs,
>  	struct sigcontext __user *sc)
>  {
>  	long err;
> +
>  	/* sc_regs is structured the same as the start of pt_regs */
>  	err = __copy_from_user(regs, &sc->sc_regs, sizeof(sc->sc_regs));
> +	if (unlikely(err))
> +		return -EFAULT;
> +
>  	/* Restore the floating-point state. */
>  	if (has_fpu)
>  		err |= restore_fp_state(regs, &sc->sc_fpregs);
> @@ -140,8 +144,12 @@ static long setup_sigcontext(struct rt_sigframe __user *frame,
>  {
>  	struct sigcontext __user *sc = &frame->uc.uc_mcontext;
>  	long err;
> +
>  	/* sc_regs is structured the same as the start of pt_regs */
>  	err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
> +	if (unlikely(err))
> +		return -EFAULT;
> +
>  	/* Save the floating-point state. */
>  	if (has_fpu)
>  		err |= save_fp_state(regs, &sc->sc_fpregs);

I don't really see any benefit to this way of doing it over what's there: these 
are only used within this file, and the caller is just doing this return 
conversion already.  If anything I find the current code easier to understand, 
as error juggling is always one of the trickier things to get right and I 
always find it easier to reason about code that's just passing through errors.

If you have some new user of this code where it makes more sense to do it this 
way then I'd be happy to take a look, but this as it stands doesn't really look 
better.