| Message ID | 4c678cd1-7e75-5c73-0de8-2ffaa3a96e0e@omp.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ptrace: user_regset_copyin_ignore() always returns 0 | expand |
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
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
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
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.
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
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 ;)).
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
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(-)