* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.