From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:32:32
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:
- ensure there is no set_fs(KERNEL_DS) left in arch specific code
- implement __get_kernel_nofault and __put_kernel_nofault
- remove the arch specific address limitation functionality
Diffstat:
arch/Kconfig | 3
arch/alpha/Kconfig | 1
arch/arc/Kconfig | 1
arch/arm/Kconfig | 1
arch/arm64/Kconfig | 1
arch/c6x/Kconfig | 1
arch/csky/Kconfig | 1
arch/h8300/Kconfig | 1
arch/hexagon/Kconfig | 1
arch/ia64/Kconfig | 1
arch/m68k/Kconfig | 1
arch/microblaze/Kconfig | 1
arch/mips/Kconfig | 1
arch/nds32/Kconfig | 1
arch/nios2/Kconfig | 1
arch/openrisc/Kconfig | 1
arch/parisc/Kconfig | 1
arch/powerpc/include/asm/processor.h | 7 -
arch/powerpc/include/asm/thread_info.h | 5 -
arch/powerpc/include/asm/uaccess.h | 78 ++++++++-----------
arch/powerpc/kernel/signal.c | 3
arch/powerpc/lib/sstep.c | 6 -
arch/riscv/Kconfig | 1
arch/s390/Kconfig | 1
arch/sh/Kconfig | 1
arch/sparc/Kconfig | 1
arch/um/Kconfig | 1
arch/x86/ia32/ia32_aout.c | 1
arch/x86/include/asm/page_32_types.h | 11 ++
arch/x86/include/asm/page_64_types.h | 38 +++++++++
arch/x86/include/asm/processor.h | 60 ---------------
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 +++--
arch/xtensa/Kconfig | 1
drivers/char/mem.c | 16 ----
drivers/misc/lkdtm/bugs.c | 2
drivers/misc/lkdtm/core.c | 4 +
drivers/misc/lkdtm/usercopy.c | 2
fs/read_write.c | 69 ++++++++++-------
fs/splice.c | 130 +++------------------------------
include/linux/fs.h | 2
include/linux/uaccess.h | 18 ++++
lib/test_bitmap.c | 10 ++
46 files changed, 235 insertions(+), 332 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:32:43
For 64-bit the only hing 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>
---
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(-)
On Mon, Aug 17, 2020 at 09:32:09AM +0200, Christoph Hellwig wrote:
For 64-bit the only hing missing was a strategic _AC, and for 32-bit we
typo: thing
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.
Shouldn't this be folded into the prior patch so there's no bisection
problem?
-Kees
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-18 19:55:45
On Tue, Aug 18, 2020 at 12:44:49PM -0700, Kees Cook wrote:
On Mon, Aug 17, 2020 at 09:32:09AM +0200, Christoph Hellwig wrote:
quoted
For 64-bit the only hing missing was a strategic _AC, and for 32-bit we
typo: thing
quoted
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.
Shouldn't this be folded into the prior patch so there's no bisection
problem?
I didn't see a problem bisecting, do you have something particular in
mind?
On Tue, Aug 18, 2020 at 09:55:39PM +0200, Christoph Hellwig wrote:
On Tue, Aug 18, 2020 at 12:44:49PM -0700, Kees Cook wrote:
quoted
On Mon, Aug 17, 2020 at 09:32:09AM +0200, Christoph Hellwig wrote:
quoted
For 64-bit the only hing missing was a strategic _AC, and for 32-bit we
typo: thing
quoted
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.
Shouldn't this be folded into the prior patch so there's no bisection
problem?
I didn't see a problem bisecting, do you have something particular in
mind?
Oh, I misunderstood this patch to be a fix for compilation. Is this just
a correctness fix?
--
Kees Cook
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:32:44
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>
---
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: David Laight <hidden> Date: 2020-08-17 08:23:47
From: Christoph Hellwig
Sent: 17 August 2020 08:32
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.
Can't that always compare against a constant even when 5-levl
page tables are enabled on x86-64?
On x86-64 it can (probably) reduce to (addr | (addr + size)) < 0.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Aug 17, 2020 at 08:23:11AM +0000, David Laight wrote:
From: Christoph Hellwig
quoted
Sent: 17 August 2020 08:32
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.
Can't that always compare against a constant even when 5-levl
page tables are enabled on x86-64?
On x86-64 it can (probably) reduce to (addr | (addr + size)) < 0.
I'll leave that to the x86 maintainers as a future cleanup if wanted.
On Mon, Aug 17, 2020 at 09:32:10AM +0200, Christoph Hellwig wrote:
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>
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:32:54
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(+)
Le 17/08/2020 à 09:32, Christoph Hellwig a écrit :
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.
__get_user_size() opens access to user space. You have to use
__get_user_size_allowed() when user access is already allowed (or when
not needed to allow it).
Christophe
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33:07
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;
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33:15
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 | 2 ++
drivers/misc/lkdtm/core.c | 4 ++++
drivers/misc/lkdtm/usercopy.c | 2 ++
3 files changed, 8 insertions(+)
@@ -312,6 +312,7 @@ void lkdtm_CORRUPT_LIST_DEL(void)pr_err("list_del() corruption not detected!\n");}+#ifdef CONFIG_SET_FS/* Test if unbalanced set_fs(KERNEL_DS)/set_fs(USER_DS) check exists. */voidlkdtm_CORRUPT_USER_DS(void){
@@ -321,6 +322,7 @@ void lkdtm_CORRUPT_USER_DS(void)/* Make sure we do not keep running with a KERNEL_DS! */force_sig(SIGKILL);}+#endif/* Test that VMAP_STACK is actually allocating with a leading guard page */voidlkdtm_STACK_GUARD_PAGE_LEADING(void)
On Mon, Aug 17, 2020 at 09:32:07AM +0200, Christoph Hellwig wrote:
quoted hunk
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 | 2 ++
drivers/misc/lkdtm/core.c | 4 ++++
drivers/misc/lkdtm/usercopy.c | 2 ++
3 files changed, 8 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33:17
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>
---
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(-)
On Mon, Aug 17, 2020 at 09:32:08AM +0200, Christoph Hellwig wrote:
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>
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33:18
There is no good reason to implement both the traditional ->read and
->write as well as the iter based ops. So implement just the iter
based ones.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/char/mem.c | 16 ----------------
1 file changed, 16 deletions(-)
On Mon, Aug 17, 2020 at 09:32:02AM +0200, Christoph Hellwig wrote:
There is no good reason to implement both the traditional ->read and
->write as well as the iter based ops. So implement just the iter
based ones.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christoph Hellwig <hch@lst.de>
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33:25
We can't run the tests for userspace bitmap parsing if set_fs() doesn't
exist.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/test_bitmap.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Le 17/08/2020 à 09:32, Christoph Hellwig a écrit :
quoted hunk
We can't run the tests for userspace bitmap parsing if set_fs() doesn't
exist.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/test_bitmap.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
get_fs() and set_fs() have stubs for when an arch doesn't define them,
so I this it would be cleaner if you were using 'if
(IS_ENABLED(CONFIG_SET_FS) && is_user)`instead of an ifdefery in the
middle of the if/else.
Christophe
quoted hunk
if (is_user) {
mm_segment_t orig_fs = get_fs();
size_t len = strlen(ptest.in);
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:52:57
On Mon, Aug 17, 2020 at 09:50:05AM +0200, Christophe Leroy wrote:
Le 17/08/2020 à 09:32, Christoph Hellwig a écrit :
quoted
We can't run the tests for userspace bitmap parsing if set_fs() doesn't
exist.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/test_bitmap.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
get_fs() and set_fs() have stubs for when an arch doesn't define them, so I
this it would be cleaner if you were using 'if (IS_ENABLED(CONFIG_SET_FS)
&& is_user)`instead of an ifdefery in the middle of the if/else.
No, I don't provide stubs in the prep patch, and that has been intentional
as I don't want this to spread much. test_bitmap would be the only place
where they are somewht useful, and I just hope this test is eventually
getting rewritten to run in a normal user space context where the
uaccess tests can be resurrected.
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33:30
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>
---
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.
On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig 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.
Signed-off-by: Christoph Hellwig <hch@lst.de>
This seems a bit disruptive? I feel like this is going to make fuzzers
really noisy (e.g. trinity likes to splice random stuff out of /sys and
/proc).
Conceptually, though:
Reviewed-by: Kees Cook <redacted>
--
Kees Cook
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-18 19:54:53
On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig 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.
Signed-off-by: Christoph Hellwig <hch@lst.de>
This seems a bit disruptive? I feel like this is going to make fuzzers
really noisy (e.g. trinity likes to splice random stuff out of /sys and
/proc).
Noisy in the sence of triggering the pr_debug or because they can't
handle -EINVAL?
On Tue, Aug 18, 2020 at 09:54:46PM +0200, Christoph Hellwig wrote:
On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
quoted
On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig 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.
Signed-off-by: Christoph Hellwig <hch@lst.de>
This seems a bit disruptive? I feel like this is going to make fuzzers
really noisy (e.g. trinity likes to splice random stuff out of /sys and
/proc).
Noisy in the sence of triggering the pr_debug or because they can't
handle -EINVAL?
Well, maybe both? I doubt much _expects_ to be using splice, so I'm fine
with that, but it seems weird not to have a fall-back, especially if
something would like to splice a file out of there. But, I'm not opposed
to the change, it just seems like it might cause pain down the road.
--
Kees Cook
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-18 20:07:33
On Tue, Aug 18, 2020 at 12:58:07PM -0700, Kees Cook wrote:
On Tue, Aug 18, 2020 at 09:54:46PM +0200, Christoph Hellwig wrote:
quoted
On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
quoted
On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig 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.
Signed-off-by: Christoph Hellwig <hch@lst.de>
This seems a bit disruptive? I feel like this is going to make fuzzers
really noisy (e.g. trinity likes to splice random stuff out of /sys and
/proc).
Noisy in the sence of triggering the pr_debug or because they can't
handle -EINVAL?
Well, maybe both? I doubt much _expects_ to be using splice, so I'm fine
with that, but it seems weird not to have a fall-back, especially if
something would like to splice a file out of there. But, I'm not opposed
to the change, it just seems like it might cause pain down the road.
The problem is that without pretending a buffer is in user space when
it actually isn't, we can't have a generic fallback. So we'll have to
have specific support - I wrote generic support for seq_file, and
willy did for /proc/sys, but at least the first caused a few problems
and a fair amount of churn, so I'd rather see first if we can get
away without it.
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:33: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>
---
fs/read_write.c | 67 +++++++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 25 deletions(-)
On Mon, Aug 17, 2020 at 09:32:03AM +0200, Christoph Hellwig wrote:
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>
@@ -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.
On Mon, Aug 17, 2020 at 09:32:05AM +0200, Christoph Hellwig wrote:
Add a CONFIG_SET_FS option that is selected by architecturess that
implement set_fs, which is all of them initially. If the option is not
set stubs for routines related to overriding the address space are
provided so that architectures can start to opt out of providing set_fs.
Signed-off-by: Christoph Hellwig <hch@lst.de>
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-17 07:39:59
Adding Linus as I forgot to add him to the patch bomb, sorry..
On Mon, Aug 17, 2020 at 09:32:01AM +0200, Christoph Hellwig wrote:
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:
- ensure there is no set_fs(KERNEL_DS) left in arch specific code
- implement __get_kernel_nofault and __put_kernel_nofault
- remove the arch specific address limitation functionality
Diffstat:
arch/Kconfig | 3
arch/alpha/Kconfig | 1
arch/arc/Kconfig | 1
arch/arm/Kconfig | 1
arch/arm64/Kconfig | 1
arch/c6x/Kconfig | 1
arch/csky/Kconfig | 1
arch/h8300/Kconfig | 1
arch/hexagon/Kconfig | 1
arch/ia64/Kconfig | 1
arch/m68k/Kconfig | 1
arch/microblaze/Kconfig | 1
arch/mips/Kconfig | 1
arch/nds32/Kconfig | 1
arch/nios2/Kconfig | 1
arch/openrisc/Kconfig | 1
arch/parisc/Kconfig | 1
arch/powerpc/include/asm/processor.h | 7 -
arch/powerpc/include/asm/thread_info.h | 5 -
arch/powerpc/include/asm/uaccess.h | 78 ++++++++-----------
arch/powerpc/kernel/signal.c | 3
arch/powerpc/lib/sstep.c | 6 -
arch/riscv/Kconfig | 1
arch/s390/Kconfig | 1
arch/sh/Kconfig | 1
arch/sparc/Kconfig | 1
arch/um/Kconfig | 1
arch/x86/ia32/ia32_aout.c | 1
arch/x86/include/asm/page_32_types.h | 11 ++
arch/x86/include/asm/page_64_types.h | 38 +++++++++
arch/x86/include/asm/processor.h | 60 ---------------
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 +++--
arch/xtensa/Kconfig | 1
drivers/char/mem.c | 16 ----
drivers/misc/lkdtm/bugs.c | 2
drivers/misc/lkdtm/core.c | 4 +
drivers/misc/lkdtm/usercopy.c | 2
fs/read_write.c | 69 ++++++++++-------
fs/splice.c | 130 +++------------------------------
include/linux/fs.h | 2
include/linux/uaccess.h | 18 ++++
lib/test_bitmap.c | 10 ++
46 files changed, 235 insertions(+), 332 deletions(-)
Le 17/08/2020 à 09:32, 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.
I like this series.
I gave it a go on my powerpc mpc832x. I tested it on top of my newest
series that reworks the 32 bits signal handlers (see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278)
with the microbenchmark test used is that series.
With KUAP activated, on top of signal32 rework, performance is boosted
as system time for the microbenchmark goes from 1.73s down to 1.56s,
that is 10% quicker
Surprisingly, with the kernel as is today without my signal's series,
your series degrades performance slightly (from 2.55s to 2.64s ie 3.5%
slower).
I also observe, in both cases, a degradation on
dd if=/dev/zero of=/dev/null count=1M
Without your series, it runs in 5.29 seconds.
With your series, it runs in 5.82 seconds, that is 10% more time.
Christophe
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:
- ensure there is no set_fs(KERNEL_DS) left in arch specific code
- implement __get_kernel_nofault and __put_kernel_nofault
- remove the arch specific address limitation functionality
Diffstat:
arch/Kconfig | 3
arch/alpha/Kconfig | 1
arch/arc/Kconfig | 1
arch/arm/Kconfig | 1
arch/arm64/Kconfig | 1
arch/c6x/Kconfig | 1
arch/csky/Kconfig | 1
arch/h8300/Kconfig | 1
arch/hexagon/Kconfig | 1
arch/ia64/Kconfig | 1
arch/m68k/Kconfig | 1
arch/microblaze/Kconfig | 1
arch/mips/Kconfig | 1
arch/nds32/Kconfig | 1
arch/nios2/Kconfig | 1
arch/openrisc/Kconfig | 1
arch/parisc/Kconfig | 1
arch/powerpc/include/asm/processor.h | 7 -
arch/powerpc/include/asm/thread_info.h | 5 -
arch/powerpc/include/asm/uaccess.h | 78 ++++++++-----------
arch/powerpc/kernel/signal.c | 3
arch/powerpc/lib/sstep.c | 6 -
arch/riscv/Kconfig | 1
arch/s390/Kconfig | 1
arch/sh/Kconfig | 1
arch/sparc/Kconfig | 1
arch/um/Kconfig | 1
arch/x86/ia32/ia32_aout.c | 1
arch/x86/include/asm/page_32_types.h | 11 ++
arch/x86/include/asm/page_64_types.h | 38 +++++++++
arch/x86/include/asm/processor.h | 60 ---------------
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 +++--
arch/xtensa/Kconfig | 1
drivers/char/mem.c | 16 ----
drivers/misc/lkdtm/bugs.c | 2
drivers/misc/lkdtm/core.c | 4 +
drivers/misc/lkdtm/usercopy.c | 2
fs/read_write.c | 69 ++++++++++-------
fs/splice.c | 130 +++------------------------------
include/linux/fs.h | 2
include/linux/uaccess.h | 18 ++++
lib/test_bitmap.c | 10 ++
46 files changed, 235 insertions(+), 332 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-18 18:06:04
On Tue, Aug 18, 2020 at 07:46:22PM +0200, Christophe Leroy wrote:
I gave it a go on my powerpc mpc832x. I tested it on top of my newest
series that reworks the 32 bits signal handlers (see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278) with
the microbenchmark test used is that series.
With KUAP activated, on top of signal32 rework, performance is boosted as
system time for the microbenchmark goes from 1.73s down to 1.56s, that is
10% quicker
Surprisingly, with the kernel as is today without my signal's series, your
series degrades performance slightly (from 2.55s to 2.64s ie 3.5% slower).
I also observe, in both cases, a degradation on
dd if=/dev/zero of=/dev/null count=1M
Without your series, it runs in 5.29 seconds.
With your series, it runs in 5.82 seconds, that is 10% more time.
That's pretty strage, I wonder if some kernel text cache line
effects come into play here?
The kernel access side is only used in slow path code, so it should
not make a difference, and the uaccess code is simplified and should be
(marginally) faster.
Btw, was this with the __{get,put}_user_allowed cockup that you noticed
fixed?
Le 18/08/2020 à 20:05, Christoph Hellwig a écrit :
On Tue, Aug 18, 2020 at 07:46:22PM +0200, Christophe Leroy wrote:
quoted
I gave it a go on my powerpc mpc832x. I tested it on top of my newest
series that reworks the 32 bits signal handlers (see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278) with
the microbenchmark test used is that series.
With KUAP activated, on top of signal32 rework, performance is boosted as
system time for the microbenchmark goes from 1.73s down to 1.56s, that is
10% quicker
Surprisingly, with the kernel as is today without my signal's series, your
series degrades performance slightly (from 2.55s to 2.64s ie 3.5% slower).
I also observe, in both cases, a degradation on
dd if=/dev/zero of=/dev/null count=1M
Without your series, it runs in 5.29 seconds.
With your series, it runs in 5.82 seconds, that is 10% more time.
That's pretty strage, I wonder if some kernel text cache line
effects come into play here?
The kernel access side is only used in slow path code, so it should
not make a difference, and the uaccess code is simplified and should be
(marginally) faster.
Btw, was this with the __{get,put}_user_allowed cockup that you noticed
fixed?
Yes it is with the __get_user_size() replaced by __get_user_size_allowed().
Christophe
Le 18/08/2020 à 20:05, Christoph Hellwig a écrit :
quoted
On Tue, Aug 18, 2020 at 07:46:22PM +0200, Christophe Leroy wrote:
quoted
I gave it a go on my powerpc mpc832x. I tested it on top of my newest
series that reworks the 32 bits signal handlers (see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278) with
the microbenchmark test used is that series.
With KUAP activated, on top of signal32 rework, performance is
boosted as
system time for the microbenchmark goes from 1.73s down to 1.56s,
that is
10% quicker
Surprisingly, with the kernel as is today without my signal's series,
your
series degrades performance slightly (from 2.55s to 2.64s ie 3.5%
slower).
I also observe, in both cases, a degradation on
dd if=/dev/zero of=/dev/null count=1M
Without your series, it runs in 5.29 seconds.
With your series, it runs in 5.82 seconds, that is 10% more time.
That's pretty strage, I wonder if some kernel text cache line
effects come into play here?
The kernel access side is only used in slow path code, so it should
not make a difference, and the uaccess code is simplified and should be
(marginally) faster.
Btw, was this with the __{get,put}_user_allowed cockup that you noticed
fixed?
Yes it is with the __get_user_size() replaced by __get_user_size_allowed().
I made a test with only the first patch of your series: That's
definitely the culprit. With only that patch applies, the duration is
6.64 seconds, that's a 25% degradation.
A perf record provides the following without the patch:
41.91% dd [kernel.kallsyms] [k] __arch_clear_user
7.02% dd [kernel.kallsyms] [k] vfs_read
6.86% dd [kernel.kallsyms] [k] new_sync_read
6.68% dd [kernel.kallsyms] [k] iov_iter_zero
6.03% dd [kernel.kallsyms] [k] transfer_to_syscall
3.39% dd [kernel.kallsyms] [k] memset
3.07% dd [kernel.kallsyms] [k] __fsnotify_parent
2.68% dd [kernel.kallsyms] [k] ksys_read
2.09% dd [kernel.kallsyms] [k] read_iter_zero
2.01% dd [kernel.kallsyms] [k] __fget_light
1.84% dd [kernel.kallsyms] [k] __fdget_pos
1.35% dd [kernel.kallsyms] [k] rw_verify_area
1.32% dd libc-2.23.so [.] __GI___libc_write
1.21% dd [kernel.kallsyms] [k] vfs_write
...
0.03% dd [kernel.kallsyms] [k] write_null
And the following with the patch:
15.54% dd [kernel.kallsyms] [k] __arch_clear_user
9.17% dd [kernel.kallsyms] [k] vfs_read
6.54% dd [kernel.kallsyms] [k] new_sync_write
6.31% dd [kernel.kallsyms] [k] transfer_to_syscall
6.29% dd [kernel.kallsyms] [k] __fsnotify_parent
6.20% dd [kernel.kallsyms] [k] new_sync_read
5.47% dd [kernel.kallsyms] [k] memset
5.13% dd [kernel.kallsyms] [k] vfs_write
4.44% dd [kernel.kallsyms] [k] iov_iter_zero
2.95% dd [kernel.kallsyms] [k] write_iter_null
2.82% dd [kernel.kallsyms] [k] ksys_read
2.46% dd [kernel.kallsyms] [k] __fget_light
2.34% dd libc-2.23.so [.] __GI___libc_read
1.89% dd [kernel.kallsyms] [k] iov_iter_advance
1.76% dd [kernel.kallsyms] [k] __fdget_pos
1.65% dd [kernel.kallsyms] [k] rw_verify_area
1.63% dd [kernel.kallsyms] [k] read_iter_zero
1.60% dd [kernel.kallsyms] [k] iov_iter_init
1.22% dd [kernel.kallsyms] [k] ksys_write
1.14% dd libc-2.23.so [.] __GI___libc_write
Christophe
From: Christoph Hellwig <hch@lst.de> Date: 2020-08-19 07:22:23
On Wed, Aug 19, 2020 at 09:16:59AM +0200, Christophe Leroy wrote:
I made a test with only the first patch of your series: That's definitely
the culprit. With only that patch applies, the duration is 6.64 seconds,
that's a 25% degradation.
For the record: the first patch is:
mem: remove duplicate ops for /dev/zero and /dev/null
So these micro-optimizations matter at least for some popular
benchmarks. It would be easy to drop, but that means we either:
- can't support kernel_read/write on these files, which should not
matter
or
- have to drop the check for both ops being present
Al, what do you think?