linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
@ 2022-09-20 20:54 Sergey Shtylyov
  2022-09-21  8:56 ` Sergey Shtylyov
  2022-09-22 11:59 ` Will Deacon
  0 siblings, 2 replies; 7+ messages in thread
From: Sergey Shtylyov @ 2022-09-20 20:54 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, linux-arm-kernel, Oleg Nesterov; +Cc: lvc-project

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);
 
 	/*
 	 * Copy fpsr, and fpcr which must follow contiguously in

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
  2022-09-20 20:54 [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0 Sergey Shtylyov
@ 2022-09-21  8:56 ` Sergey Shtylyov
  2022-09-22 11:59 ` Will Deacon
  1 sibling, 0 replies; 7+ messages in thread
From: Sergey Shtylyov @ 2022-09-21  8:56 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon, linux-arm-kernel, Oleg Nesterov; +Cc: lvc-project

On 9/20/22 11:54 PM, Sergey Shtylyov wrote:

> user_regset_copyin_ignore() always return 0, so checking its result seems

   Oops, should've been "returns"...

> 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>
[...]

MBR, Sergey

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
  2022-09-20 20:54 [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0 Sergey Shtylyov
  2022-09-21  8:56 ` Sergey Shtylyov
@ 2022-09-22 11:59 ` Will Deacon
  2022-09-22 17:37   ` Sergey Shtylyov
  1 sibling, 1 reply; 7+ messages in thread
From: Will Deacon @ 2022-09-22 11:59 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Catalin Marinas, linux-arm-kernel, Oleg Nesterov, lvc-project

On Tue, Sep 20, 2022 at 11:54:55PM +0300, Sergey Shtylyov 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.

Will

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
  2022-09-22 11:59 ` Will Deacon
@ 2022-09-22 17:37   ` Sergey Shtylyov
  2022-09-26 14:01     ` Catalin Marinas
  0 siblings, 1 reply; 7+ messages in thread
From: Sergey Shtylyov @ 2022-09-22 17:37 UTC (permalink / raw)
  To: Will Deacon; +Cc: Catalin Marinas, linux-arm-kernel, Oleg Nesterov, lvc-project

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?

> Will

MBR, Sergey

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
  2022-09-22 17:37   ` Sergey Shtylyov
@ 2022-09-26 14:01     ` Catalin Marinas
  2022-09-27 18:46       ` Sergey Shtylyov
  0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2022-09-26 14:01 UTC (permalink / raw)
  To: Sergey Shtylyov; +Cc: Will Deacon, linux-arm-kernel, Oleg Nesterov, lvc-project

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
  2022-09-26 14:01     ` Catalin Marinas
@ 2022-09-27 18:46       ` Sergey Shtylyov
  2022-09-29 17:11         ` Catalin Marinas
  0 siblings, 1 reply; 7+ messages in thread
From: Sergey Shtylyov @ 2022-09-27 18:46 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: Will Deacon, linux-arm-kernel, Oleg Nesterov, lvc-project

Hello!

On 9/26/22 5:01 PM, Catalin Marinas 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.

   Hm... I usually try to avoid a cross-tree patch series but here I should
be able to use the linux-next repo as I series' base. Is that OK?

MBR, Sergey

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0
  2022-09-27 18:46       ` Sergey Shtylyov
@ 2022-09-29 17:11         ` Catalin Marinas
  0 siblings, 0 replies; 7+ messages in thread
From: Catalin Marinas @ 2022-09-29 17:11 UTC (permalink / raw)
  To: Sergey Shtylyov; +Cc: Will Deacon, linux-arm-kernel, Oleg Nesterov, lvc-project

On Tue, Sep 27, 2022 at 09:46:11PM +0300, Sergey Shtylyov wrote:
> On 9/26/22 5:01 PM, Catalin Marinas wrote:
> >>>> user_regset_copyin_ignore() always return 0, so checking its result seems
> >>>> pointless -- don't do this...
[...]
> >>> 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.
> 
>    Hm... I usually try to avoid a cross-tree patch series but here I should
> be able to use the linux-next repo as I series' base. Is that OK?

It depends on how you plan to merge it, though basing the series off
linux-next is not ideal. It's fine to merge individual patches in one
release though specific trees and the final patch in another but when
you posted this single patch we didn't have the context. So ideally I'd
like to see an ack on the patch converting user_regset_copyin_ignore()
to void. It wasn't obvious there's such patch.

You can also have a cross-tree series and convince one of the maintainer
to pick it up (usually cc'ing akpm does the trick ;)).

-- 
Catalin

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-29 17:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20 20:54 [PATCH] arm64: ptrace: user_regset_copyin_ignore() always returns 0 Sergey Shtylyov
2022-09-21  8:56 ` Sergey Shtylyov
2022-09-22 11:59 ` Will Deacon
2022-09-22 17:37   ` Sergey Shtylyov
2022-09-26 14:01     ` Catalin Marinas
2022-09-27 18:46       ` Sergey Shtylyov
2022-09-29 17:11         ` Catalin Marinas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).