Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0

[Catalin Marinas <catalin.marinas@arm.com>]

On Thu, Sep 22, 2022 at 08:37:57PM +0300, Sergey Shtylyov wrote:
> On 9/22/22 2:59 PM, Will Deacon wrote:
> [...]
> >> user_regset_copyin_ignore() always return 0, so checking its result seems
> >> pointless -- don't do this...
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> >> analysis tool.
> >>
> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >>
> >> ---
> >> This patch is against the 'for-next/core' branch of the ARM64 repo...
> >>
> >>  arch/arm64/kernel/ptrace.c |   16 ++++------------
> >>  1 file changed, 4 insertions(+), 12 deletions(-)
> >>
> >> Index: linux/arch/arm64/kernel/ptrace.c
> >> ===================================================================
> >> --- linux.orig/arch/arm64/kernel/ptrace.c
> >> +++ linux/arch/arm64/kernel/ptrace.c
> >> @@ -514,9 +514,7 @@ static int hw_break_set(struct task_stru
> >>  
> >>  	/* Resource info and pad */
> >>  	offset = offsetof(struct user_hwdebug_state, dbg_regs);
> >> -	ret = user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
> >> -	if (ret)
> >> -		return ret;
> >> +	user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
> >>  
> >>  	/* (address, ctrl) registers */
> >>  	limit = regset->n * regset->size;
> >> @@ -543,11 +541,8 @@ static int hw_break_set(struct task_stru
> >>  			return ret;
> >>  		offset += PTRACE_HBP_CTRL_SZ;
> >>  
> >> -		ret = user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> >> -						offset,
> >> -						offset + PTRACE_HBP_PAD_SZ);
> >> -		if (ret)
> >> -			return ret;
> >> +		user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> >> +					  offset, offset + PTRACE_HBP_PAD_SZ);
> >>  		offset += PTRACE_HBP_PAD_SZ;
> >>  		idx++;
> >>  	}
> >> @@ -939,10 +934,7 @@ static int sve_set_common(struct task_st
> >>  
> >>  	start = end;
> >>  	end = SVE_PT_SVE_FPSR_OFFSET(vq);
> >> -	ret = user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
> >> -					start, end);
> >> -	if (ret)
> >> -		goto out;
> >> +	user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, start, end);
> > 
> > I think it would be better to have user_regset_copyin_ignore() return void
> > so that we don't run the risk of missing an error code if it starts
> > returning one in future.
> 
>    That's the plan! But I need to convert the users 1st, right?

Right, though normally we'd like to see the full series that does the
arch clean-up followed by the user_regset_copyin_ignore() return changed
to void. If for some reason the last patch is rejected by the maintainer
because there are plans to actually return some non-zero value, we'd
have to revert the above.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel