| Message ID | 20200429070303.17599-2-jk@ozlabs.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] powerpc/spufs: fix copy_to_user while atomic | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (54dc28ff5e0b3585224d49a31b53e030342ca5c3) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | warning | Upstream build failed, couldn't test patch |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 123 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Le 29/04/2020 à 09:03, Jeremy Kerr a écrit : > From: Christoph Hellwig <hch@lst.de> > > Just use the proper non __-prefixed get/put_user variants where that is > not done yet. But it means you are doing the access_ok() check everytime, which is what is to be avoided by doing the access_ok() once then using the __-prefixed variant. Christophe > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > --- > arch/powerpc/platforms/cell/spufs/file.c | 42 +++++------------------- > 1 file changed, 8 insertions(+), 34 deletions(-) > > diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c > index b4e1ef650b40..cd7d10f27fad 100644 > --- a/arch/powerpc/platforms/cell/spufs/file.c > +++ b/arch/powerpc/platforms/cell/spufs/file.c > @@ -590,17 +590,12 @@ static ssize_t spufs_mbox_read(struct file *file, char __user *buf, > size_t len, loff_t *pos) > { > struct spu_context *ctx = file->private_data; > - u32 mbox_data, __user *udata; > + u32 mbox_data, __user *udata = (void __user *)buf; > ssize_t count; > > if (len < 4) > return -EINVAL; > > - if (!access_ok(buf, len)) > - return -EFAULT; > - > - udata = (void __user *)buf; > - > count = spu_acquire(ctx); > if (count) > return count; > @@ -616,7 +611,7 @@ static ssize_t spufs_mbox_read(struct file *file, char __user *buf, > * but still need to return the data we have > * read successfully so far. > */ > - ret = __put_user(mbox_data, udata); > + ret = put_user(mbox_data, udata); > if (ret) { > if (!count) > count = -EFAULT; > @@ -698,17 +693,12 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf, > size_t len, loff_t *pos) > { > struct spu_context *ctx = file->private_data; > - u32 ibox_data, __user *udata; > + u32 ibox_data, __user *udata = (void __user *)buf; > ssize_t count; > > if (len < 4) > return -EINVAL; > > - if (!access_ok(buf, len)) > - return -EFAULT; > - > - udata = (void __user *)buf; > - > count = spu_acquire(ctx); > if (count) > goto out; > @@ -727,7 +717,7 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf, > } > > /* if we can't write at all, return -EFAULT */ > - count = __put_user(ibox_data, udata); > + count = put_user(ibox_data, udata); > if (count) > goto out_unlock; > > @@ -741,7 +731,7 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf, > * but still need to return the data we have > * read successfully so far. > */ > - ret = __put_user(ibox_data, udata); > + ret = put_user(ibox_data, udata); > if (ret) > break; > } > @@ -836,17 +826,13 @@ static ssize_t spufs_wbox_write(struct file *file, const char __user *buf, > size_t len, loff_t *pos) > { > struct spu_context *ctx = file->private_data; > - u32 wbox_data, __user *udata; > + u32 wbox_data, __user *udata = (void __user *)buf; > ssize_t count; > > if (len < 4) > return -EINVAL; > > - udata = (void __user *)buf; > - if (!access_ok(buf, len)) > - return -EFAULT; > - > - if (__get_user(wbox_data, udata)) > + if (get_user(wbox_data, udata)) > return -EFAULT; > > count = spu_acquire(ctx); > @@ -873,7 +859,7 @@ static ssize_t spufs_wbox_write(struct file *file, const char __user *buf, > /* write as much as possible */ > for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) { > int ret; > - ret = __get_user(wbox_data, udata); > + ret = get_user(wbox_data, udata); > if (ret) > break; > > @@ -1982,9 +1968,6 @@ static ssize_t spufs_mbox_info_read(struct file *file, char __user *buf, > u32 stat, data; > int ret; > > - if (!access_ok(buf, len)) > - return -EFAULT; > - > ret = spu_acquire_saved(ctx); > if (ret) > return ret; > @@ -2028,9 +2011,6 @@ static ssize_t spufs_ibox_info_read(struct file *file, char __user *buf, > u32 stat, data; > int ret; > > - if (!access_ok(buf, len)) > - return -EFAULT; > - > ret = spu_acquire_saved(ctx); > if (ret) > return ret; > @@ -2082,9 +2062,6 @@ static ssize_t spufs_wbox_info_read(struct file *file, char __user *buf, > u32 data[ARRAY_SIZE(ctx->csa.spu_mailbox_data)]; > int ret, count; > > - if (!access_ok(buf, len)) > - return -EFAULT; > - > ret = spu_acquire_saved(ctx); > if (ret) > return ret; > @@ -2143,9 +2120,6 @@ static ssize_t spufs_dma_info_read(struct file *file, char __user *buf, > struct spu_dma_info info; > int ret; > > - if (!access_ok(buf, len)) > - return -EFAULT; > - > ret = spu_acquire_saved(ctx); > if (ret) > return ret; >
Hi Christophe, > > Just use the proper non __-prefixed get/put_user variants where > > that is not done yet. > > But it means you are doing the access_ok() check everytime, which is > what is to be avoided by doing the access_ok() once then using the > __-prefixed variant. 5 out of 8 of these are just a access_ok(); simple_read_from_buffer(). For the cases where it's multiple __put/get_user()s, the max will be 5. (for the mbox access). Is that worth optimising the access_ok() checks? Cheers, Jeremy
On Thu, Apr 30, 2020 at 08:39:00AM +0800, Jeremy Kerr wrote: > Hi Christophe, > > > > Just use the proper non __-prefixed get/put_user variants where > > > that is not done yet. > > > > But it means you are doing the access_ok() check everytime, which is > > what is to be avoided by doing the access_ok() once then using the > > __-prefixed variant. > > 5 out of 8 of these are just a access_ok(); simple_read_from_buffer(). > > For the cases where it's multiple __put/get_user()s, the max will be 5. > (for the mbox access). Is that worth optimising the access_ok() checks? access_ok is just trivial comparism to the segment limit, I don't think it has a relavant performance impact.
Le 30/04/2020 à 07:39, Christoph Hellwig a écrit : > On Thu, Apr 30, 2020 at 08:39:00AM +0800, Jeremy Kerr wrote: >> Hi Christophe, >> >>>> Just use the proper non __-prefixed get/put_user variants where >>>> that is not done yet. >>> >>> But it means you are doing the access_ok() check everytime, which is >>> what is to be avoided by doing the access_ok() once then using the >>> __-prefixed variant. >> >> 5 out of 8 of these are just a access_ok(); simple_read_from_buffer(). >> >> For the cases where it's multiple __put/get_user()s, the max will be 5. >> (for the mbox access). Is that worth optimising the access_ok() checks? > > access_ok is just trivial comparism to the segment limit, I don't > think it has a relavant performance impact. > I think it has an impact. See the difference between the two following trivial functions: int test1(unsigned long val, unsigned long *addr) { return __put_user(val, addr); } int test2(unsigned long val, unsigned long *addr) { return put_user(val, addr); } 00000000 <test1>: 0: 39 20 00 00 li r9,0 4: 90 64 00 00 stw r3,0(r4) 8: 7d 23 4b 78 mr r3,r9 c: 4e 80 00 20 blr 00000000 <test2>: 0: 81 22 04 38 lwz r9,1080(r2) 4: 7c 6a 1b 78 mr r10,r3 8: 7f 89 20 40 cmplw cr7,r9,r4 c: 41 9c 00 24 blt cr7,30 <test2+0x30> 10: 7d 24 48 50 subf r9,r4,r9 14: 38 60 ff f2 li r3,-14 18: 2b 89 00 02 cmplwi cr7,r9,2 1c: 4c 9d 00 20 blelr cr7 20: 39 20 00 00 li r9,0 24: 91 44 00 00 stw r10,0(r4) 28: 7d 23 4b 78 mr r3,r9 2c: 4e 80 00 20 blr 30: 38 60 ff f2 li r3,-14 34: 4e 80 00 20 blr It looks like GCC is smart enough to read the limit in task struct only once when we have two consecutive put_user() but there is still some difference: int test3(unsigned long val, unsigned long *addr) { return put_user(val, addr) ? : put_user(val, addr + 1); } int test4(unsigned long val, unsigned long *addr) { if (!access_ok(addr, sizeof(*addr))) return -EFAULT; return __put_user(val, addr) ? : __put_user(val, addr + 1); } 00000000 <test3>: 0: 81 42 04 38 lwz r10,1080(r2) 4: 7f 8a 20 40 cmplw cr7,r10,r4 8: 41 9c 00 48 blt cr7,50 <test3+0x50> c: 7d 04 50 50 subf r8,r4,r10 10: 39 20 ff f2 li r9,-14 14: 2b 88 00 02 cmplwi cr7,r8,2 18: 40 9d 00 30 ble cr7,48 <test3+0x48> 1c: 39 20 00 00 li r9,0 20: 90 64 00 00 stw r3,0(r4) 24: 2f 89 00 00 cmpwi cr7,r9,0 28: 40 9e 00 20 bne cr7,48 <test3+0x48> 2c: 38 84 00 04 addi r4,r4,4 30: 7f 8a 20 40 cmplw cr7,r10,r4 34: 41 9c 00 1c blt cr7,50 <test3+0x50> 38: 7d 44 50 50 subf r10,r4,r10 3c: 2b 8a 00 02 cmplwi cr7,r10,2 40: 40 9d 00 10 ble cr7,50 <test3+0x50> 44: 90 64 00 00 stw r3,0(r4) 48: 7d 23 4b 78 mr r3,r9 4c: 4e 80 00 20 blr 50: 39 20 ff f2 li r9,-14 54: 4b ff ff f4 b 48 <test3+0x48> Disassembly of section .text.test4: 00000000 <test4>: 0: 81 22 04 38 lwz r9,1080(r2) 4: 7f 89 20 40 cmplw cr7,r9,r4 8: 41 9c 00 34 blt cr7,3c <test4+0x3c> c: 7d 44 48 50 subf r10,r4,r9 10: 39 20 ff f2 li r9,-14 14: 2b 8a 00 06 cmplwi cr7,r10,6 18: 40 9d 00 1c ble cr7,34 <test4+0x34> 1c: 39 20 00 00 li r9,0 20: 90 64 00 00 stw r3,0(r4) 24: 2f 89 00 00 cmpwi cr7,r9,0 28: 40 9e 00 0c bne cr7,34 <test4+0x34> 2c: 38 84 00 04 addi r4,r4,4 30: 90 64 00 00 stw r3,0(r4) 34: 7d 23 4b 78 mr r3,r9 38: 4e 80 00 20 blr 3c: 39 20 ff f2 li r9,-14 40: 4b ff ff f4 b 34 <test4+0x34>
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c index b4e1ef650b40..cd7d10f27fad 100644 --- a/arch/powerpc/platforms/cell/spufs/file.c +++ b/arch/powerpc/platforms/cell/spufs/file.c @@ -590,17 +590,12 @@ static ssize_t spufs_mbox_read(struct file *file, char __user *buf, size_t len, loff_t *pos) { struct spu_context *ctx = file->private_data; - u32 mbox_data, __user *udata; + u32 mbox_data, __user *udata = (void __user *)buf; ssize_t count; if (len < 4) return -EINVAL; - if (!access_ok(buf, len)) - return -EFAULT; - - udata = (void __user *)buf; - count = spu_acquire(ctx); if (count) return count; @@ -616,7 +611,7 @@ static ssize_t spufs_mbox_read(struct file *file, char __user *buf, * but still need to return the data we have * read successfully so far. */ - ret = __put_user(mbox_data, udata); + ret = put_user(mbox_data, udata); if (ret) { if (!count) count = -EFAULT; @@ -698,17 +693,12 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf, size_t len, loff_t *pos) { struct spu_context *ctx = file->private_data; - u32 ibox_data, __user *udata; + u32 ibox_data, __user *udata = (void __user *)buf; ssize_t count; if (len < 4) return -EINVAL; - if (!access_ok(buf, len)) - return -EFAULT; - - udata = (void __user *)buf; - count = spu_acquire(ctx); if (count) goto out; @@ -727,7 +717,7 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf, } /* if we can't write at all, return -EFAULT */ - count = __put_user(ibox_data, udata); + count = put_user(ibox_data, udata); if (count) goto out_unlock; @@ -741,7 +731,7 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf, * but still need to return the data we have * read successfully so far. */ - ret = __put_user(ibox_data, udata); + ret = put_user(ibox_data, udata); if (ret) break; } @@ -836,17 +826,13 @@ static ssize_t spufs_wbox_write(struct file *file, const char __user *buf, size_t len, loff_t *pos) { struct spu_context *ctx = file->private_data; - u32 wbox_data, __user *udata; + u32 wbox_data, __user *udata = (void __user *)buf; ssize_t count; if (len < 4) return -EINVAL; - udata = (void __user *)buf; - if (!access_ok(buf, len)) - return -EFAULT; - - if (__get_user(wbox_data, udata)) + if (get_user(wbox_data, udata)) return -EFAULT; count = spu_acquire(ctx); @@ -873,7 +859,7 @@ static ssize_t spufs_wbox_write(struct file *file, const char __user *buf, /* write as much as possible */ for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) { int ret; - ret = __get_user(wbox_data, udata); + ret = get_user(wbox_data, udata); if (ret) break; @@ -1982,9 +1968,6 @@ static ssize_t spufs_mbox_info_read(struct file *file, char __user *buf, u32 stat, data; int ret; - if (!access_ok(buf, len)) - return -EFAULT; - ret = spu_acquire_saved(ctx); if (ret) return ret; @@ -2028,9 +2011,6 @@ static ssize_t spufs_ibox_info_read(struct file *file, char __user *buf, u32 stat, data; int ret; - if (!access_ok(buf, len)) - return -EFAULT; - ret = spu_acquire_saved(ctx); if (ret) return ret; @@ -2082,9 +2062,6 @@ static ssize_t spufs_wbox_info_read(struct file *file, char __user *buf, u32 data[ARRAY_SIZE(ctx->csa.spu_mailbox_data)]; int ret, count; - if (!access_ok(buf, len)) - return -EFAULT; - ret = spu_acquire_saved(ctx); if (ret) return ret; @@ -2143,9 +2120,6 @@ static ssize_t spufs_dma_info_read(struct file *file, char __user *buf, struct spu_dma_info info; int ret; - if (!access_ok(buf, len)) - return -EFAULT; - ret = spu_acquire_saved(ctx); if (ret) return ret;