On Fri, Apr 03, 2020 at 05:13:25PM +1000, Nicholas Piggin wrote: > Michal Suchánek's on March 25, 2020 5:38 am: > > On Tue, Mar 24, 2020 at 06:48:20PM +1000, Nicholas Piggin wrote: > >> Michal Suchanek's on March 19, 2020 10:19 pm: > >> > There are two almost identical copies for 32bit and 64bit. > >> > > >> > The function is used only in 32bit code which will be split out in next > >> > patch so consolidate to one function. > >> > > >> > Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx> > >> > Reviewed-by: Christophe Leroy <christophe.leroy@xxxxxx> > >> > --- > >> > v6: new patch > >> > v8: move the consolidated function out of the ifdef block. > >> > v11: rebase on top of def0bfdbd603 > >> > --- > >> > arch/powerpc/perf/callchain.c | 48 +++++++++++++++++------------------ > >> > 1 file changed, 24 insertions(+), 24 deletions(-) > >> > > >> > diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c > >> > index cbc251981209..c9a78c6e4361 100644 > >> > --- a/arch/powerpc/perf/callchain.c > >> > +++ b/arch/powerpc/perf/callchain.c > >> > @@ -161,18 +161,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret) > >> > return read_user_stack_slow(ptr, ret, 8); > >> > } > >> > > >> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret) > >> > -{ > >> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) || > >> > - ((unsigned long)ptr & 3)) > >> > - return -EFAULT; > >> > - > >> > - if (!probe_user_read(ret, ptr, sizeof(*ret))) > >> > - return 0; > >> > - > >> > - return read_user_stack_slow(ptr, ret, 4); > >> > -} > >> > - > >> > static inline int valid_user_sp(unsigned long sp, int is_64) > >> > { > >> > if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32) > >> > @@ -277,19 +265,9 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry, > >> > } > >> > > >> > #else /* CONFIG_PPC64 */ > >> > -/* > >> > - * On 32-bit we just access the address and let hash_page create a > >> > - * HPTE if necessary, so there is no need to fall back to reading > >> > - * the page tables. Since this is called at interrupt level, > >> > - * do_page_fault() won't treat a DSI as a page fault. > >> > - */ > >> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret) > >> > +static int read_user_stack_slow(void __user *ptr, void *buf, int nb) > >> > { > >> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) || > >> > - ((unsigned long)ptr & 3)) > >> > - return -EFAULT; > >> > - > >> > - return probe_user_read(ret, ptr, sizeof(*ret)); > >> > + return 0; > >> > } > >> > > >> > static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry, > >> > @@ -312,6 +290,28 @@ static inline int valid_user_sp(unsigned long sp, int is_64) > >> > > >> > #endif /* CONFIG_PPC64 */ > >> > > >> > +/* > >> > + * On 32-bit we just access the address and let hash_page create a > >> > + * HPTE if necessary, so there is no need to fall back to reading > >> > + * the page tables. Since this is called at interrupt level, > >> > + * do_page_fault() won't treat a DSI as a page fault. > >> > + */ > >> > >> The comment is actually probably better to stay in the 32-bit > >> read_user_stack_slow implementation. Is that function defined > >> on 32-bit purely so that you can use IS_ENABLED()? In that case > > It documents the IS_ENABLED() and that's where it is. The 32bit > > definition is only a technical detail. > > Sorry for the late reply, busy trying to fix bugs in the C rewrite > series. I don't think it is the right place, it should be in the > ppc32 implementation detail. Which does not exist anymore after the 32bit and 64bit part is split. > ppc64 has an equivalent comment at the top of its read_user_stack functions. > > >> I would prefer to put a BUG() there which makes it self documenting. > > Which will cause checkpatch complaints about introducing new BUG() which > > is frowned on. > > It's fine in this case, that warning is about not introducing > runtime bugs, but this wouldn't be. > > But... I actually don't like adding read_user_stack_slow on 32-bit > and especially not just to make IS_ENABLED work. That's to not break build at this point. Later the function is removed. > > IMO this would be better if you really want to consolidate it > > --- > > diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c > index cbc251981209..ca3a599b3f54 100644 > --- a/arch/powerpc/perf/callchain.c > +++ b/arch/powerpc/perf/callchain.c > @@ -108,7 +108,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re > * interrupt context, so if the access faults, we read the page tables > * to find which page (if any) is mapped and access it directly. > */ > -static int read_user_stack_slow(void __user *ptr, void *buf, int nb) > +static int read_user_stack_slow(const void __user *ptr, void *buf, int nb) > { > int ret = -EFAULT; > pgd_t *pgdir; > @@ -149,28 +149,21 @@ static int read_user_stack_slow(void __user *ptr, void *buf, int nb) > return ret; > } > > -static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret) > +static int __read_user_stack(const void __user *ptr, void *ret, size_t size) > { > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned long) || > - ((unsigned long)ptr & 7)) > + if ((unsigned long)ptr > TASK_SIZE - size || > + ((unsigned long)ptr & (size - 1))) > return -EFAULT; > > - if (!probe_user_read(ret, ptr, sizeof(*ret))) > + if (!probe_user_read(ret, ptr, size)) > return 0; > > - return read_user_stack_slow(ptr, ret, 8); > + return read_user_stack_slow(ptr, ret, size); > } > > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret) > +static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret) > { > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) || > - ((unsigned long)ptr & 3)) > - return -EFAULT; > - > - if (!probe_user_read(ret, ptr, sizeof(*ret))) > - return 0; > - > - return read_user_stack_slow(ptr, ret, 4); > + return __read_user_stack(ptr, ret, sizeof(*ret)); > } > > static inline int valid_user_sp(unsigned long sp, int is_64) > @@ -283,13 +276,13 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry, > * the page tables. Since this is called at interrupt level, > * do_page_fault() won't treat a DSI as a page fault. > */ > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret) > +static int __read_user_stack(const void __user *ptr, void *ret, size_t size) > { > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) || > - ((unsigned long)ptr & 3)) > + if ((unsigned long)ptr > TASK_SIZE - size || > + ((unsigned long)ptr & (size - 1))) > return -EFAULT; > > - return probe_user_read(ret, ptr, sizeof(*ret)); > + return probe_user_read(ret, ptr, size); > } > > static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry, > @@ -312,6 +305,11 @@ static inline int valid_user_sp(unsigned long sp, int is_64) > > #endif /* CONFIG_PPC64 */ > > +static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret) > +{ > + return __read_user_stack(ptr, ret, sizeof(*ret)); > +} > + > /* > * Layout for non-RT signal frames > */