| Message ID | 20210128040424.12720-3-cmr@codefail.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve signal performance on PPC64 with KUAP | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (44158b256b30415079588d0fcb1bccbdc2ccd009) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 51 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
From: Christopher M. Riedl > Sent: 28 January 2021 04:04 > > Reuse the "safe" implementation from signal.c except for calling > unsafe_copy_from_user() to copy into a local buffer. > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > --- > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h > index 2559a681536e..c18402d625f1 100644 > --- a/arch/powerpc/kernel/signal.h > +++ b/arch/powerpc/kernel/signal.h > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > &buf[i], label);\ > } while (0) > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ > + struct task_struct *__t = task; \ > + u64 __user *__f = (u64 __user *)from; \ > + u64 buf[ELF_NFPREG]; \ How big is that buffer? Isn't is likely to be reasonably large compared to a reasonable kernel stack frame. Especially since this isn't even a leaf function. > + int i; \ > + \ > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ That really ought to be sizeof(buf). David > + label); \ > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > + __t->thread.TS_FPR(i) = buf[i]; \ > + __t->thread.fp_state.fpscr = buf[i]; \ > +} while (0) > + > +#define unsafe_copy_vsx_from_user(task, from, label) do { \ > + struct task_struct *__t = task; \ > + u64 __user *__f = (u64 __user *)from; \ > + u64 buf[ELF_NVSRHALFREG]; \ > + int i; \ > + \ > + unsafe_copy_from_user(buf, __f, \ > + ELF_NVSRHALFREG * sizeof(double), \ > + label); \ > + for (i = 0; i < ELF_NVSRHALFREG ; i++) \ > + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \ > +} while (0) > + > + > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > #define unsafe_copy_ckfpr_to_user(to, task, label) do { \ > struct task_struct *__t = task; \ > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \ > ELF_NFPREG * sizeof(double), label) > > +#define unsafe_copy_fpr_from_user(task, from, label) \ > + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \ > + ELF_NFPREG * sizeof(double), label) > + > static inline unsigned long > copy_fpr_to_user(void __user *to, struct task_struct *task) > { > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from) > #else > #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0) > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0) > + > static inline unsigned long > copy_fpr_to_user(void __user *to, struct task_struct *task) > { > -- > 2.26.1 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Le 28/01/2021 à 11:38, David Laight a écrit : > From: Christopher M. Riedl >> Sent: 28 January 2021 04:04 >> >> Reuse the "safe" implementation from signal.c except for calling >> unsafe_copy_from_user() to copy into a local buffer. >> >> Signed-off-by: Christopher M. Riedl <cmr@codefail.de> >> --- >> arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h >> index 2559a681536e..c18402d625f1 100644 >> --- a/arch/powerpc/kernel/signal.h >> +++ b/arch/powerpc/kernel/signal.h >> @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); >> &buf[i], label);\ >> } while (0) >> >> +#define unsafe_copy_fpr_from_user(task, from, label) do { \ >> + struct task_struct *__t = task; \ >> + u64 __user *__f = (u64 __user *)from; \ >> + u64 buf[ELF_NFPREG]; \ > > How big is that buffer? #define ELF_NFPREG 33 So that's 264 bytes. That's a bit big but still reasonable I think. Christophe > Isn't is likely to be reasonably large compared to a reasonable > kernel stack frame. > Especially since this isn't even a leaf function. > >> + int i; \ >> + \ >> + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > That really ought to be sizeof(buf). > > David > > >> + label); \ >> + for (i = 0; i < ELF_NFPREG - 1; i++) \ >> + __t->thread.TS_FPR(i) = buf[i]; \ >> + __t->thread.fp_state.fpscr = buf[i]; \ >> +} while (0) >> + >> +#define unsafe_copy_vsx_from_user(task, from, label) do { \ >> + struct task_struct *__t = task; \ >> + u64 __user *__f = (u64 __user *)from; \ >> + u64 buf[ELF_NVSRHALFREG]; \ >> + int i; \ >> + \ >> + unsafe_copy_from_user(buf, __f, \ >> + ELF_NVSRHALFREG * sizeof(double), \ >> + label); \ >> + for (i = 0; i < ELF_NVSRHALFREG ; i++) \ >> + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \ >> +} while (0) >> + >> + >> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> #define unsafe_copy_ckfpr_to_user(to, task, label) do { \ >> struct task_struct *__t = task; \ >> @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); >> unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \ >> ELF_NFPREG * sizeof(double), label) >> >> +#define unsafe_copy_fpr_from_user(task, from, label) \ >> + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \ >> + ELF_NFPREG * sizeof(double), label) >> + >> static inline unsigned long >> copy_fpr_to_user(void __user *to, struct task_struct *task) >> { >> @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from) >> #else >> #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0) >> >> +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0) >> + >> static inline unsigned long >> copy_fpr_to_user(void __user *to, struct task_struct *task) >> { >> -- >> 2.26.1 > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote: > From: Christopher M. Riedl > > Sent: 28 January 2021 04:04 > > > > Reuse the "safe" implementation from signal.c except for calling > > unsafe_copy_from_user() to copy into a local buffer. > > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > --- > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h > > index 2559a681536e..c18402d625f1 100644 > > --- a/arch/powerpc/kernel/signal.h > > +++ b/arch/powerpc/kernel/signal.h > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > &buf[i], label);\ > > } while (0) > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ > > + struct task_struct *__t = task; \ > > + u64 __user *__f = (u64 __user *)from; \ > > + u64 buf[ELF_NFPREG]; \ > > How big is that buffer? > Isn't is likely to be reasonably large compared to a reasonable > kernel stack frame. > Especially since this isn't even a leaf function. > I think Christophe answered this - I don't really have an opinion either way. What would be a 'reasonable' kernel stack frame for reference? > > + int i; \ > > + \ > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > That really ought to be sizeof(buf). > Agreed, I will fix this. Thanks! > David > > > > + label); \ > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > + __t->thread.TS_FPR(i) = buf[i]; \ > > + __t->thread.fp_state.fpscr = buf[i]; \ > > +} while (0) > > + > > +#define unsafe_copy_vsx_from_user(task, from, label) do { \ > > + struct task_struct *__t = task; \ > > + u64 __user *__f = (u64 __user *)from; \ > > + u64 buf[ELF_NVSRHALFREG]; \ > > + int i; \ > > + \ > > + unsafe_copy_from_user(buf, __f, \ > > + ELF_NVSRHALFREG * sizeof(double), \ > > + label); \ > > + for (i = 0; i < ELF_NVSRHALFREG ; i++) \ > > + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \ > > +} while (0) > > + > > + > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > #define unsafe_copy_ckfpr_to_user(to, task, label) do { \ > > struct task_struct *__t = task; \ > > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \ > > ELF_NFPREG * sizeof(double), label) > > > > +#define unsafe_copy_fpr_from_user(task, from, label) \ > > + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \ > > + ELF_NFPREG * sizeof(double), label) > > + > > static inline unsigned long > > copy_fpr_to_user(void __user *to, struct task_struct *task) > > { > > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from) > > #else > > #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0) > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0) > > + > > static inline unsigned long > > copy_fpr_to_user(void __user *to, struct task_struct *task) > > { > > -- > > 2.26.1 > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Christopher M. Riedl > Sent: 01 February 2021 15:56 > > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote: > > From: Christopher M. Riedl > > > Sent: 28 January 2021 04:04 > > > > > > Reuse the "safe" implementation from signal.c except for calling > > > unsafe_copy_from_user() to copy into a local buffer. > > > > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > > --- > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ > > > 1 file changed, 33 insertions(+) > > > > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h > > > index 2559a681536e..c18402d625f1 100644 > > > --- a/arch/powerpc/kernel/signal.h > > > +++ b/arch/powerpc/kernel/signal.h > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > > &buf[i], label);\ > > > } while (0) > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ > > > + struct task_struct *__t = task; \ > > > + u64 __user *__f = (u64 __user *)from; \ > > > + u64 buf[ELF_NFPREG]; \ > > > > How big is that buffer? > > Isn't is likely to be reasonably large compared to a reasonable > > kernel stack frame. > > Especially since this isn't even a leaf function. > > > > I think Christophe answered this - I don't really have an opinion either > way. What would be a 'reasonable' kernel stack frame for reference? Zero :-) > > > > + int i; \ > > > + \ > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > > + label); \ > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > > + __t->thread.TS_FPR(i) = buf[i]; \ > > > + __t->thread.fp_state.fpscr = buf[i]; \ > > > +} while (0) On further reflection, since you immediately loop through the buffer why not just use user_access_begin() and unsafe_get_user() in the loop. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote: > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote: > > From: Christopher M. Riedl > > > Sent: 28 January 2021 04:04 > > > > > > Reuse the "safe" implementation from signal.c except for calling > > > unsafe_copy_from_user() to copy into a local buffer. > > > > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > > --- > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ > > > 1 file changed, 33 insertions(+) > > > > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h > > > index 2559a681536e..c18402d625f1 100644 > > > --- a/arch/powerpc/kernel/signal.h > > > +++ b/arch/powerpc/kernel/signal.h > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > > &buf[i], label);\ > > > } while (0) > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ > > > + struct task_struct *__t = task; \ > > > + u64 __user *__f = (u64 __user *)from; \ > > > + u64 buf[ELF_NFPREG]; \ > > > > How big is that buffer? > > Isn't is likely to be reasonably large compared to a reasonable > > kernel stack frame. > > Especially since this isn't even a leaf function. > > > > I think Christophe answered this - I don't really have an opinion either > way. What would be a 'reasonable' kernel stack frame for reference? See include/linux/poll.h, where the limit is of the order of 800 bytes and the number of entries in an on stack array is chosen at compile time (different between 32 and 64 bit for example). The values are used in do_sys_poll, which, with almost 1000 bytes of stack footprint, appears close to the top of "make checkstack". In addition do_sys_poll has to call the ->poll function of every file descriptor in its table, so it is not a tail function. This 264 bytes array looks reasonable, but please use 'make checkstack' to verify that the function's total stack usage stays within reason. Gabriel > > > > + int i; \ > > > + \ > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > > > That really ought to be sizeof(buf). > > > > Agreed, I will fix this. Thanks! > > > David > > > > > > > + label); \ > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > > + __t->thread.TS_FPR(i) = buf[i]; \ > > > + __t->thread.fp_state.fpscr = buf[i]; \ > > > +} while (0) > > > + > > > +#define unsafe_copy_vsx_from_user(task, from, label) do { \ > > > + struct task_struct *__t = task; \ > > > + u64 __user *__f = (u64 __user *)from; \ > > > + u64 buf[ELF_NVSRHALFREG]; \ > > > + int i; \ > > > + \ > > > + unsafe_copy_from_user(buf, __f, \ > > > + ELF_NVSRHALFREG * sizeof(double), \ > > > + label); \ > > > + for (i = 0; i < ELF_NVSRHALFREG ; i++) \ > > > + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \ > > > +} while (0) > > > + > > > + > > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > > #define unsafe_copy_ckfpr_to_user(to, task, label) do { \ > > > struct task_struct *__t = task; \ > > > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > > unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \ > > > ELF_NFPREG * sizeof(double), label) > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) \ > > > + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \ > > > + ELF_NFPREG * sizeof(double), label) > > > + > > > static inline unsigned long > > > copy_fpr_to_user(void __user *to, struct task_struct *task) > > > { > > > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from) > > > #else > > > #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0) > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0) > > > + > > > static inline unsigned long > > > copy_fpr_to_user(void __user *to, struct task_struct *task) > > > { > > > -- > > > 2.26.1 > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > > MK1 1PT, UK > > Registration No: 1397386 (Wales) >
On Mon Feb 1, 2021 at 10:15 AM CST, David Laight wrote: > From: Christopher M. Riedl > > Sent: 01 February 2021 15:56 > > > > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote: > > > From: Christopher M. Riedl > > > > Sent: 28 January 2021 04:04 > > > > > > > > Reuse the "safe" implementation from signal.c except for calling > > > > unsafe_copy_from_user() to copy into a local buffer. > > > > > > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > > > --- > > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ > > > > 1 file changed, 33 insertions(+) > > > > > > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h > > > > index 2559a681536e..c18402d625f1 100644 > > > > --- a/arch/powerpc/kernel/signal.h > > > > +++ b/arch/powerpc/kernel/signal.h > > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > > > &buf[i], label);\ > > > > } while (0) > > > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ > > > > + struct task_struct *__t = task; \ > > > > + u64 __user *__f = (u64 __user *)from; \ > > > > + u64 buf[ELF_NFPREG]; \ > > > > > > How big is that buffer? > > > Isn't is likely to be reasonably large compared to a reasonable > > > kernel stack frame. > > > Especially since this isn't even a leaf function. > > > > > > > I think Christophe answered this - I don't really have an opinion either > > way. What would be a 'reasonable' kernel stack frame for reference? > > Zero :-) > Hehe good point! > > > > > > + int i; \ > > > > + \ > > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > > > + label); \ > > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > > > + __t->thread.TS_FPR(i) = buf[i]; \ > > > > + __t->thread.fp_state.fpscr = buf[i]; \ > > > > +} while (0) > > On further reflection, since you immediately loop through the buffer > why not just use user_access_begin() and unsafe_get_user() in the loop. Christophe had suggested this a few revisions ago as well. When I tried this approach, the signal handling performance took a pretty big hit: https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html I included some numbers on v3 as well but decided to drop the approach altogether for this one since it just didn't seem worth the hit. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Christopher M. Riedl > Sent: 01 February 2021 16:55 ... > > > > > + int i; \ > > > > > + \ > > > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > > > > + label); \ > > > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > > > > + __t->thread.TS_FPR(i) = buf[i]; \ > > > > > + __t->thread.fp_state.fpscr = buf[i]; \ > > > > > +} while (0) > > > > On further reflection, since you immediately loop through the buffer > > why not just use user_access_begin() and unsafe_get_user() in the loop. > > Christophe had suggested this a few revisions ago as well. When I tried > this approach, the signal handling performance took a pretty big hit: > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html > > I included some numbers on v3 as well but decided to drop the approach > altogether for this one since it just didn't seem worth the hit. Was that using unsafe_get_user (which relies on user_access_begin() having 'opened up' user accesses) or just get_user() that does it for every access? The former should be ok, the latter will be horrid. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon Feb 1, 2021 at 11:37 AM CST, David Laight wrote: > From: Christopher M. Riedl > > Sent: 01 February 2021 16:55 > ... > > > > > > + int i; \ > > > > > > + \ > > > > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > > > > > + label); \ > > > > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > > > > > + __t->thread.TS_FPR(i) = buf[i]; \ > > > > > > + __t->thread.fp_state.fpscr = buf[i]; \ > > > > > > +} while (0) > > > > > > On further reflection, since you immediately loop through the buffer > > > why not just use user_access_begin() and unsafe_get_user() in the loop. > > > > Christophe had suggested this a few revisions ago as well. When I tried > > this approach, the signal handling performance took a pretty big hit: > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html > > > > I included some numbers on v3 as well but decided to drop the approach > > altogether for this one since it just didn't seem worth the hit. > > Was that using unsafe_get_user (which relies on user_access_begin() > having 'opened up' user accesses) or just get_user() that does > it for every access? > > The former should be ok, the latter will be horrid. It was using unsafe_get_user() whereas unsafe_copy_from_user() will just call the optimized __copy_tofrom_user() a single time - assuming that user access is open. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > MK1 1PT, UK > Registration No: 1397386 (Wales)
On Mon Feb 1, 2021 at 10:54 AM CST, Gabriel Paubert wrote: > On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote: > > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote: > > > From: Christopher M. Riedl > > > > Sent: 28 January 2021 04:04 > > > > > > > > Reuse the "safe" implementation from signal.c except for calling > > > > unsafe_copy_from_user() to copy into a local buffer. > > > > > > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > > > --- > > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ > > > > 1 file changed, 33 insertions(+) > > > > > > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h > > > > index 2559a681536e..c18402d625f1 100644 > > > > --- a/arch/powerpc/kernel/signal.h > > > > +++ b/arch/powerpc/kernel/signal.h > > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > > > &buf[i], label);\ > > > > } while (0) > > > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ > > > > + struct task_struct *__t = task; \ > > > > + u64 __user *__f = (u64 __user *)from; \ > > > > + u64 buf[ELF_NFPREG]; \ > > > > > > How big is that buffer? > > > Isn't is likely to be reasonably large compared to a reasonable > > > kernel stack frame. > > > Especially since this isn't even a leaf function. > > > > > > > I think Christophe answered this - I don't really have an opinion either > > way. What would be a 'reasonable' kernel stack frame for reference? > > See include/linux/poll.h, where the limit is of the order of 800 bytes > and the number of entries in an on stack array is chosen at compile time > (different between 32 and 64 bit for example). > > The values are used in do_sys_poll, which, with almost 1000 bytes of > stack footprint, appears close to the top of "make checkstack". In > addition do_sys_poll has to call the ->poll function of every file > descriptor in its table, so it is not a tail function. > > This 264 bytes array looks reasonable, but please use 'make checkstack' > to verify that the function's total stack usage stays within reason. Neat, looks like total usage is a bit larger but still reasonable and less than half of 800B: 0xc000000000017e900 __unsafe_restore_sigcontext.constprop.0 [vmlinux]:352 Thanks for the tip! > > Gabriel > > > > > > > + int i; \ > > > > + \ > > > > + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ > > > > > > That really ought to be sizeof(buf). > > > > > > > Agreed, I will fix this. Thanks! > > > > > David > > > > > > > > > > + label); \ > > > > + for (i = 0; i < ELF_NFPREG - 1; i++) \ > > > > + __t->thread.TS_FPR(i) = buf[i]; \ > > > > + __t->thread.fp_state.fpscr = buf[i]; \ > > > > +} while (0) > > > > + > > > > +#define unsafe_copy_vsx_from_user(task, from, label) do { \ > > > > + struct task_struct *__t = task; \ > > > > + u64 __user *__f = (u64 __user *)from; \ > > > > + u64 buf[ELF_NVSRHALFREG]; \ > > > > + int i; \ > > > > + \ > > > > + unsafe_copy_from_user(buf, __f, \ > > > > + ELF_NVSRHALFREG * sizeof(double), \ > > > > + label); \ > > > > + for (i = 0; i < ELF_NVSRHALFREG ; i++) \ > > > > + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \ > > > > +} while (0) > > > > + > > > > + > > > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > > > #define unsafe_copy_ckfpr_to_user(to, task, label) do { \ > > > > struct task_struct *__t = task; \ > > > > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); > > > > unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \ > > > > ELF_NFPREG * sizeof(double), label) > > > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) \ > > > > + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \ > > > > + ELF_NFPREG * sizeof(double), label) > > > > + > > > > static inline unsigned long > > > > copy_fpr_to_user(void __user *to, struct task_struct *task) > > > > { > > > > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from) > > > > #else > > > > #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0) > > > > > > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0) > > > > + > > > > static inline unsigned long > > > > copy_fpr_to_user(void __user *to, struct task_struct *task) > > > > { > > > > -- > > > > 2.26.1 > > > > > > - > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > > > MK1 1PT, UK > > > Registration No: 1397386 (Wales) > > >
"Christopher M. Riedl" <cmr@codefail.de> writes: > On Mon Feb 1, 2021 at 10:54 AM CST, Gabriel Paubert wrote: >> On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote: >> > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote: >> > > From: Christopher M. Riedl >> > > > Sent: 28 January 2021 04:04 >> > > > >> > > > Reuse the "safe" implementation from signal.c except for calling >> > > > unsafe_copy_from_user() to copy into a local buffer. >> > > > >> > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> >> > > > --- >> > > > arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ >> > > > 1 file changed, 33 insertions(+) >> > > > >> > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h >> > > > index 2559a681536e..c18402d625f1 100644 >> > > > --- a/arch/powerpc/kernel/signal.h >> > > > +++ b/arch/powerpc/kernel/signal.h >> > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); >> > > > &buf[i], label);\ >> > > > } while (0) >> > > > >> > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { \ >> > > > + struct task_struct *__t = task; \ >> > > > + u64 __user *__f = (u64 __user *)from; \ >> > > > + u64 buf[ELF_NFPREG]; \ >> > > >> > > How big is that buffer? >> > > Isn't is likely to be reasonably large compared to a reasonable >> > > kernel stack frame. >> > > Especially since this isn't even a leaf function. >> > > >> > >> > I think Christophe answered this - I don't really have an opinion either >> > way. What would be a 'reasonable' kernel stack frame for reference? >> >> See include/linux/poll.h, where the limit is of the order of 800 bytes >> and the number of entries in an on stack array is chosen at compile time >> (different between 32 and 64 bit for example). >> >> The values are used in do_sys_poll, which, with almost 1000 bytes of >> stack footprint, appears close to the top of "make checkstack". In >> addition do_sys_poll has to call the ->poll function of every file >> descriptor in its table, so it is not a tail function. >> >> This 264 bytes array looks reasonable, but please use 'make checkstack' >> to verify that the function's total stack usage stays within reason. > > Neat, looks like total usage is a bit larger but still reasonable and > less than half of 800B: > > 0xc000000000017e900 __unsafe_restore_sigcontext.constprop.0 [vmlinux]:352 We warn for frames larger than 2KB on 64-bit, see FRAME_WARN in lib/Kconfig.debug. So 264 bytes is entirely reasonable IMHO. cheers
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h index 2559a681536e..c18402d625f1 100644 --- a/arch/powerpc/kernel/signal.h +++ b/arch/powerpc/kernel/signal.h @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); &buf[i], label);\ } while (0) +#define unsafe_copy_fpr_from_user(task, from, label) do { \ + struct task_struct *__t = task; \ + u64 __user *__f = (u64 __user *)from; \ + u64 buf[ELF_NFPREG]; \ + int i; \ + \ + unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double), \ + label); \ + for (i = 0; i < ELF_NFPREG - 1; i++) \ + __t->thread.TS_FPR(i) = buf[i]; \ + __t->thread.fp_state.fpscr = buf[i]; \ +} while (0) + +#define unsafe_copy_vsx_from_user(task, from, label) do { \ + struct task_struct *__t = task; \ + u64 __user *__f = (u64 __user *)from; \ + u64 buf[ELF_NVSRHALFREG]; \ + int i; \ + \ + unsafe_copy_from_user(buf, __f, \ + ELF_NVSRHALFREG * sizeof(double), \ + label); \ + for (i = 0; i < ELF_NVSRHALFREG ; i++) \ + __t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i]; \ +} while (0) + + #ifdef CONFIG_PPC_TRANSACTIONAL_MEM #define unsafe_copy_ckfpr_to_user(to, task, label) do { \ struct task_struct *__t = task; \ @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from); unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \ ELF_NFPREG * sizeof(double), label) +#define unsafe_copy_fpr_from_user(task, from, label) \ + unsafe_copy_from_user((task)->thread.fp_state.fpr, from, \ + ELF_NFPREG * sizeof(double), label) + static inline unsigned long copy_fpr_to_user(void __user *to, struct task_struct *task) { @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from) #else #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0) +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0) + static inline unsigned long copy_fpr_to_user(void __user *to, struct task_struct *task) {
Reuse the "safe" implementation from signal.c except for calling unsafe_copy_from_user() to copy into a local buffer. Signed-off-by: Christopher M. Riedl <cmr@codefail.de> --- arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)