From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:10:02
Hi all,
this series removes the last set_fs() used to force a kernel address
space for the uaccess code in the kernel read/write/splice code, and then
stops implementing the address space overrides entirely for x86 and
powerpc.
The file system part has been posted a few times, and the read/write side
has been pretty much unchanced. For splice this series drops the
conversion of the seq_file and sysctl code to the iter ops, and thus loses
the splice support for them. The reasons for that is that it caused a lot
of churn for not much use - splice for these small files really isn't much
of a win, even if existing userspace uses it. All callers I found do the
proper fallback, but if this turns out to be an issue the conversion can
be resurrected.
Besides x86 and powerpc I plan to eventually convert all other
architectures, although this will be a slow process, starting with the
easier ones once the infrastructure is merged. The process to convert
architectures is roughtly:
(1) ensure there is no set_fs(KERNEL_DS) left in arch specific code
(2) implement __get_kernel_nofault and __put_kernel_nofault
(3) remove the arch specific address limitation functionality
Changes since v1:
- drop the patch to remove the non-iter ops for /dev/zero and
/dev/null as they caused a performance regression
- don't enable user access in __get_kernel on powerpc
- xfail the set_fs() based lkdtm tests
Diffstat:
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:01:06
Provide __get_kernel_nofault and __put_kernel_nofault routines to
implement the maccess routines without messing with set_fs and without
opening up access to user space.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/include/asm/uaccess.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:01:35
Stop providing the possibility to override the address space using
set_fs() now that there is no need for that any more. To properly
handle the TASK_SIZE_MAX checking for 4 vs 5-level page tables on
x86 a new alternative is introduced, which just like the one in
entry_64.S has to use the hardcoded virtual address bits to escape
the fact that TASK_SIZE_MAX isn't actually a constant when 5-level
page tables are enabled.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/Kconfig | 1 -
arch/x86/ia32/ia32_aout.c | 1 -
arch/x86/include/asm/processor.h | 11 +----------
arch/x86/include/asm/thread_info.h | 2 --
arch/x86/include/asm/uaccess.h | 26 +-------------------------
arch/x86/kernel/asm-offsets.c | 3 ---
arch/x86/lib/getuser.S | 28 ++++++++++++++++++----------
arch/x86/lib/putuser.S | 21 ++++++++++++---------
8 files changed, 32 insertions(+), 61 deletions(-)
@@ -538,8 +534,6 @@ struct thread_struct {*/unsignedlongiopl_emul;-mm_segment_taddr_limit;-unsignedintsig_on_uaccess_err:1;/* Floating point and extended processor state */
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:02:19
For 64-bit the only thing missing was a strategic _AC, and for 32-bit we
need to use __PAGE_OFFSET instead of PAGE_OFFSET in the TASK_SIZE
definition to escape the explicit unsigned long cast. This just works
because __PAGE_OFFSET is defined using _AC itself and thus never needs
the cast anyway.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/include/asm/page_32_types.h | 4 ++--
arch/x86/include/asm/page_64_types.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:02:33
Stop providing the possibility to override the address space using
set_fs() now that there is no need for that any more.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/Kconfig | 1 -
arch/powerpc/include/asm/processor.h | 7 ---
arch/powerpc/include/asm/thread_info.h | 5 +--
arch/powerpc/include/asm/uaccess.h | 62 ++++++++------------------
arch/powerpc/kernel/signal.c | 3 --
arch/powerpc/lib/sstep.c | 6 +--
6 files changed, 22 insertions(+), 62 deletions(-)
@@ -108,11 +108,11 @@ static nokprobe_inline long address_ok(struct pt_regs *regs,{if(!user_mode(regs))return1;-if(__access_ok(ea,nb,USER_DS))+if(__access_ok(ea,nb))return1;-if(__access_ok(ea,1,USER_DS))+if(__access_ok(ea,1))/* Access overlaps the end of the user region */-regs->dar=USER_DS.seg;+regs->dar=TASK_SIZE_MAX-1;elseregs->dar=ea;return0;
@@ -118,6 +118,7 @@ config ARMselectPCI_SYSCALLifPCIselectPERF_USE_VMALLOCselectRTC_LIB+selectSET_FSselectSYS_SUPPORTS_APM_EMULATION# Above selects are sorted alphabetically; please add new ones# according to that. Thanks.
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:09:28
Once we can't manipulate the address limit, we also can't test what
happens when the manipulation is abused.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/misc/lkdtm/bugs.c | 4 ++++
drivers/misc/lkdtm/usercopy.c | 4 ++++
2 files changed, 8 insertions(+)
@@ -315,11 +315,15 @@ void lkdtm_CORRUPT_LIST_DEL(void)/* Test if unbalanced set_fs(KERNEL_DS)/set_fs(USER_DS) check exists. */voidlkdtm_CORRUPT_USER_DS(void){+#ifdef CONFIG_SET_FSpr_info("setting bad task size limit\n");set_fs(KERNEL_DS);/* Make sure we do not keep running with a KERNEL_DS! */force_sig(SIGKILL);+#else+pr_err("XFAIL: this requires set_fs()\n");+#endif}/* Test that VMAP_STACK is actually allocating with a leading guard page */
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:09:34
At least for 64-bit this moves them closer to some of the defines
they are based on, and it prepares for using the TASK_SIZE_MAX
definition from assembly.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <redacted>
---
arch/x86/include/asm/page_32_types.h | 11 +++++++
arch/x86/include/asm/page_64_types.h | 38 +++++++++++++++++++++
arch/x86/include/asm/processor.h | 49 ----------------------------
3 files changed, 49 insertions(+), 49 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:09:37
default_file_splice_write is the last piece of generic code that uses
set_fs to make the uaccess routines operate on kernel pointers. It
implements a "fallback loop" for splicing from files that do not actually
provide a proper splice_read method. The usual file systems and other
high bandwith instances all provide a ->splice_read, so this just removes
support for various device drivers and procfs/debugfs files. If splice
support for any of those turns out to be important it can be added back
by switching them to the iter ops and using generic_file_splice_read.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <redacted>
---
fs/read_write.c | 2 +-
fs/splice.c | 130 +++++----------------------------------------
include/linux/fs.h | 2 -
3 files changed, 15 insertions(+), 119 deletions(-)
@@ -342,89 +342,6 @@ const struct pipe_buf_operations nosteal_pipe_buf_ops = {};EXPORT_SYMBOL(nosteal_pipe_buf_ops);-staticssize_tkernel_readv(structfile*file,conststructkvec*vec,-unsignedlongvlen,loff_toffset)-{-mm_segment_told_fs;-loff_tpos=offset;-ssize_tres;--old_fs=get_fs();-set_fs(KERNEL_DS);-/* The cast to a user pointer is valid due to the set_fs() */-res=vfs_readv(file,(conststructiovec__user*)vec,vlen,&pos,0);-set_fs(old_fs);--returnres;-}--staticssize_tdefault_file_splice_read(structfile*in,loff_t*ppos,-structpipe_inode_info*pipe,size_tlen,-unsignedintflags)-{-structkvec*vec,__vec[PIPE_DEF_BUFFERS];-structiov_iterto;-structpage**pages;-unsignedintnr_pages;-unsignedintmask;-size_toffset,base,copied=0;-ssize_tres;-inti;--if(pipe_full(pipe->head,pipe->tail,pipe->max_usage))-return-EAGAIN;--/*-*Trytokeeppageboundariesmatchingtosourcepagecacheones--*itprobablywon'tbemuchhelp,but...-*/-offset=*ppos&~PAGE_MASK;--iov_iter_pipe(&to,READ,pipe,len+offset);--res=iov_iter_get_pages_alloc(&to,&pages,len+offset,&base);-if(res<=0)-return-ENOMEM;--nr_pages=DIV_ROUND_UP(res+base,PAGE_SIZE);--vec=__vec;-if(nr_pages>PIPE_DEF_BUFFERS){-vec=kmalloc_array(nr_pages,sizeof(structkvec),GFP_KERNEL);-if(unlikely(!vec)){-res=-ENOMEM;-gotoout;-}-}--mask=pipe->ring_size-1;-pipe->bufs[to.head&mask].offset=offset;-pipe->bufs[to.head&mask].len-=offset;--for(i=0;i<nr_pages;i++){-size_tthis_len=min_t(size_t,len,PAGE_SIZE-offset);-vec[i].iov_base=page_address(pages[i])+offset;-vec[i].iov_len=this_len;-len-=this_len;-offset=0;-}--res=kernel_readv(in,vec,nr_pages,*ppos);-if(res>0){-copied=res;-*ppos+=res;-}--if(vec!=__vec)-kfree(vec);-out:-for(i=0;i<nr_pages;i++)-put_page(pages[i]);-kvfree(pages);-iov_iter_advance(&to,copied);/* truncates and discards */-returnres;-}-/**Send'sd->len'bytestosocketfrom'sd->file'atposition'sd->pos'*usingsendpage().Returnthenumberofbytessent.
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-27 15:09:38
Don't allow calling ->read or ->write with set_fs as a preparation for
killing off set_fs. All the instances that we use kernel_read/write on
are using the iter ops already.
If a file has both the regular ->read/->write methods and the iter
variants those could have different semantics for messed up enough
drivers. Also fails the kernel access to them in that case.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <redacted>
---
fs/read_write.c | 67 +++++++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 25 deletions(-)
From: David Laight <hidden> Date: 2020-08-27 15:58:10
From: Christoph Hellwig
Sent: 27 August 2020 16:00
Don't allow calling ->read or ->write with set_fs as a preparation for
killing off set_fs. All the instances that we use kernel_read/write on
are using the iter ops already.
If a file has both the regular ->read/->write methods and the iter
variants those could have different semantics for messed up enough
drivers. Also fails the kernel access to them in that case.
Is there a real justification for that?
For system calls supplying both methods makes sense to avoid
the extra code paths for a simple read/write.
Any one stupid enough to make them behave differently gets
what they deserve.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig [off-list ref] wrote:
Once we can't manipulate the address limit, we also can't test what
happens when the manipulation is abused.
Just remove these tests entirely.
Once set_fs() doesn't exist on x86, the tests no longer make any sense
what-so-ever, because test coverage will be basically zero.
So don't make the code uglier just to maintain a fiction that
something is tested when it isn't really.
Linus
This should be
LOAD_TASK_SIZE_MAX_MINUS_N(1)
cmp %_ASM_DX,%_ASM_AX
instead (and then because we no longer modify _ASM_AX, we'd also
remove the offset on the access).
SYM_FUNC_START(__put_user_2)
- ENTER
- mov TASK_addr_limit(%_ASM_BX),%_ASM_BX
+ LOAD_TASK_SIZE_MAX
sub $1,%_ASM_BX
It's even more obvious here. We load a constant and then immediately
do a "sub $1" on that value.
It's not a huge deal, you don't have to respin the series for this, I
just wanted to point it out so that people are aware of it and if I
forget somebody else will hopefully remember that "we should fix that
too".
Linus
On Thu, Aug 27, 2020 at 03:58:02PM +0000, David Laight wrote:
Is there a real justification for that?
For system calls supplying both methods makes sense to avoid
the extra code paths for a simple read/write.
Al asked for it as two of our four in-tree instances do have weird
semantics, and we can't change that any more. And the other two
don't make sense to be used with kernel_read and kernel_write (
(/dev/null and /dev/zero).
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-29 09:24:13
On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig [off-list ref] wrote:
quoted
Once we can't manipulate the address limit, we also can't test what
happens when the manipulation is abused.
Just remove these tests entirely.
Once set_fs() doesn't exist on x86, the tests no longer make any sense
what-so-ever, because test coverage will be basically zero.
So don't make the code uglier just to maintain a fiction that
something is tested when it isn't really.
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-29 09:25:38
On Thu, Aug 27, 2020 at 11:15:12AM -0700, Linus Torvalds wrote:
quoted
SYM_FUNC_START(__put_user_2)
- ENTER
- mov TASK_addr_limit(%_ASM_BX),%_ASM_BX
+ LOAD_TASK_SIZE_MAX
sub $1,%_ASM_BX
It's even more obvious here. We load a constant and then immediately
do a "sub $1" on that value.
It's not a huge deal, you don't have to respin the series for this, I
just wanted to point it out so that people are aware of it and if I
forget somebody else will hopefully remember that "we should fix that
too".
The changes seem easy enough and I need to respin at least for the
lkdtm changes, and probaby also for a pending fix in the low-level
x86 code that will hopefully be picked up for 5.9.
But the more important questions is: how do we want to pick the series
up? Especially due to the splice changes I really want it to be in
linux-next as long as possible.
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-01 07:08:51
Looks like since the start of this series we've grown new code to
use kernel_write on sysctl files based on boot parameters. The good
news is that this just means I need to resurrect the sysctl series
as all that work was done already.
Hi Christoph,
Le 27/08/2020 à 17:00, Christoph Hellwig a écrit :
Hi all,
this series removes the last set_fs() used to force a kernel address
space for the uaccess code in the kernel read/write/splice code, and then
stops implementing the address space overrides entirely for x86 and
powerpc.
The file system part has been posted a few times, and the read/write side
has been pretty much unchanced. For splice this series drops the
conversion of the seq_file and sysctl code to the iter ops, and thus loses
the splice support for them. The reasons for that is that it caused a lot
of churn for not much use - splice for these small files really isn't much
of a win, even if existing userspace uses it. All callers I found do the
proper fallback, but if this turns out to be an issue the conversion can
be resurrected.
Besides x86 and powerpc I plan to eventually convert all other
architectures, although this will be a slow process, starting with the
easier ones once the infrastructure is merged. The process to convert
architectures is roughtly:
(1) ensure there is no set_fs(KERNEL_DS) left in arch specific code
(2) implement __get_kernel_nofault and __put_kernel_nofault
(3) remove the arch specific address limitation functionality
Changes since v1:
- drop the patch to remove the non-iter ops for /dev/zero and
/dev/null as they caused a performance regression
- don't enable user access in __get_kernel on powerpc
- xfail the set_fs() based lkdtm tests
Diffstat:
I'm still sceptic with the results I get.
With 5.9-rc2:
root@vgoippro:~# time dd if=/dev/zero of=/dev/null count=1M
1048576+0 records in
1048576+0 records out
536870912 bytes (512.0MB) copied, 5.585880 seconds, 91.7MB/s
real 0m 5.59s
user 0m 1.40s
sys 0m 4.19s
With your series:
root@vgoippro:/tmp# time dd if=/dev/zero of=/dev/null count=1M
1048576+0 records in
1048576+0 records out
536870912 bytes (512.0MB) copied, 7.780540 seconds, 65.8MB/s
real 0m 7.79s
user 0m 2.12s
sys 0m 5.66s
Top of perf report of a standard perf record:
With 5.9-rc2:
20.31% dd [kernel.kallsyms] [k] __arch_clear_user
8.37% dd [kernel.kallsyms] [k] transfer_to_syscall
7.37% dd [kernel.kallsyms] [k] __fsnotify_parent
6.95% dd [kernel.kallsyms] [k] iov_iter_zero
5.72% dd [kernel.kallsyms] [k] new_sync_read
4.87% dd [kernel.kallsyms] [k] vfs_write
4.47% dd [kernel.kallsyms] [k] vfs_read
3.07% dd [kernel.kallsyms] [k] ksys_write
2.77% dd [kernel.kallsyms] [k] ksys_read
2.65% dd [kernel.kallsyms] [k] __fget_light
2.37% dd [kernel.kallsyms] [k] __fdget_pos
2.35% dd [kernel.kallsyms] [k] memset
1.53% dd [kernel.kallsyms] [k] rw_verify_area
1.52% dd [kernel.kallsyms] [k] read_iter_zero
With your series:
19.60% dd [kernel.kallsyms] [k] __arch_clear_user
10.92% dd [kernel.kallsyms] [k] iov_iter_zero
9.50% dd [kernel.kallsyms] [k] vfs_write
8.97% dd [kernel.kallsyms] [k] __fsnotify_parent
5.46% dd [kernel.kallsyms] [k] transfer_to_syscall
5.42% dd [kernel.kallsyms] [k] vfs_read
3.58% dd [kernel.kallsyms] [k] ksys_read
2.84% dd [kernel.kallsyms] [k] read_iter_zero
2.24% dd [kernel.kallsyms] [k] ksys_write
1.80% dd [kernel.kallsyms] [k] __fget_light
1.34% dd [kernel.kallsyms] [k] __fdget_pos
0.91% dd [kernel.kallsyms] [k] memset
0.91% dd [kernel.kallsyms] [k] rw_verify_area
Christophe
From: Matthew Wilcox <willy@infradead.org> Date: 2020-09-01 17:42:36
On Tue, Sep 01, 2020 at 06:25:12PM +0100, Al Viro wrote:
On Tue, Sep 01, 2020 at 07:13:00PM +0200, Christophe Leroy wrote:
quoted
10.92% dd [kernel.kallsyms] [k] iov_iter_zero
Interesting... Could you get an instruction-level profile inside iov_iter_zero(),
along with the disassembly of that sucker?
Also, does [1] make any difference? Probably not since it's translating
O flags into IOCB flags instead of RWF flags into IOCB flags. I wonder
if there's a useful trick we can play here ... something like:
static inline int iocb_flags(struct file *file)
{
int res = 0;
if (likely(!file->f_flags & O_APPEND | O_DIRECT | O_DSYNC | __O_SYNC)) && !IS_SYNC(file->f_mapping->host))
return res;
if (file->f_flags & O_APPEND)
res |= IOCB_APPEND;
if (file->f_flags & O_DIRECT)
res |= IOCB_DIRECT;
if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))
res |= IOCB_DSYNC;
if (file->f_flags & __O_SYNC)
res |= IOCB_SYNC;
return res;
}
Can we do something like force O_DSYNC to be set if the inode IS_SYNC()
at the time of open? Or is setting the sync bit on the inode required
to affect currently-open files?
[1] https://lore.kernel.org/linux-fsdevel/95de7ce4-9254-39f1-304f-4455f66bf0f4@kernel.dk/
On Sat, Aug 29, 2020 at 11:24:06AM +0200, Christoph Hellwig wrote:
On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
quoted
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig [off-list ref] wrote:
quoted
Once we can't manipulate the address limit, we also can't test what
happens when the manipulation is abused.
Just remove these tests entirely.
Once set_fs() doesn't exist on x86, the tests no longer make any sense
what-so-ever, because test coverage will be basically zero.
So don't make the code uglier just to maintain a fiction that
something is tested when it isn't really.
Sure fine with me unless Kees screams.
If we don't have set_fs, we don't need the tests. :)
--
Kees Cook
On Sat, Aug 29, 2020 at 11:24:06AM +0200, Christoph Hellwig wrote:
On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
quoted
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig [off-list ref] wrote:
quoted
Once we can't manipulate the address limit, we also can't test what
happens when the manipulation is abused.
Just remove these tests entirely.
Once set_fs() doesn't exist on x86, the tests no longer make any sense
what-so-ever, because test coverage will be basically zero.
So don't make the code uglier just to maintain a fiction that
something is tested when it isn't really.
Sure fine with me unless Kees screams.
To clarify: if any of x86, arm64, arm, powerpc, riscv, and s390 are
using set_fs(), I want to keep this test. "ugly" is fine in lkdtm. :)
--
Kees Cook
Le 27/08/2020 à 17:00, Christoph Hellwig a écrit :
quoted hunk
Stop providing the possibility to override the address space using
set_fs() now that there is no need for that any more.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/Kconfig | 1 -
arch/powerpc/include/asm/processor.h | 7 ---
arch/powerpc/include/asm/thread_info.h | 5 +--
arch/powerpc/include/asm/uaccess.h | 62 ++++++++------------------
arch/powerpc/kernel/signal.c | 3 --
arch/powerpc/lib/sstep.c | 6 +--
6 files changed, 22 insertions(+), 62 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-02 08:09:19
On Tue, Sep 01, 2020 at 11:57:37AM -0700, Kees Cook wrote:
On Sat, Aug 29, 2020 at 11:24:06AM +0200, Christoph Hellwig wrote:
quoted
On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
quoted
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig [off-list ref] wrote:
quoted
Once we can't manipulate the address limit, we also can't test what
happens when the manipulation is abused.
Just remove these tests entirely.
Once set_fs() doesn't exist on x86, the tests no longer make any sense
what-so-ever, because test coverage will be basically zero.
So don't make the code uglier just to maintain a fiction that
something is tested when it isn't really.
Sure fine with me unless Kees screams.
To clarify: if any of x86, arm64, arm, powerpc, riscv, and s390 are
using set_fs(), I want to keep this test. "ugly" is fine in lkdtm. :)
And Linus wants them gone entirely, so I'll need a stage fight between
the two of you. At least for this merge window I'm only planning on
x86 and power, plus maybe riscv if I get the work done in time. Although
helper from the maintainers would be welcome. s390 has a driver that
still uses set_fs that will need some surgery, although it shouldn't
be too bad, but arm will be a piece of work. Unless I get help it will
take a while.
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-02 08:10:23
On Tue, Sep 01, 2020 at 06:25:12PM +0100, Al Viro wrote:
On Tue, Sep 01, 2020 at 07:13:00PM +0200, Christophe Leroy wrote:
quoted
10.92% dd [kernel.kallsyms] [k] iov_iter_zero
Interesting... Could you get an instruction-level profile inside iov_iter_zero(),
along with the disassembly of that sucker?
So the interesting thing here is with that none of these code paths
should have changed at all, and the biggest items on the profile look
the same modulo some minor reordering.
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
No, this is accidental and broken. Can you re-run your benchmark with
this fixed?
Is TASK_SIZE_MASK defined such that you can do:
return (addr | size) < TASK_SIZE_MAX) || !size;
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
No, this is accidental and broken. Can you re-run your benchmark with
this fixed?
Is TASK_SIZE_MASK defined such that you can do:
return (addr | size) < TASK_SIZE_MAX) || !size;
TASK_SIZE_MAX will usually be 0xc0000000
With:
addr = 0x80000000;
size = 0x80000000;
I expect it to fail ....
With the formula you propose it will succeed, won't it ?
Hmmm... Was i getting confused about some comments for 64bit
about there being such a big hole between valid user and kernel
addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
That would be true for 64bit x86 (and probably ppc (& arm??))
if TASK_SIZE_MAX were 0x4 << 60.
IIUC the highest user address is (much) less than 0x0 << 60
and the lowest kernel address (much) greater than 0xf << 60
on all these 64bit platforms.
Actually if doing access_ok() inside get_user() you don't
need to check the size at all.
You don't even need to in copy_to/from_user() provided
it always does a forwards copy.
(Rather that copying the last word first for misaligned lengths.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
No, this is accidental and broken. Can you re-run your benchmark with
this fixed?
Is TASK_SIZE_MASK defined such that you can do:
return (addr | size) < TASK_SIZE_MAX) || !size;
TASK_SIZE_MAX will usually be 0xc0000000
With:
addr = 0x80000000;
size = 0x80000000;
I expect it to fail ....
With the formula you propose it will succeed, won't it ?
Christophe
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
No, this is accidental and broken. Can you re-run your benchmark with
this fixed?
Is TASK_SIZE_MASK defined such that you can do:
return (addr | size) < TASK_SIZE_MAX) || !size;
TASK_SIZE_MAX will usually be 0xc0000000
With:
addr = 0x80000000;
size = 0x80000000;
I expect it to fail ....
With the formula you propose it will succeed, won't it ?
Hmmm... Was i getting confused about some comments for 64bit
about there being such a big hole between valid user and kernel
addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
That would be true for 64bit x86 (and probably ppc (& arm??))
if TASK_SIZE_MAX were 0x4 << 60.
IIUC the highest user address is (much) less than 0x0 << 60
and the lowest kernel address (much) greater than 0xf << 60
on all these 64bit platforms.
Actually if doing access_ok() inside get_user() you don't
need to check the size at all.
You mean on 64 bit or on any platform ?
What about a word write to 0xbffffffe, won't it overwrite 0xc0000000 ?
You don't even need to in copy_to/from_user() provided
it always does a forwards copy.
Do you mean due to the gap ?
Is it garantied to be a gap ? Even on a 32 bits having TASK_SIZE set to
0xc0000000 and PAGE_OFFSET set to the same ?
Christophe
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
No, this is accidental and broken. Can you re-run your benchmark with
this fixed?
With this fix, I get
root@vgoippro:~# time dd if=/dev/zero of=/dev/null count=1M
1048576+0 records in
1048576+0 records out
536870912 bytes (512.0MB) copied, 6.776327 seconds, 75.6MB/s
real 0m 6.78s
user 0m 1.64s
sys 0m 5.13s
That's still far from the 91.7MB/s I get with 5.9-rc2, but better than
the 65.8MB/s I got yesterday with your series. Still some way to go thought.
Christophe
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
No, this is accidental and broken. Can you re-run your benchmark with
this fixed?
Is TASK_SIZE_MASK defined such that you can do:
return (addr | size) < TASK_SIZE_MAX) || !size;
TASK_SIZE_MAX will usually be 0xc0000000
With:
addr = 0x80000000;
size = 0x80000000;
I expect it to fail ....
With the formula you propose it will succeed, won't it ?
Hmmm... Was i getting confused about some comments for 64bit
about there being such a big hole between valid user and kernel
addresses that it was enough to check that 'size < TASK_SIZE_MAX'.
That would be true for 64bit x86 (and probably ppc (& arm??))
if TASK_SIZE_MAX were 0x4 << 60.
IIUC the highest user address is (much) less than 0x0 << 60
and the lowest kernel address (much) greater than 0xf << 60
on all these 64bit platforms.
Actually if doing access_ok() inside get_user() you don't
need to check the size at all.
You mean on 64 bit or on any platform ?
64bit and 32bit
What about a word write to 0xbffffffe, won't it overwrite 0xc0000000 ?
quoted
You don't even need to in copy_to/from_user() provided
it always does a forwards copy.
Do you mean due to the gap ?
Is it garantied to be a gap ? Even on a 32 bits having TASK_SIZE set to
0xc0000000 and PAGE_OFFSET set to the same ?
I read somewhere (I won't find it again) that the last 4k page
(below 0xc0000000) must not be allocated on i386 because some
cpu (both intel and amd) do 'horrid things' if they try to
(IIRC) do instruction prefetches across the boundary.
So the accesses to 0xbffffffe will fault and the one to 0xc0000000
won't happen (in any useful way at least).
I'd suspect that not allocating the 3G-4k page would be a safe
bet on all architectures - even 68k.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Wed, Sep 2, 2020 at 8:17 AM Christophe Leroy
[off-list ref] wrote:
With this fix, I get
root@vgoippro:~# time dd if=/dev/zero of=/dev/null count=1M
536870912 bytes (512.0MB) copied, 6.776327 seconds, 75.6MB/s
That's still far from the 91.7MB/s I get with 5.9-rc2, but better than
the 65.8MB/s I got yesterday with your series. Still some way to go thought.
I don't see why this change would make any difference.
And btw, why do the 32-bit and 64-bit checks even differ? It's not
like the extra (single) instruction should even matter. I think the
main reason is that the simpler 64-bit case could stay as a macro
(because it only uses "addr" and "size" once), but honestly, that
"simplification" doesn't help when you then need to have that #ifdef
for the 32-bit case and an inline function anyway.
So why isn't it just
static inline int __access_ok(unsigned long addr, unsigned long size)
{ return addr <= TASK_SIZE_MAX && size <= TASK_SIZE_MAX-addr; }
for both and be done with it?
The "size=0" check is only relevant for the "addr == TASK_SIZE_MAX"
case, and existed in the old code because it had that "-1" thing
becasue "seg.seg" was actually TASK_SIZE-1.
Now that we don't have any TASK_SIZE-1, zero isn't special any more.
However, I suspect a bigger reason for the actual performance
degradation would be the patch that makes things use "write_iter()"
for writing, even when a simpler "write()" exists.
For writing to /dev/null, the cost of setting up iterators and all the
pointless indirection is all kinds of stupid.
So I think "write()" should just go back to default to using
"->write()" rather than "->write_iter()" if the simpler case exists.
Linus
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-03 07:11:57
On Wed, Sep 02, 2020 at 11:02:22AM -0700, Linus Torvalds wrote:
I don't see why this change would make any difference.
Me neither, but while looking at a different project I did spot places
that actually do an access_ok with len 0, that's why I wanted him to
try.
That being said: Christophe are these number stables? Do you get
similar numbers with multiple runs?
And btw, why do the 32-bit and 64-bit checks even differ? It's not
like the extra (single) instruction should even matter. I think the
main reason is that the simpler 64-bit case could stay as a macro
(because it only uses "addr" and "size" once), but honestly, that
"simplification" doesn't help when you then need to have that #ifdef
for the 32-bit case and an inline function anyway.
I'll have to leave that to the powerpc folks. The intent was to not
change the behavior (and I even fucked that up for the the size == 0
case).
However, I suspect a bigger reason for the actual performance
degradation would be the patch that makes things use "write_iter()"
for writing, even when a simpler "write()" exists.
Except that we do not actually have such a patch. For normal user
writes we only use ->write_iter if ->write is not present. But what
shows up in the profile is that /dev/zero only has a read_iter op and
not a normal read. I've added a patch below that implements a normal
read which might help a tad with this workload, but should not be part
of a regression.
Also Christophe: can you bisect which patch starts this? Is it really
this last patch in the series?
---
On Wed, Sep 2, 2020 at 8:17 AM Christophe Leroy
[off-list ref] wrote:
quoted
With this fix, I get
root@vgoippro:~# time dd if=/dev/zero of=/dev/null count=1M
536870912 bytes (512.0MB) copied, 6.776327 seconds, 75.6MB/s
That's still far from the 91.7MB/s I get with 5.9-rc2, but better than
the 65.8MB/s I got yesterday with your series. Still some way to go thought.
I don't see why this change would make any difference.
Neither do I.
Looks like nowadays, CONFIG_STACKPROTECTOR has become a default.
I rebuilt the kernel without it, I now get a throughput of 99.8MB/s both
without and with this series.
Looking at the generated code (GCC 10.1), a small change in a function
seems to make large changes in the generated code when
CONFIG_STACKPROTECTOR is set.
In addition to that, trivial functions which don't use the stack at all
get a stack frame anyway when CONFIG_STACKPROTECTOR is set, allthough
that's only -fstack-protector-strong. And there is no canary check.
Without CONFIG_STACKPROTECTOR:
c01572a0 <no_llseek>:
c01572a0: 38 60 ff ff li r3,-1
c01572a4: 38 80 ff e3 li r4,-29
c01572a8: 4e 80 00 20 blr
With CONFIG_STACKPROTECTOR (regardless of CONFIG_STACKPROTECTOR_STRONG
or not):
c0164e08 <no_llseek>:
c0164e08: 94 21 ff f0 stwu r1,-16(r1)
c0164e0c: 38 60 ff ff li r3,-1
c0164e10: 38 80 ff e3 li r4,-29
c0164e14: 38 21 00 10 addi r1,r1,16
c0164e18: 4e 80 00 20 blr
Wondering why CONFIG_STACKPROTECTOR has become the default. It seems to
imply a 10% performance loss even in the best case (91.7MB/s versus
99.8MB/s)
Note that without CONFIG_STACKPROTECTOR_STRONG, I'm at 99.3MB/s, so
that's really the _STRONG alternative that hurts.
Christophe
Le 03/09/2020 à 09:11, Christoph Hellwig a écrit :
On Wed, Sep 02, 2020 at 11:02:22AM -0700, Linus Torvalds wrote:
quoted
I don't see why this change would make any difference.
Me neither, but while looking at a different project I did spot places
that actually do an access_ok with len 0, that's why I wanted him to
try.
That being said: Christophe are these number stables? Do you get
similar numbers with multiple runs?
Yes the numbers are similar with multiple runs and multiple reboots.
quoted
And btw, why do the 32-bit and 64-bit checks even differ? It's not
like the extra (single) instruction should even matter. I think the
main reason is that the simpler 64-bit case could stay as a macro
(because it only uses "addr" and "size" once), but honestly, that
"simplification" doesn't help when you then need to have that #ifdef
for the 32-bit case and an inline function anyway.
I'll have to leave that to the powerpc folks. The intent was to not
change the behavior (and I even fucked that up for the the size == 0
case).
quoted
However, I suspect a bigger reason for the actual performance
degradation would be the patch that makes things use "write_iter()"
for writing, even when a simpler "write()" exists.
Except that we do not actually have such a patch. For normal user
writes we only use ->write_iter if ->write is not present. But what
shows up in the profile is that /dev/zero only has a read_iter op and
not a normal read. I've added a patch below that implements a normal
read which might help a tad with this workload, but should not be part
of a regression.
Also Christophe: can you bisect which patch starts this? Is it really
this last patch in the series?
5.9-rc2: 91.5MB/s
Patch 1: 74.9MB/s
Patch 2: 97.9MB/s
Patch 3: 97.7MB/s
Patch 4 to 9: 97.9MB/s
Patch 10: 85.3MB/s
Patch 11: 75.4MB/s
See my other mail, when removing CONFIG_STACKPROTECTOR, I get a stable
99.8MB/s throughput.
Christophe
Le 03/09/2020 à 09:11, Christoph Hellwig a écrit :
Except that we do not actually have such a patch. For normal user
writes we only use ->write_iter if ->write is not present. But what
shows up in the profile is that /dev/zero only has a read_iter op and
not a normal read. I've added a patch below that implements a normal
read which might help a tad with this workload, but should not be part
of a regression.
With that patch below, throughput is 113.5MB/s (instead of 99.9MB/s).
So a 14% improvement. That's not bad.
Christophe
From: David Howells <dhowells@redhat.com> Date: 2020-10-27 09:29:57
Christoph Hellwig [off-list ref] wrote:
default_file_splice_write is the last piece of generic code that uses
set_fs to make the uaccess routines operate on kernel pointers. It
implements a "fallback loop" for splicing from files that do not actually
provide a proper splice_read method. The usual file systems and other
high bandwith instances all provide a ->splice_read, so this just removes
support for various device drivers and procfs/debugfs files. If splice
support for any of those turns out to be important it can be added back
by switching them to the iter ops and using generic_file_splice_read.
Hmmm... this causes the copy_file_range() syscall to fail with EINVAL in some
places where before it used to work.
For my part, it causes the generic/112 xfstest to fail with afs, but there may
be other places.
Is this a regression we need to fix in the VFS core? Or is it something we
need to fix in xfstests and assume userspace will fallback to doing it itself?
David
From: David Howells <dhowells@redhat.com> Date: 2020-10-27 09:52:46
David Howells [off-list ref] wrote:
quoted
default_file_splice_write is the last piece of generic code that uses
set_fs to make the uaccess routines operate on kernel pointers. It
implements a "fallback loop" for splicing from files that do not actually
provide a proper splice_read method. The usual file systems and other
high bandwith instances all provide a ->splice_read, so this just removes
support for various device drivers and procfs/debugfs files. If splice
support for any of those turns out to be important it can be added back
by switching them to the iter ops and using generic_file_splice_read.
Hmmm... this causes the copy_file_range() syscall to fail with EINVAL in some
places where before it used to work.
For my part, it causes the generic/112 xfstest to fail with afs, but there may
be other places.
Is this a regression we need to fix in the VFS core? Or is it something we
need to fix in xfstests and assume userspace will fallback to doing it itself?
That said, for afs at least, the fix seems to be just this:
From: Christoph Hellwig <hch@lst.de> Date: 2020-10-27 09:56:24
On Tue, Oct 27, 2020 at 09:51:34AM +0000, David Howells wrote:
David Howells [off-list ref] wrote:
quoted
quoted
default_file_splice_write is the last piece of generic code that uses
set_fs to make the uaccess routines operate on kernel pointers. It
implements a "fallback loop" for splicing from files that do not actually
provide a proper splice_read method. The usual file systems and other
high bandwith instances all provide a ->splice_read, so this just removes
support for various device drivers and procfs/debugfs files. If splice
support for any of those turns out to be important it can be added back
by switching them to the iter ops and using generic_file_splice_read.
Hmmm... this causes the copy_file_range() syscall to fail with EINVAL in some
places where before it used to work.
For my part, it causes the generic/112 xfstest to fail with afs, but there may
be other places.
Is this a regression we need to fix in the VFS core? Or is it something we
need to fix in xfstests and assume userspace will fallback to doing it itself?
That said, for afs at least, the fix seems to be just this:
And that is the correct fix, I was about to send it to you.
We can't have a "generic" splice using ->read/->write without set_fs,
in addition to the iter_file_splice_write based version being a lot
more efficient than what you had before.