From: Christoph Hellwig <hch@lst.de> Date: 2020-09-25 04:52:02
Now that import_iovec handles compat iovecs as well, all the duplicated
code in the compat readv/writev helpers is not needed. Remove them
and switch the compat syscall handlers to use the native helpers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 +++++++----------------------------------
include/linux/compat.h | 20 ++---
2 files changed, 40 insertions(+), 159 deletions(-)
@@ -546,25 +546,25 @@ asmlinkage long compat_sys_getdents(unsigned int fd,/* fs/read_write.c */asmlinkagelongcompat_sys_lseek(unsignedint,compat_off_t,unsignedint);asmlinkagessize_tcompat_sys_readv(compat_ulong_tfd,-conststructcompat_iovec__user*vec,compat_ulong_tvlen);+conststructiovec__user*vec,compat_ulong_tvlen);asmlinkagessize_tcompat_sys_writev(compat_ulong_tfd,-conststructcompat_iovec__user*vec,compat_ulong_tvlen);+conststructiovec__user*vec,compat_ulong_tvlen);/* No generic prototype for pread64 and pwrite64 */asmlinkagessize_tcompat_sys_preadv(compat_ulong_tfd,-conststructcompat_iovec__user*vec,+conststructiovec__user*vec,compat_ulong_tvlen,u32pos_low,u32pos_high);asmlinkagessize_tcompat_sys_pwritev(compat_ulong_tfd,-conststructcompat_iovec__user*vec,+conststructiovec__user*vec,compat_ulong_tvlen,u32pos_low,u32pos_high);#ifdef __ARCH_WANT_COMPAT_SYS_PREADV64asmlinkagelongcompat_sys_preadv64(unsignedlongfd,-conststructcompat_iovec__user*vec,+conststructiovec__user*vec,unsignedlongvlen,loff_tpos);#endif#ifdef __ARCH_WANT_COMPAT_SYS_PWRITEV64asmlinkagelongcompat_sys_pwritev64(unsignedlongfd,-conststructcompat_iovec__user*vec,+conststructiovec__user*vec,unsignedlongvlen,loff_tpos);#endif
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-25 04:52:03
Split rw_copy_check_uvector into two new helpers with more sensible
calling conventions:
- iovec_from_user copies a iovec from userspace either into the provided
stack buffer if it fits, or allocates a new buffer for it. Returns
the actually used iovec. It also verifies that iov_len does fit a
signed type, and handles compat iovecs if the compat flag is set.
- __import_iovec consolidates the native and compat versions of
import_iovec. It calls iovec_from_user, then validates each iovec
actually points to user addresses, and ensures the total length
doesn't overflow.
This has two major implications:
- the access_process_vm case loses the total lenght checking, which
wasn't required anyway, given that each call receives two iovecs
for the local and remote side of the operation, and it verifies
the total length on the local side already.
- instead of a single loop there now are two loops over the iovecs.
Given that the iovecs are cache hot this doesn't make a major
difference
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/compat.h | 6 -
include/linux/fs.h | 13 --
include/linux/uio.h | 12 +-
lib/iov_iter.c | 300 ++++++++++++++++-------------------------
mm/process_vm_access.c | 34 +++--
5 files changed, 138 insertions(+), 227 deletions(-)
@@ -451,12 +451,6 @@ extern long compat_arch_ptrace(struct task_struct *child, compat_long_t request,structepoll_event;/* fortunately, this one is fixed-layout */-externssize_tcompat_rw_copy_check_uvector(inttype,-conststructcompat_iovec__user*uvector,-unsignedlongnr_segs,-unsignedlongfast_segs,structiovec*fast_pointer,-structiovec**ret_pointer);-externvoid__user*compat_alloc_user_space(unsignedlonglen);intcompat_restore_altstack(constcompat_stack_t__user*uss);
@@ -1650,107 +1651,133 @@ const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags)}EXPORT_SYMBOL(dup_iter);-/**-*rw_copy_check_uvector()-Copyanarrayof&structiovecfromuserspace-*intothekernelandcheckthatitisvalid.-*-*@type:Oneof%CHECK_IOVEC_ONLY,%READ,or%WRITE.-*@uvector:Pointertotheuserspacearray.-*@nr_segs:Numberofelementsinuserspacearray.-*@fast_segs:Numberofelementsin@fast_pointer.-*@fast_pointer:Pointerto(usuallysmallon-stack)kernelarray.-*@ret_pointer:(outputparameter)Pointertoavariablethatwillpointto-*either@fast_pointer,anewlyallocatedkernelarray,orNULL,-*dependingonwhicharraywasused.-*-*Thisfunctioncopiesanarrayof&structiovecof@nr_segsfrom-*userspaceintothekernelandchecksthateachelementisvalid(e.g.-*itdoesnotpointtoakerneladdressorcauseoverflowbybeingtoo-*large,etc.).-*-*Asanoptimization,thecallermayprovideapointertoasmall-*on-stackarrayin@fast_pointer,typically%UIO_FASTIOVelementslong-*(thesizeofthisarray,or0ifunused,shouldbegivenin@fast_segs).-*-*@ret_pointerwillalwayspointtothearraythatwasused,sothe-*callermusttakecarenottocallkfree()onite.g.incasethe-*@fast_pointerarraywasusedanditwasallocatedonthestack.-*-*Return:Thetotalnumberofbytescoveredbytheiovecarrayonsuccess-*oranegativeerrorcodeonerror.-*/-ssize_trw_copy_check_uvector(inttype,conststructiovec__user*uvector,-unsignedlongnr_segs,unsignedlongfast_segs,-structiovec*fast_pointer,structiovec**ret_pointer)+staticintcopy_compat_iovec_from_user(structiovec*iov,+conststructiovec__user*uvec,unsignedlongnr_segs)+{+conststructcompat_iovec__user*uiov=+(conststructcompat_iovec__user*)uvec;+intret=-EFAULT,i;++if(!user_access_begin(uvec,nr_segs*sizeof(*uvec)))+return-EFAULT;++for(i=0;i<nr_segs;i++){+compat_uptr_tbuf;+compat_ssize_tlen;++unsafe_get_user(len,&uiov[i].iov_len,uaccess_end);+unsafe_get_user(buf,&uiov[i].iov_base,uaccess_end);++/* check for compat_size_t not fitting in compat_ssize_t .. */+if(len<0){+ret=-EINVAL;+gotouaccess_end;+}+iov[i].iov_base=compat_ptr(buf);+iov[i].iov_len=len;+}++ret=0;+uaccess_end:+user_access_end();+returnret;+}++staticintcopy_iovec_from_user(structiovec*iov,+conststructiovec__user*uvec,unsignedlongnr_segs){unsignedlongseg;-ssize_tret;-structiovec*iov=fast_pointer;-/*-*SuSsays"The readv() function *may* fail if the iovcnt argument-*waslessthanorequalto0,orgreaterthan{IOV_MAX}.Linuxhas-*traditionallyreturnedzeroforzerosegments,so...-*/-if(nr_segs==0){-ret=0;-gotoout;+if(copy_from_user(iov,uvec,nr_segs*sizeof(*uvec)))+return-EFAULT;+for(seg=0;seg<nr_segs;seg++){+if((ssize_t)iov[seg].iov_len<0)+return-EINVAL;}+return0;+}++structiovec*iovec_from_user(conststructiovec__user*uvec,+unsignedlongnr_segs,unsignedlongfast_segs,+structiovec*fast_iov,boolcompat)+{+structiovec*iov=fast_iov;+intret;+/*-*Firstgetthe"struct iovec"fromusermemoryand-*verifyallthepointers+*SuSsays"The readv() function *may* fail if the iovcnt argument was+*lessthanorequalto0,orgreaterthan{IOV_MAX}.Linuxhas+*traditionallyreturnedzeroforzerosegments,so...*/-if(nr_segs>UIO_MAXIOV){-ret=-EINVAL;-gotoout;-}+if(nr_segs==0)+returniov;+if(nr_segs>UIO_MAXIOV)+returnERR_PTR(-EINVAL);if(nr_segs>fast_segs){iov=kmalloc_array(nr_segs,sizeof(structiovec),GFP_KERNEL);-if(iov==NULL){-ret=-ENOMEM;-gotoout;-}+if(!iov)+returnERR_PTR(-ENOMEM);}-if(copy_from_user(iov,uvector,nr_segs*sizeof(*uvector))){-ret=-EFAULT;-gotoout;++if(compat)+ret=copy_compat_iovec_from_user(iov,uvec,nr_segs);+else+ret=copy_iovec_from_user(iov,uvec,nr_segs);+if(ret){+if(iov!=fast_iov)+kfree(iov);+returnERR_PTR(ret);+}++returniov;+}++ssize_t__import_iovec(inttype,conststructiovec__user*uvec,+unsignednr_segs,unsignedfast_segs,structiovec**iovp,+structiov_iter*i,boolcompat)+{+ssize_ttotal_len=0;+unsignedlongseg;+structiovec*iov;++iov=iovec_from_user(uvec,nr_segs,fast_segs,*iovp,compat);+if(IS_ERR(iov)){+*iovp=NULL;+returnPTR_ERR(iov);}/*-*AccordingtotheSingleUnixSpecificationweshouldreturnEINVAL-*ifanelementlengthis<0whencasttossize_torifthe-*totallengthwouldoverflowthessize_treturnvalueofthe-*systemcall.+*AccordingtotheSingleUnixSpecificationweshouldreturnEINVALif+*anelementlengthis<0whencasttossize_torifthetotallength+*wouldoverflowthessize_treturnvalueofthesystemcall.**Linuxcapsallread/writecallstoMAX_RW_COUNT,andavoidsthe*overflowcase.*/-ret=0;for(seg=0;seg<nr_segs;seg++){-void__user*buf=iov[seg].iov_base;ssize_tlen=(ssize_t)iov[seg].iov_len;-/* see if we we're about to use an invalid len or if-*it'sabouttooverflowssize_t*/-if(len<0){-ret=-EINVAL;-gotoout;+if(!access_ok(iov[seg].iov_base,len)){+if(iov!=*iovp)+kfree(iov);+*iovp=NULL;+return-EFAULT;}-if(type>=0-&&unlikely(!access_ok(buf,len))){-ret=-EFAULT;-gotoout;-}-if(len>MAX_RW_COUNT-ret){-len=MAX_RW_COUNT-ret;++if(len>MAX_RW_COUNT-total_len){+len=MAX_RW_COUNT-total_len;iov[seg].iov_len=len;}-ret+=len;+total_len+=len;}-out:-*ret_pointer=iov;-returnret;++iov_iter_init(i,type,iov,nr_segs,total_len);+if(iov==*iovp)+*iovp=NULL;+else+*iovp=iov;+returntotal_len;}/**
@@ -372,8 +372,8 @@ 327 common syncfs sys_syncfs 328 common setns sys_setns 329 common sendmmsg sys_sendmmsg compat_sys_sendmmsg-330 common process_vm_readv sys_process_vm_readv compat_sys_process_vm_readv-331 common process_vm_writev sys_process_vm_writev compat_sys_process_vm_writev+330 common process_vm_readv sys_process_vm_readv+331 common process_vm_writev sys_process_vm_writev 332 common kcmp sys_kcmp 333 common finit_module sys_finit_module 334 common sched_setattr sys_sched_setattr
@@ -347,8 +347,8 @@ 337 common clock_adjtime sys_clock_adjtime sys_clock_adjtime32 338 common syncfs sys_syncfs sys_syncfs 339 common setns sys_setns sys_setns-340 common process_vm_readv sys_process_vm_readv compat_sys_process_vm_readv-341 common process_vm_writev sys_process_vm_writev compat_sys_process_vm_writev+340 common process_vm_readv sys_process_vm_readv sys_process_vm_readv+341 common process_vm_writev sys_process_vm_writev sys_process_vm_writev 342 common s390_runtime_instr sys_s390_runtime_instr sys_s390_runtime_instr 343 common kcmp sys_kcmp sys_kcmp 344 common finit_module sys_finit_module sys_finit_module
@@ -347,8 +347,8 @@ 337 common clock_adjtime sys_clock_adjtime compat_sys_clock_adjtime 338 common syncfs sys_syncfs sys_syncfs 339 common setns sys_setns sys_setns-340 common process_vm_readv sys_process_vm_readv compat_sys_process_vm_readv-341 common process_vm_writev sys_process_vm_writev compat_sys_process_vm_writev+340 common process_vm_readv sys_process_vm_readv sys_process_vm_readv+341 common process_vm_writev sys_process_vm_writev sys_process_vm_writev 342 common s390_runtime_instr sys_s390_runtime_instr sys_s390_runtime_instr 343 common kcmp sys_kcmp compat_sys_kcmp 344 common finit_module sys_finit_module compat_sys_finit_module
@@ -159,8 +159,8 @@ 142 common _newselect sys_select compat_sys_select 143 common flock sys_flock 144 common msync sys_msync-145 common readv sys_readv compat_sys_readv-146 common writev sys_writev compat_sys_writev+145 common readv sys_readv+146 common writev sys_writev 147 common getsid sys_getsid 148 common fdatasync sys_fdatasync 149 common _sysctl sys_ni_syscall
@@ -193,8 +193,8 @@ 142 common _newselect sys_select compat_sys_select 143 common flock sys_flock 144 common msync sys_msync-145 common readv sys_readv compat_sys_readv-146 common writev sys_writev compat_sys_writev+145 common readv sys_readv+146 common writev sys_writev 147 common getsid sys_getsid 148 common fdatasync sys_fdatasync 149 nospu _sysctl sys_ni_syscall
@@ -545,10 +545,6 @@ asmlinkage long compat_sys_getdents(unsigned int fd,/* fs/read_write.c */asmlinkagelongcompat_sys_lseek(unsignedint,compat_off_t,unsignedint);-asmlinkagessize_tcompat_sys_readv(compat_ulong_tfd,-conststructiovec__user*vec,compat_ulong_tvlen);-asmlinkagessize_tcompat_sys_writev(compat_ulong_tfd,-conststructiovec__user*vec,compat_ulong_tvlen);/* No generic prototype for pread64 and pwrite64 */asmlinkagessize_tcompat_sys_preadv(compat_ulong_tfd,conststructiovec__user*vec,
@@ -189,8 +189,8 @@ 142 common _newselect sys_select compat_sys_select 143 common flock sys_flock 144 common msync sys_msync-145 common readv sys_readv compat_sys_readv-146 common writev sys_writev compat_sys_writev+145 common readv sys_readv+146 common writev sys_writev 147 common getsid sys_getsid 148 common fdatasync sys_fdatasync 149 nospu _sysctl sys_ni_syscall
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-25 04:53:02
Use in compat_syscall to import either native or the compat iovecs, and
remove the now superflous compat_import_iovec.
This removes the need for special compat logic in most callers, and
the remaining ones can still be simplified by using __import_iovec
with a bool compat parameter.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/scsi_ioctl.c | 12 ++----------
drivers/scsi/sg.c | 9 +--------
fs/aio.c | 8 ++------
fs/io_uring.c | 20 ++++++++------------
fs/read_write.c | 6 ++++--
fs/splice.c | 2 +-
include/linux/uio.h | 8 --------
lib/iov_iter.c | 14 ++------------
mm/process_vm_access.c | 3 ++-
net/compat.c | 4 ++--
security/keys/compat.c | 5 ++---
11 files changed, 26 insertions(+), 65 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-25 04:53:08
Now that import_iovec handles compat iovecs, the native version of
keyctl_instantiate_key_iov can be used for the compat case as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
security/keys/compat.c | 36 ++----------------------------------
security/keys/internal.h | 5 -----
security/keys/keyctl.c | 2 +-
3 files changed, 3 insertions(+), 40 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-25 04:53:11
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
@@ -752,185 +752,6 @@ static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter,returnret;}-/**-*rw_copy_check_uvector()-Copyanarrayof&structiovecfromuserspace-*intothekernelandcheckthatitisvalid.-*-*@type:Oneof%CHECK_IOVEC_ONLY,%READ,or%WRITE.-*@uvector:Pointertotheuserspacearray.-*@nr_segs:Numberofelementsinuserspacearray.-*@fast_segs:Numberofelementsin@fast_pointer.-*@fast_pointer:Pointerto(usuallysmallon-stack)kernelarray.-*@ret_pointer:(outputparameter)Pointertoavariablethatwillpointto-*either@fast_pointer,anewlyallocatedkernelarray,orNULL,-*dependingonwhicharraywasused.-*-*Thisfunctioncopiesanarrayof&structiovecof@nr_segsfrom-*userspaceintothekernelandchecksthateachelementisvalid(e.g.-*itdoesnotpointtoakerneladdressorcauseoverflowbybeingtoo-*large,etc.).-*-*Asanoptimization,thecallermayprovideapointertoasmall-*on-stackarrayin@fast_pointer,typically%UIO_FASTIOVelementslong-*(thesizeofthisarray,or0ifunused,shouldbegivenin@fast_segs).-*-*@ret_pointerwillalwayspointtothearraythatwasused,sothe-*callermusttakecarenottocallkfree()onite.g.incasethe-*@fast_pointerarraywasusedanditwasallocatedonthestack.-*-*Return:Thetotalnumberofbytescoveredbytheiovecarrayonsuccess-*oranegativeerrorcodeonerror.-*/-ssize_trw_copy_check_uvector(inttype,conststructiovec__user*uvector,-unsignedlongnr_segs,unsignedlongfast_segs,-structiovec*fast_pointer,-structiovec**ret_pointer)-{-unsignedlongseg;-ssize_tret;-structiovec*iov=fast_pointer;--/*-*SuSsays"The readv() function *may* fail if the iovcnt argument-*waslessthanorequalto0,orgreaterthan{IOV_MAX}.Linuxhas-*traditionallyreturnedzeroforzerosegments,so...-*/-if(nr_segs==0){-ret=0;-gotoout;-}--/*-*Firstgetthe"struct iovec"fromusermemoryand-*verifyallthepointers-*/-if(nr_segs>UIO_MAXIOV){-ret=-EINVAL;-gotoout;-}-if(nr_segs>fast_segs){-iov=kmalloc_array(nr_segs,sizeof(structiovec),GFP_KERNEL);-if(iov==NULL){-ret=-ENOMEM;-gotoout;-}-}-if(copy_from_user(iov,uvector,nr_segs*sizeof(*uvector))){-ret=-EFAULT;-gotoout;-}--/*-*AccordingtotheSingleUnixSpecificationweshouldreturnEINVAL-*ifanelementlengthis<0whencasttossize_torifthe-*totallengthwouldoverflowthessize_treturnvalueofthe-*systemcall.-*-*Linuxcapsallread/writecallstoMAX_RW_COUNT,andavoidsthe-*overflowcase.-*/-ret=0;-for(seg=0;seg<nr_segs;seg++){-void__user*buf=iov[seg].iov_base;-ssize_tlen=(ssize_t)iov[seg].iov_len;--/* see if we we're about to use an invalid len or if-*it'sabouttooverflowssize_t*/-if(len<0){-ret=-EINVAL;-gotoout;-}-if(type>=0-&&unlikely(!access_ok(buf,len))){-ret=-EFAULT;-gotoout;-}-if(len>MAX_RW_COUNT-ret){-len=MAX_RW_COUNT-ret;-iov[seg].iov_len=len;-}-ret+=len;-}-out:-*ret_pointer=iov;-returnret;-}--#ifdef CONFIG_COMPAT-ssize_tcompat_rw_copy_check_uvector(inttype,-conststructcompat_iovec__user*uvector,unsignedlongnr_segs,-unsignedlongfast_segs,structiovec*fast_pointer,-structiovec**ret_pointer)-{-compat_ssize_ttot_len;-structiovec*iov=*ret_pointer=fast_pointer;-ssize_tret=0;-intseg;--/*-*SuSsays"The readv() function *may* fail if the iovcnt argument-*waslessthanorequalto0,orgreaterthan{IOV_MAX}.Linuxhas-*traditionallyreturnedzeroforzerosegments,so...-*/-if(nr_segs==0)-gotoout;--ret=-EINVAL;-if(nr_segs>UIO_MAXIOV)-gotoout;-if(nr_segs>fast_segs){-ret=-ENOMEM;-iov=kmalloc_array(nr_segs,sizeof(structiovec),GFP_KERNEL);-if(iov==NULL)-gotoout;-}-*ret_pointer=iov;--ret=-EFAULT;-if(!access_ok(uvector,nr_segs*sizeof(*uvector)))-gotoout;--/*-*Singleunixspecification:-*Weshould-EINVALifanelementlengthisnot>=0andfittingan-*ssize_t.-*-*InLinux,thetotallengthislimitedtoMAX_RW_COUNT,thereis-*nooverflowpossibility.-*/-tot_len=0;-ret=-EINVAL;-for(seg=0;seg<nr_segs;seg++){-compat_uptr_tbuf;-compat_ssize_tlen;--if(__get_user(len,&uvector->iov_len)||-__get_user(buf,&uvector->iov_base)){-ret=-EFAULT;-gotoout;-}-if(len<0)/* size_t not fitting in compat_ssize_t .. */-gotoout;-if(type>=0&&-!access_ok(compat_ptr(buf),len)){-ret=-EFAULT;-gotoout;-}-if(len>MAX_RW_COUNT-tot_len)-len=MAX_RW_COUNT-tot_len;-tot_len+=len;-iov->iov_base=compat_ptr(buf);-iov->iov_len=(compat_size_t)len;-uvector++;-iov++;-}-ret=tot_len;--out:-returnret;-}-#endif-staticssize_tdo_iter_read(structfile*file,structiov_iter*iter,loff_t*pos,rwf_tflags){
@@ -1650,6 +1650,109 @@ const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags)}EXPORT_SYMBOL(dup_iter);+/**+*rw_copy_check_uvector()-Copyanarrayof&structiovecfromuserspace+*intothekernelandcheckthatitisvalid.+*+*@type:Oneof%CHECK_IOVEC_ONLY,%READ,or%WRITE.+*@uvector:Pointertotheuserspacearray.+*@nr_segs:Numberofelementsinuserspacearray.+*@fast_segs:Numberofelementsin@fast_pointer.+*@fast_pointer:Pointerto(usuallysmallon-stack)kernelarray.+*@ret_pointer:(outputparameter)Pointertoavariablethatwillpointto+*either@fast_pointer,anewlyallocatedkernelarray,orNULL,+*dependingonwhicharraywasused.+*+*Thisfunctioncopiesanarrayof&structiovecof@nr_segsfrom+*userspaceintothekernelandchecksthateachelementisvalid(e.g.+*itdoesnotpointtoakerneladdressorcauseoverflowbybeingtoo+*large,etc.).+*+*Asanoptimization,thecallermayprovideapointertoasmall+*on-stackarrayin@fast_pointer,typically%UIO_FASTIOVelementslong+*(thesizeofthisarray,or0ifunused,shouldbegivenin@fast_segs).+*+*@ret_pointerwillalwayspointtothearraythatwasused,sothe+*callermusttakecarenottocallkfree()onite.g.incasethe+*@fast_pointerarraywasusedanditwasallocatedonthestack.+*+*Return:Thetotalnumberofbytescoveredbytheiovecarrayonsuccess+*oranegativeerrorcodeonerror.+*/+ssize_trw_copy_check_uvector(inttype,conststructiovec__user*uvector,+unsignedlongnr_segs,unsignedlongfast_segs,+structiovec*fast_pointer,structiovec**ret_pointer)+{+unsignedlongseg;+ssize_tret;+structiovec*iov=fast_pointer;++/*+*SuSsays"The readv() function *may* fail if the iovcnt argument+*waslessthanorequalto0,orgreaterthan{IOV_MAX}.Linuxhas+*traditionallyreturnedzeroforzerosegments,so...+*/+if(nr_segs==0){+ret=0;+gotoout;+}++/*+*Firstgetthe"struct iovec"fromusermemoryand+*verifyallthepointers+*/+if(nr_segs>UIO_MAXIOV){+ret=-EINVAL;+gotoout;+}+if(nr_segs>fast_segs){+iov=kmalloc_array(nr_segs,sizeof(structiovec),GFP_KERNEL);+if(iov==NULL){+ret=-ENOMEM;+gotoout;+}+}+if(copy_from_user(iov,uvector,nr_segs*sizeof(*uvector))){+ret=-EFAULT;+gotoout;+}++/*+*AccordingtotheSingleUnixSpecificationweshouldreturnEINVAL+*ifanelementlengthis<0whencasttossize_torifthe+*totallengthwouldoverflowthessize_treturnvalueofthe+*systemcall.+*+*Linuxcapsallread/writecallstoMAX_RW_COUNT,andavoidsthe+*overflowcase.+*/+ret=0;+for(seg=0;seg<nr_segs;seg++){+void__user*buf=iov[seg].iov_base;+ssize_tlen=(ssize_t)iov[seg].iov_len;++/* see if we we're about to use an invalid len or if+*it'sabouttooverflowssize_t*/+if(len<0){+ret=-EINVAL;+gotoout;+}+if(type>=0+&&unlikely(!access_ok(buf,len))){+ret=-EFAULT;+gotoout;+}+if(len>MAX_RW_COUNT-ret){+len=MAX_RW_COUNT-ret;+iov[seg].iov_len=len;+}+ret+=len;+}+out:+*ret_pointer=iov;+returnret;+}+/***import_iovec()-Copyanarrayof&structiovecfromuserspace*intothekernel,checkthatitisvalid,andinitializeanew
@@ -1695,6 +1798,79 @@ EXPORT_SYMBOL(import_iovec);#ifdef CONFIG_COMPAT#include<linux/compat.h>+ssize_tcompat_rw_copy_check_uvector(inttype,+conststructcompat_iovec__user*uvector,+unsignedlongnr_segs,unsignedlongfast_segs,+structiovec*fast_pointer,structiovec**ret_pointer)+{+compat_ssize_ttot_len;+structiovec*iov=*ret_pointer=fast_pointer;+ssize_tret=0;+intseg;++/*+*SuSsays"The readv() function *may* fail if the iovcnt argument+*waslessthanorequalto0,orgreaterthan{IOV_MAX}.Linuxhas+*traditionallyreturnedzeroforzerosegments,so...+*/+if(nr_segs==0)+gotoout;++ret=-EINVAL;+if(nr_segs>UIO_MAXIOV)+gotoout;+if(nr_segs>fast_segs){+ret=-ENOMEM;+iov=kmalloc_array(nr_segs,sizeof(structiovec),GFP_KERNEL);+if(iov==NULL)+gotoout;+}+*ret_pointer=iov;++ret=-EFAULT;+if(!access_ok(uvector,nr_segs*sizeof(*uvector)))+gotoout;++/*+*Singleunixspecification:+*Weshould-EINVALifanelementlengthisnot>=0andfittingan+*ssize_t.+*+*InLinux,thetotallengthislimitedtoMAX_RW_COUNT,thereis+*nooverflowpossibility.+*/+tot_len=0;+ret=-EINVAL;+for(seg=0;seg<nr_segs;seg++){+compat_uptr_tbuf;+compat_ssize_tlen;++if(__get_user(len,&uvector->iov_len)||+__get_user(buf,&uvector->iov_base)){+ret=-EFAULT;+gotoout;+}+if(len<0)/* size_t not fitting in compat_ssize_t .. */+gotoout;+if(type>=0&&+!access_ok(compat_ptr(buf),len)){+ret=-EFAULT;+gotoout;+}+if(len>MAX_RW_COUNT-tot_len)+len=MAX_RW_COUNT-tot_len;+tot_len+=len;+iov->iov_base=compat_ptr(buf);+iov->iov_len=(compat_size_t)len;+uvector++;+iov++;+}+ret=tot_len;++out:+returnret;+}+ssize_tcompat_import_iovec(inttype,conststructcompat_iovec__user*uvector,unsignednr_segs,unsignedfast_segs,
From: Christoph Hellwig <hch@lst.de> Date: 2020-09-25 04:53:13
There is no compat_sys_readv64v2 syscall, only a compat_sys_preadv64v2
one.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/compat.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -369,7 +369,7 @@ 282 common unshare sys_unshare 283 common splice sys_splice 284 common tee sys_tee-285 common vmsplice sys_vmsplice compat_sys_vmsplice+285 common vmsplice sys_vmsplice 286 common openat sys_openat compat_sys_openat 287 common mkdirat sys_mkdirat 288 common mknodat sys_mknodat
@@ -316,7 +316,7 @@ 306 common splice sys_splice sys_splice 307 common sync_file_range sys_sync_file_range compat_sys_s390_sync_file_range 308 common tee sys_tee sys_tee-309 common vmsplice sys_vmsplice compat_sys_vmsplice+309 common vmsplice sys_vmsplice sys_vmsplice 310 common move_pages sys_move_pages compat_sys_move_pages 311 common getcpu sys_getcpu sys_getcpu 312 common epoll_pwait sys_epoll_pwait compat_sys_epoll_pwait
@@ -363,7 +363,7 @@ 282 common unshare sys_unshare 283 common splice sys_splice 284 common tee sys_tee-285 common vmsplice sys_vmsplice compat_sys_vmsplice+285 common vmsplice sys_vmsplice 286 common openat sys_openat compat_sys_openat 287 common mkdirat sys_mkdirat 288 common mknodat sys_mknodat
@@ -316,7 +316,7 @@ 306 common splice sys_splice compat_sys_splice 307 common sync_file_range sys_sync_file_range compat_sys_s390_sync_file_range 308 common tee sys_tee compat_sys_tee-309 common vmsplice sys_vmsplice compat_sys_vmsplice+309 common vmsplice sys_vmsplice sys_vmsplice 310 common move_pages sys_move_pages compat_sys_move_pages 311 common getcpu sys_getcpu compat_sys_getcpu 312 common epoll_pwait sys_epoll_pwait compat_sys_epoll_pwait
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
thanks,
greg k-h
From: David Laight <hidden> Date: 2020-10-21 20:59:51
From: Greg KH
Sent: 21 October 2020 17:13
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
My original commit just moved the function source from one file to another.
So it is odd that it makes any difference.
I don't even know if it gets inlined by Christoph's actual patch.
(I have another patch that depended on it that I need to resubmit.)
Some of the other changes from Christoph's same patch set might
make a difference though.
Might be worth forcing it to be not inlined - so it is no change.
Or try adding a kernel log to import_iovec() or the associated
copy failing.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-21 23:39:39
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Nick, any ideas here as to who I should report this to?
I'll work on a fixup patch for the Android kernel tree to see if I can
work around it there, but others will hit this in Linus's tree sooner or
later...
thanks,
greg k-h
From: David Hildenbrand <hidden> Date: 2020-10-22 08:35:42
On 22.10.20 10:26, Greg KH wrote:
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
--
Thanks,
David / dhildenb
From: David Laight <hidden> Date: 2020-10-22 08:40:46
From: David Hildenbrand
Sent: 22 October 2020 09:35
On 22.10.20 10:26, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Hildenbrand <hidden> Date: 2020-10-22 08:49:17
On 22.10.20 10:40, David Laight wrote:
From: David Hildenbrand
quoted
Sent: 22 October 2020 09:35
On 22.10.20 10:26, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
--
Thanks,
David / dhildenb
On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
On 22.10.20 10:40, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 09:35
On 22.10.20 10:26, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
I agree, I hate to blame the compiler, that's almost never the real
problem, but this one sure "feels" like it.
I'm running some more tests, trying to narrow things down as just adding
a "noinline" to the function that got moved here doesn't work on Linus's
tree at the moment because the function was split into multiple
functions.
Give me a few hours...
thanks,
greg k-h
From: David Laight <hidden> Date: 2020-10-22 09:03:02
From: David Hildenbrand
Sent: 22 October 2020 09:49
...
quoted
quoted
quoted
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
Usually down to missing asm 'memory' constraints...
Need to read the obj file to see what the compiler did.
The code must be 'approximately right' or nothing would run.
So I'd guess it has to do with > 8 fragments.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-10-22 09:06:47
From: Greg KH
Sent: 22 October 2020 10:02
...
I'm running some more tests, trying to narrow things down as just adding
a "noinline" to the function that got moved here doesn't work on Linus's
tree at the moment because the function was split into multiple
functions.
I was going to look at that once rc2 is in - and the kernel is
likely to work.
I suspect the split version doesn't get inlined the same way?
Which leaves the horrid argument conversion the inline got
rid of back there again.
Which all rather begs the question of why the compiler doesn't
generate the expected code.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Oct 22, 2020 at 10:26 AM Greg KH [off-list ref] wrote:
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Nick, any ideas here as to who I should report this to?
I'll work on a fixup patch for the Android kernel tree to see if I can
work around it there, but others will hit this in Linus's tree sooner or
later...
I see that Christoph rewrote the function again in bfdc59701d6d
("iov_iter: refactor rw_copy_check_uvector and import_iovec"),
do you know if the current mainline version is also affected?
Do you know if it happens across multiple architectures or might
be specific to either x86 or arm64?
https://bugs.llvm.org/ is obviously the place for reporting the
issue if it does turn out to be a bug in clang, but that requires
a specific thing going wrong in the output.
One idea I have for debugging it is to sprinkle the inlined
function with lots of barrier()s to prevent a lot of the optimizations.
If that solves the issue, you can bisect through those until you
find one barrier that makes the difference between working and
broken and then look at diff of the assembler output.
Arnd
On Thu, Oct 22, 2020 at 10:26 AM Greg KH [off-list ref] wrote:
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Nick, any ideas here as to who I should report this to?
I'll work on a fixup patch for the Android kernel tree to see if I can
work around it there, but others will hit this in Linus's tree sooner or
later...
I see that Christoph rewrote the function again in bfdc59701d6d
("iov_iter: refactor rw_copy_check_uvector and import_iovec"),
do you know if the current mainline version is also affected?
Do you know if it happens across multiple architectures or might
be specific to either x86 or arm64?
https://bugs.llvm.org/ is obviously the place for reporting the
issue if it does turn out to be a bug in clang, but that requires
a specific thing going wrong in the output.
One idea I have for debugging it is to sprinkle the inlined
function with lots of barrier()s to prevent a lot of the optimizations.
If that solves the issue, you can bisect through those until you
find one barrier that makes the difference between working and
broken and then look at diff of the assembler output.
Arnd
From: David Hildenbrand <hidden> Date: 2020-10-22 09:19:32
On 22.10.20 11:01, Greg KH wrote:
On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 10:40, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 09:35
On 22.10.20 10:26, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
I agree, I hate to blame the compiler, that's almost never the real
problem, but this one sure "feels" like it.
I'm running some more tests, trying to narrow things down as just adding
a "noinline" to the function that got moved here doesn't work on Linus's
tree at the moment because the function was split into multiple
functions.
Give me a few hours...
I might be wrong but
a) import_iovec() uses:
- unsigned nr_segs -> int
- unsigned fast_segs -> int
b) rw_copy_check_uvector() uses:
- unsigned long nr_segs -> long
- unsigned long fast_seg -> long
So when calling rw_copy_check_uvector(), we have to zero-extend the
registers used for passing the arguments. That's definitely done when
calling the function explicitly. Maybe when inlining something is messed up?
Just a thought ...
--
Thanks,
David / dhildenb
From: David Hildenbrand <hidden> Date: 2020-10-22 09:25:49
On 22.10.20 11:19, David Hildenbrand wrote:
On 22.10.20 11:01, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 10:40, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 09:35
On 22.10.20 10:26, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
I agree, I hate to blame the compiler, that's almost never the real
problem, but this one sure "feels" like it.
I'm running some more tests, trying to narrow things down as just adding
a "noinline" to the function that got moved here doesn't work on Linus's
tree at the moment because the function was split into multiple
functions.
Give me a few hours...
I might be wrong but
a) import_iovec() uses:
- unsigned nr_segs -> int
- unsigned fast_segs -> int
b) rw_copy_check_uvector() uses:
- unsigned long nr_segs -> long
- unsigned long fast_seg -> long
So when calling rw_copy_check_uvector(), we have to zero-extend the
registers used for passing the arguments. That's definitely done when
calling the function explicitly. Maybe when inlining something is messed up?
Just a thought ...
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
Again, just a thought.
--
Thanks,
David / dhildenb
From: David Laight <hidden> Date: 2020-10-22 09:28:28
From: David Hildenbrand
Sent: 22 October 2020 10:19
On 22.10.20 11:01, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 10:40, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 09:35
On 22.10.20 10:26, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:39:14AM +0100, Al Viro wrote:
quoted
On Wed, Oct 21, 2020 at 06:13:01PM +0200, Greg KH wrote:
quoted
On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote:
quoted
From: David Laight <redacted>
This lets the compiler inline it into import_iovec() generating
much better code.
Signed-off-by: David Laight <redacted>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/read_write.c | 179 ------------------------------------------------
lib/iov_iter.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 179 deletions(-)
Strangely, this commit causes a regression in Linus's tree right now.
I can't really figure out what the regression is, only that this commit
triggers a "large Android system binary" from working properly. There's
no kernel log messages anywhere, and I don't have any way to strace the
thing in the testing framework, so any hints that people can provide
would be most appreciated.
It's a pure move - modulo changed line breaks in the argument lists
the functions involved are identical before and after that (just checked
that directly, by checking out the trees before and after, extracting two
functions in question from fs/read_write.c and lib/iov_iter.c (before and
after, resp.) and checking the diff between those.
How certain is your bisection?
The bisection is very reproducable.
But, this looks now to be a compiler bug. I'm using the latest version
of clang and if I put "noinline" at the front of the function,
everything works.
Well, the compiler can do more invasive optimizations when inlining. If
you have buggy code that relies on some unspecified behavior, inlining
can change the behavior ... but going over that code, there isn't too
much action going on. At least nothing screamed at me.
Apart from all the optimisations that get rid off the 'pass be reference'
parameters and strange conditional tests.
Plenty of scope for the compiler getting it wrong.
But nothing even vaguely illegal.
Not the first time that people blame the compiler to then figure out
that something else is wrong ... but maybe this time is different :)
I agree, I hate to blame the compiler, that's almost never the real
problem, but this one sure "feels" like it.
I'm running some more tests, trying to narrow things down as just adding
a "noinline" to the function that got moved here doesn't work on Linus's
tree at the moment because the function was split into multiple
functions.
Give me a few hours...
I might be wrong but
a) import_iovec() uses:
- unsigned nr_segs -> int
- unsigned fast_segs -> int
b) rw_copy_check_uvector() uses:
- unsigned long nr_segs -> long
- unsigned long fast_seg -> long
So when calling rw_copy_check_uvector(), we have to zero-extend the
registers used for passing the arguments. That's definitely done when
calling the function explicitly. Maybe when inlining something is messed up?
That's also not needed on x86-64 - the high bits get cleared by 32bit writes.
But, IIRC, arm64 leaves them unchanged or undefined.
I guessing that every array access uses a *(Rx + Ry) addressing
mode. So indexing an array even with 'unsigned int' requires
an explicit zero-extend on arm64?
(x86-64 ends up with an explicit sign extend when indexing an
array with 'signed int'.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-10-22 09:33:12
From: David Hildenbrand
Sent: 22 October 2020 10:25
...
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Hildenbrand <hidden> Date: 2020-10-22 09:37:00
On 22.10.20 11:32, David Laight wrote:
From: David Hildenbrand
quoted
Sent: 22 October 2020 10:25
...
quoted
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
quoted
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
--
Thanks,
David / dhildenb
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
On 22.10.20 11:32, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 10:25
...
quoted
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
quoted
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
Let me try that as well, as I seem to have a good reproducer, but it
takes a while to run...
greg k-h
On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 11:32, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 10:25
...
quoted
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
quoted
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
Let me try that as well, as I seem to have a good reproducer, but it
takes a while to run...
Ok, that didn't work.
And I can't seem to "fix" this by adding noinline to patches further
along in the patch series (because this commit's function is no longer
present due to later patches.)
Will keep digging...
greg k-h
From: David Hildenbrand <hidden> Date: 2020-10-22 12:42:48
On 22.10.20 14:18, Greg KH wrote:
On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 11:32, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 10:25
...
quoted
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
quoted
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
Let me try that as well, as I seem to have a good reproducer, but it
takes a while to run...
Ok, that didn't work.
And I can't seem to "fix" this by adding noinline to patches further
along in the patch series (because this commit's function is no longer
present due to later patches.)
We might have the same issues with iovec_from_user() and friends now.
Will keep digging...
greg k-h
Might be worth to give this a try, just to see if it's related to
garbage in upper 32 bit and the way clang is handling it (might be a BUG
in clang, though):
On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
quoted hunk
On 22.10.20 14:18, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 11:32, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 10:25
...
quoted
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
quoted
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
Let me try that as well, as I seem to have a good reproducer, but it
takes a while to run...
Ok, that didn't work.
And I can't seem to "fix" this by adding noinline to patches further
along in the patch series (because this commit's function is no longer
present due to later patches.)
We might have the same issues with iovec_from_user() and friends now.
quoted
Will keep digging...
greg k-h
Might be worth to give this a try, just to see if it's related to
garbage in upper 32 bit and the way clang is handling it (might be a BUG
in clang, though):
Ah, I tested the other way around, making everything "unsigned long"
instead. Will go try this too, as other tests are still running...
thanks,
greg k-h
From: Christoph Hellwig <hch@lst.de> Date: 2020-10-22 13:23:55
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
Most callchains of import_iovec start with the assembly syscall wrappers.
On Thu, Oct 22, 2020 at 02:57:59PM +0200, Greg KH wrote:
On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 14:18, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
quoted
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
quoted
On 22.10.20 11:32, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 22 October 2020 10:25
...
quoted
... especially because I recall that clang and gcc behave slightly
differently:
https://github.com/hjl-tools/x86-psABI/issues/2
"Function args are different: narrow types are sign or zero extended to
32 bits, depending on their type. clang depends on this for incoming
args, but gcc doesn't make that assumption. But both compilers do it
when calling, so gcc code can call clang code.
It really is best to use 'int' (or even 'long') for all numeric
arguments (and results) regardless of the domain of the value.
Related, I've always worried about 'bool'....
quoted
The upper 32 bits of registers are always undefined garbage for types
smaller than 64 bits."
On x86-64 the high bits are zeroed by all 32bit loads.
Yeah, but does not help here.
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
We can test easily by changing the parameters instead of adding an "inline".
Let me try that as well, as I seem to have a good reproducer, but it
takes a while to run...
Ok, that didn't work.
And I can't seem to "fix" this by adding noinline to patches further
along in the patch series (because this commit's function is no longer
present due to later patches.)
We might have the same issues with iovec_from_user() and friends now.
quoted
Will keep digging...
greg k-h
Might be worth to give this a try, just to see if it's related to
garbage in upper 32 bit and the way clang is handling it (might be a BUG
in clang, though):
Ah, I tested the other way around, making everything "unsigned long"
instead. Will go try this too, as other tests are still running...
Ok, no, this didn't work either.
Nick, I think I need some compiler help here. Any ideas?
I don't think the patch above would reliably clear the upper bits if they
contain garbage.
If the integer extension is the problem, the way I'd try it is to make the
function take an 'unsigned long' and then explictly mask the upper
bits with
seg = lower_32_bits(seg);
Can you attach the iov_iter.s files from the broken build, plus the
one with 'noinline' for comparison? Maybe something can be seen
in there.
Arnd
Ah, I tested the other way around, making everything "unsigned long"
instead. Will go try this too, as other tests are still running...
Ok, no, this didn't work either.
Nick, I think I need some compiler help here. Any ideas?
I don't think the patch above would reliably clear the upper bits if they
contain garbage.
If the integer extension is the problem, the way I'd try it is to make the
function take an 'unsigned long' and then explictly mask the upper
bits with
seg = lower_32_bits(seg);
Can you attach the iov_iter.s files from the broken build, plus the
one with 'noinline' for comparison? Maybe something can be seen
in there.
I don't know how to extract the .s files easily from the AOSP build
system, I'll look into that. I'm also now testing by downgrading to an
older version of clang (10 instead of 11), to see if that matters at all
or not...
thanks,
greg k-h
From: David Laight <hidden> Date: 2020-10-22 16:15:19
From: Greg KH
Sent: 22 October 2020 15:40
On Thu, Oct 22, 2020 at 04:28:20PM +0200, Arnd Bergmann wrote:
...
quoted
Can you attach the iov_iter.s files from the broken build, plus the
one with 'noinline' for comparison? Maybe something can be seen
in there.
I don't know how to extract the .s files easily from the AOSP build
system, I'll look into that. I'm also now testing by downgrading to an
older version of clang (10 instead of 11), to see if that matters at all
or not...
Back from a day out - after it stopped raining.
Trying to use up leave before the end of the year.
Can you use objdump on the kernel binary itself and cut out
the single function?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-10-22 16:35:32
From: Christoph Hellwig
Sent: 22 October 2020 14:24
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
quoted
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
Most callchains of import_iovec start with the assembly syscall wrappers.
Wait...
readv(2) defines:
ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
But the syscall is defined as:
SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen)
{
return do_readv(fd, vec, vlen, 0);
}
I'm guessing that nothing actually masks the high bits that come
from an application that is compiled with clang?
The vlen is 'unsigned long' through the first few calls.
So unless there is a non-inlined function than takes vlen
as 'int' the high garbage bits from userspace are kept.
Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.
What does the ARM EABI say about register parameters?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
It doesn't really matter what the manpage says. What does the AOSP
libc header say?
The only copy I can find is:
/usr/include/x86_64-linux-gnu/sys/uio.h:extern ssize_t readv (int __fd, const struct iovec *__iovec, int __count)
/usr/include/x86_64-linux-gnu/sys/uio.h- __wur;
and not surprisingly agrees.
POSIX and/or TOG will (more or less) mandate the prototype.
quoted
But the syscall is defined as:
SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen)
{
return do_readv(fd, vec, vlen, 0);
}
I wonder when the high bits of 'fd' get zapped?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2020-10-22 17:54:27
On Thu, Oct 22, 2020 at 9:35 AM David Laight [off-list ref] wrote:
From: Christoph Hellwig
quoted
Sent: 22 October 2020 14:24
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
quoted
My thinking: if the compiler that calls import_iovec() has garbage in
the upper 32 bit
a) gcc will zero it out and not rely on it being zero.
b) clang will not zero it out, assuming it is zero.
But
a) will zero it out when calling the !inlined variant
b) clang will zero it out when calling the !inlined variant
When inlining, b) strikes. We access garbage. That would mean that we
have calling code that's not generated by clang/gcc IIUC.
Most callchains of import_iovec start with the assembly syscall wrappers.
Wait...
readv(2) defines:
ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
But the syscall is defined as:
SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen)
{
return do_readv(fd, vec, vlen, 0);
}
I'm guessing that nothing actually masks the high bits that come
from an application that is compiled with clang?
The vlen is 'unsigned long' through the first few calls.
So unless there is a non-inlined function than takes vlen
as 'int' the high garbage bits from userspace are kept.
Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.
Why not x86-64? Wouldn't it be *any* LP64 ISA?
Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue . Greg, would you mind running this
through the wringer?
What does the ARM EABI say about register parameters?
AAPCS is the ABI for 64b ARM, IIUC, which is the ISA GKH is reporting
the problem against. IIUC, EABI is one of the 32b ABIs. aarch64 is
LP64 just like x86_64.
--
Thanks,
~Nick Desaulniers
On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
[off-list ref] wrote:
On Thu, Oct 22, 2020 at 9:35 AM David Laight [off-list ref] wrote:
quoted
Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.
Why not x86-64? Wouldn't it be *any* LP64 ISA?
x86-64 is slightly special because most instructions on a 32-bit
argument clear the upper 32 bits, while on most architectures
the same instruction would leave the upper bits unchanged.
Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue . Greg, would you mind running this
through the wringer?
I would not expect this to change anything for the bug that Greg
is chasing, unless there is also a bug in clang.
In the version before the patch, we get a 64-bit argument from
user space, which may consist of the intended value in the lower
bits plus garbage in the upper bits. However, vlen only gets
passed down into import_iovec() without any other operations
on it, and ince import_iovec takes a 32-bit argument, this is
where it finally gets narrowed.
After your patch, the SYSCALL_DEFINE3() does the narrowing
conversion with the same clearing of the upper bits.
If there is a problem somewhere leading up to import_iovec(),
it would have to in some code that expects to get a 32-bit
register argument but gets called with a register that has
garbage in the upper bits /without/ going through a correct
sanitizing function like SYSCALL_DEFINE3().
Arnd
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2020-10-22 19:05:19
On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann [off-list ref] wrote:
On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
[off-list ref] wrote:
quoted
On Thu, Oct 22, 2020 at 9:35 AM David Laight [off-list ref] wrote:
quoted
Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.
Why not x86-64? Wouldn't it be *any* LP64 ISA?
x86-64 is slightly special because most instructions on a 32-bit
argument clear the upper 32 bits, while on most architectures
the same instruction would leave the upper bits unchanged.
Oh interesting, depends on the operations too on x86_64 IIUC?
quoted
Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue . Greg, would you mind running this
through the wringer?
I would not expect this to change anything for the bug that Greg
is chasing, unless there is also a bug in clang.
In the version before the patch, we get a 64-bit argument from
user space, which may consist of the intended value in the lower
bits plus garbage in the upper bits. However, vlen only gets
passed down into import_iovec() without any other operations
on it, and since import_iovec takes a 32-bit argument, this is
where it finally gets narrowed.
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
After your patch, the SYSCALL_DEFINE3() does the narrowing
conversion with the same clearing of the upper bits.
If there is a problem somewhere leading up to import_iovec(),
it would have to in some code that expects to get a 32-bit
register argument but gets called with a register that has
garbage in the upper bits /without/ going through a correct
sanitizing function like SYSCALL_DEFINE3().
Arnd
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-22 19:25:16
On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
FWIW,
void f(unsinged long v)
{
if (v != 1)
printf("failed\n");
}
void g(unsigned int v)
{
f(v);
}
void h(unsigned long v)
{
g(v);
}
main()
{
h(0x100000001);
}
must not produce any output on a host with 32bit int and 64bit long, regardless of
the inlining, having functions live in different compilation units, etc.
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-22 19:27:38
On Thu, Oct 22, 2020 at 08:24:58PM +0100, Al Viro wrote:
On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
quoted
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-22 20:07:05
On Thu, Oct 22, 2020 at 08:24:58PM +0100, Al Viro wrote:
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
Unless I'm misreading AAPCS64,
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by the callee
rather than the caller"
in 6.4.2 means that callee must not _not_ expect the upper 32 bits of %x0..%x7 to contain
anything valid for 32bit arguments and it must zero-extend %w0..%w7 when passing that to
something that expects a 64bit argument. On inlining it should be the same situation as
storing unsigned int argument into unsigned long local variable and working with that - if
void f(unsigned int w)
{
unsigned long x = w;
printf("%lx\n", x);
}
ends up passing %x0 to printf, it's an obvious bug - it must do something like
uxtw x0, w0
first.
What am I missing here?
On Thu, Oct 22, 2020 at 9:05 PM Nick Desaulniers
[off-list ref] wrote:
On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann [off-list ref] wrote:
quoted
On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
[off-list ref] wrote:
quoted
On Thu, Oct 22, 2020 at 9:35 AM David Laight [off-list ref] wrote:
quoted
Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.
Why not x86-64? Wouldn't it be *any* LP64 ISA?
x86-64 is slightly special because most instructions on a 32-bit
argument clear the upper 32 bits, while on most architectures
the same instruction would leave the upper bits unchanged.
Oh interesting, depends on the operations too on x86_64 IIUC?
It seems this doesn't impact the calling conventions (see below),
it's just that there are more cases on x86 where the callee doesn't
have to explicitly clear the upper bits because the this is implied.
quoted
quoted
Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue . Greg, would you mind running this
through the wringer?
I would not expect this to change anything for the bug that Greg
is chasing, unless there is also a bug in clang.
In the version before the patch, we get a 64-bit argument from
user space, which may consist of the intended value in the lower
bits plus garbage in the upper bits. However, vlen only gets
passed down into import_iovec() without any other operations
on it, and since import_iovec takes a 32-bit argument, this is
where it finally gets narrowed.
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
Sorry I got it wrong, looked up the aarch64 AAPCS now, which
explains
"Unlike in the 32-bit AAPCS, named integral values must be
narrowed by the callee rather than the caller."
Also confirmed using https://godbolt.org/z/acPrjj, which
shows more combinations of compilers and architectures
in addition to your example. I had expected arm64 to be
like powerpc64 and arm32 in this case, but it's the reverse.
I also verified that SYSCALL_DEFINEx() is correct on arm64
and saw that as of v4.19 it passes the syscall arguments
through pt_regs, which will do the right thing here regardless
of the argument passing rules. The earlier version also seems
to be working as intended.
Arnd
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-22 20:09:32
On Thu, Oct 22, 2020 at 09:06:29PM +0100, Al Viro wrote:
On Thu, Oct 22, 2020 at 08:24:58PM +0100, Al Viro wrote:
quoted
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
Unless I'm misreading AAPCS64,
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by the callee
rather than the caller"
in 6.4.2 means that callee must not _not_ expect the upper 32 bits of %x0..%x7 to contain
Sorry, artefact of editing - that's
"in 6.4.2 means that callee must _not_ expect the upper 32 bits of %x0..%x7 to contain"
obviously.
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2020-10-22 20:11:38
On Thu, Oct 22, 2020 at 12:25 PM Al Viro [off-list ref] wrote:
On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
quoted
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
FWIW,
void f(unsinged long v)
{
if (v != 1)
printf("failed\n");
}
void g(unsigned int v)
{
f(v);
}
void h(unsigned long v)
{
g(v);
}
main()
{
h(0x100000001);
}
A good/analogous example, but things get weird when the leaf node in
the call chain is inline asm: https://godbolt.org/z/s19TY5
(I'm not sure that's precisely what's going on here; I'll need to dive
more into the calls rw_copy_check_uvector() makes to see if there's
inline asm somewhere, pretty sure calls to get_user with `nr_regs`
exist).
must not produce any output on a host with 32bit int and 64bit long, regardless of
the inlining, having functions live in different compilation units, etc.
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
But the syscall is defined as:
SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen)
{
return do_readv(fd, vec, vlen, 0);
}
FWIW, glibc makes the readv() syscall assuming that fd and vlen are 'int' as
well. So this problem isn't specific to Android's libc.
From objdump -d /lib/x86_64-linux-gnu/libc.so.6:
00000000000f4db0 <readv@@GLIBC_2.2.5>:
f4db0: 64 8b 04 25 18 00 00 mov %fs:0x18,%eax
f4db7: 00
f4db8: 85 c0 test %eax,%eax
f4dba: 75 14 jne f4dd0 <readv@@GLIBC_2.2.5+0x20>
f4dbc: b8 13 00 00 00 mov $0x13,%eax
f4dc1: 0f 05 syscall
...
There's some code for pthread cancellation, but no zeroing of the upper half of
the fd and vlen arguments, which are in %edi and %edx respectively. But the
glibc function prototype uses 'int' for them, not 'unsigned long'
'ssize_t readv(int fd, const struct iovec *iov, int iovcnt);'.
So the high halves of the fd and iovcnt registers can contain garbage. Or at
least that's what gcc (9.3.0) and clang (9.0.1) assume; they both compile the
following
void g(unsigned int x);
void f(unsigned long x)
{
g(x);
}
into f() making a tail call to g(), without zeroing the top half of %rdi.
Also note the following program succeeds on Linux 5.9 on x86_64. On kernels
that have this bug, it should fail. (I couldn't get it to actually fail, so it
must depend on the compiler and/or the kernel config...)
#include <fcntl.h>
#include <stdio.h>
#include <sys/syscall.h>
#include <sys/uio.h>
#include <unistd.h>
int main()
{
int fd = open("/dev/zero", O_RDONLY);
char buf[1000];
struct iovec iov = { .iov_base = buf, .iov_len = sizeof(buf) };
long ret;
ret = syscall(__NR_readv, fd, &iov, 0x100000001);
if (ret < 0)
perror("readv failed");
else
printf("read %ld bytes\n", ret);
}
I think the right fix is to change the readv() (and writev(), etc.) syscalls to
take 'unsigned int' rather than 'unsigned long', as that is what the users are
assuming...
- Eric
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-22 21:28:47
On Thu, Oct 22, 2020 at 01:59:32PM -0700, Eric Biggers wrote:
Also note the following program succeeds on Linux 5.9 on x86_64. On kernels
that have this bug, it should fail. (I couldn't get it to actually fail, so it
must depend on the compiler and/or the kernel config...)
It doesn't. See https://www.spinics.net/lists/linux-scsi/msg147836.html for
discussion of that mess.
ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
unsigned long vlen, loff_t *pos, rwf_t flags)
{
struct iovec iovstack[UIO_FASTIOV];
struct iovec *iov = iovstack;
struct iov_iter iter;
ssize_t ret;
ret = import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
if (ret >= 0) {
ret = do_iter_read(file, &iter, pos, flags);
kfree(iov);
}
return ret;
}
and import_iovec() takes unsigned int as the third argument, so it *will*
truncate to 32 bits, no matter what. Has done so since 0504c074b546
"switch {compat_,}do_readv_writev() to {compat_,}import_iovec()" back in
March 2015. Yes, it was an incompatible userland ABI change, even though
nothing that used glibc/uclibc/dietlibc would've noticed.
Better yet, up until 2.1.90pre1 passing a 64bit value as the _first_ argument
of readv(2) used to fail with -EBADF if it was too large; at that point it
started to get quietly truncated to 32bit first. And again, no libc users
would've noticed (neither would anything except deliberate regression test
looking for that specific behaviour).
Note that we also have process_madvise(2) with size_t for vlen (huh? It's
a number of array elements, not an object size) and process_vm_{read,write}v(2),
that have unsigned long for the same thing. And the last two *are* using
the same unsigned long from glibc POV.
From: David Laight <hidden> Date: 2020-10-22 22:04:11
From: Nick Desaulniers
Sent: 22 October 2020 20:05
...
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
Right but is the called function going to use 32bit ops
and/or mask the register?
Certainly that is likely on x86-64.
I've rather lost track of where the masking is expected
to happen.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-10-22 22:07:18
From: Al Viro
Sent: 22 October 2020 20:25
On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
quoted
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
FWIW,
void f(unsinged long v)
{
if (v != 1)
printf("failed\n");
}
void g(unsigned int v)
{
f(v);
}
void h(unsigned long v)
{
g(v);
}
main()
{
h(0x100000001);
}
must not produce any output on a host with 32bit int and 64bit long, regardless of
the inlining, having functions live in different compilation units, etc.
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
Put g() in a separate compilation unit and use the 'wrong' type
in the prototypes t() used to call g() and g() uses to call f().
Then you might see where and masking does (or does not) happen.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-10-23 12:46:48
From: Greg KH <gregkh@linuxfoundation.org>
Sent: 22 October 2020 14:51
I've rammed the code into godbolt.
https://godbolt.org/z/9v5PPW
Definitely a clang bug.
Search for [wx]24 in the clang output.
nr_segs comes in as w2 and the initial bound checks are done on w2.
w24 is loaded from w2 - I don't believe this changes the high bits.
There are no references to w24, just x24.
So the kmalloc_array() is passed 'huge' and will fail.
The iov_iter_init also gets the 64bit value.
Note that the gcc code has a sign-extend copy of w2.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Hildenbrand <hidden> Date: 2020-10-23 13:09:57
On 23.10.20 14:46, David Laight wrote:
From: Greg KH <gregkh@linuxfoundation.org>
quoted
Sent: 22 October 2020 14:51
I've rammed the code into godbolt.
https://godbolt.org/z/9v5PPW
Definitely a clang bug.
Search for [wx]24 in the clang output.
nr_segs comes in as w2 and the initial bound checks are done on w2.
w24 is loaded from w2 - I don't believe this changes the high bits.
There are no references to w24, just x24.
So the kmalloc_array() is passed 'huge' and will fail.
The iov_iter_init also gets the 64bit value.
Note that the gcc code has a sign-extend copy of w2.
Do we have a result from using "unsigned long" in the base function and
explicitly masking of the high bits? That should definitely work.
Now, I am not a compiler expert, but as I already cited, at least on
x86-64 clang expects that the high bits were cleared by the caller - in
contrast to gcc. I suspect it's the same on arm64, but again, I am no
compiler expert.
If what I said and cites for x86-64 is correct, if the function expects
an "unsigned int", it will happily use 64bit operations without further
checks where valid when assuming high bits are zero. That's why even
converting everything to "unsigned int" as proposed by me won't work on
clang - it assumes high bits are zero (as indicated by Nick).
As I am neither a compiler experts (did I mention that already? ;) ) nor
an arm64 experts, I can't tell if this is a compiler BUG or not.
Main issue seems to be garbage in high bits as originally suggested by me.
--
Thanks,
David / dhildenb
From: David Hildenbrand <hidden> Date: 2020-10-23 13:12:50
On 22.10.20 21:24, Al Viro wrote:
On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
quoted
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
FWIW,
void f(unsinged long v)
{
if (v != 1)
printf("failed\n");
}
void g(unsigned int v)
{
f(v);
}
void h(unsigned long v)
{
g(v);
}
main()
{
h(0x100000001);
}
must not produce any output on a host with 32bit int and 64bit long, regardless of
the inlining, having functions live in different compilation units, etc.
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
The interesting case is having g() in a separate compilation unit and
force-calling g() with 0x100000001 via inline ASM. So forcing garbage
into high bits.
I'll paly with it.
--
Thanks,
David / dhildenb
On Fri, Oct 23, 2020 at 2:46 PM David Laight [off-list ref] wrote:
From: Greg KH <gregkh@linuxfoundation.org>
quoted
Sent: 22 October 2020 14:51
I've rammed the code into godbolt.
https://godbolt.org/z/9v5PPW
Definitely a clang bug.
Search for [wx]24 in the clang output.
nr_segs comes in as w2 and the initial bound checks are done on w2.
w24 is loaded from w2 - I don't believe this changes the high bits.
You believe wrong, "mov w24, w2" is a zero-extending operation.
Arnd
From: David Laight <hidden> Date: 2020-10-23 13:29:09
From: Arnd Bergmann
Sent: 23 October 2020 14:23
On Fri, Oct 23, 2020 at 2:46 PM David Laight [off-list ref] wrote:
quoted
From: Greg KH <gregkh@linuxfoundation.org>
quoted
Sent: 22 October 2020 14:51
I've rammed the code into godbolt.
https://godbolt.org/z/9v5PPW
Definitely a clang bug.
Search for [wx]24 in the clang output.
nr_segs comes in as w2 and the initial bound checks are done on w2.
w24 is loaded from w2 - I don't believe this changes the high bits.
You believe wrong, "mov w24, w2" is a zero-extending operation.
Ah ok, but gcc uses utxw for the same task.
I guess they could be the same opcode.
Last time I wrote ARM thumb didn't really exist - never mind 64bit
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Hildenbrand <hidden> Date: 2020-10-23 14:33:27
On 23.10.20 15:09, David Hildenbrand wrote:
On 23.10.20 14:46, David Laight wrote:
quoted
From: Greg KH <gregkh@linuxfoundation.org>
quoted
Sent: 22 October 2020 14:51
I've rammed the code into godbolt.
https://godbolt.org/z/9v5PPW
Definitely a clang bug.
Search for [wx]24 in the clang output.
nr_segs comes in as w2 and the initial bound checks are done on w2.
w24 is loaded from w2 - I don't believe this changes the high bits.
There are no references to w24, just x24.
So the kmalloc_array() is passed 'huge' and will fail.
The iov_iter_init also gets the 64bit value.
Note that the gcc code has a sign-extend copy of w2.
Do we have a result from using "unsigned long" in the base function and
explicitly masking of the high bits? That should definitely work.
Now, I am not a compiler expert, but as I already cited, at least on
x86-64 clang expects that the high bits were cleared by the caller - in
contrast to gcc. I suspect it's the same on arm64, but again, I am no
compiler expert.
If what I said and cites for x86-64 is correct, if the function expects
an "unsigned int", it will happily use 64bit operations without further
checks where valid when assuming high bits are zero. That's why even
converting everything to "unsigned int" as proposed by me won't work on
clang - it assumes high bits are zero (as indicated by Nick).
As I am neither a compiler experts (did I mention that already? ;) ) nor
an arm64 experts, I can't tell if this is a compiler BUG or not.
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
--
Thanks,
David / dhildenb
From: David Laight <hidden> Date: 2020-10-23 14:39:37
From: David Hildenbrand
Sent: 23 October 2020 15:33
...
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
From: David Hildenbrand
quoted
Sent: 23 October 2020 15:33
...
quoted
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
I've worked around the merge at the moment in the android tree, but it
is still quite reproducable, and will try to get a .o file to
disassemble on Monday or so...
thanks,
greg k-h
From: David Hildenbrand <hidden> Date: 2020-10-23 16:33:22
On 23.10.20 16:47, 'Greg KH' wrote:
On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 23 October 2020 15:33
...
quoted
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
I've worked around the merge at the moment in the android tree, but it
is still quite reproducable, and will try to get a .o file to
disassemble on Monday or so...
I just compiled pre and post fb041b598997d63c0f7d7305dfae70046bf66fe1 with
clang version 11.0.0 (Fedora 11.0.0-0.2.rc1.fc33)
for aarch64 with defconfig and extracted import_iovec and
rw_copy_check_uvector (skipping the compat things)
Pre fb041b598997d63c0f7d7305dfae70046bf66fe1 import_iovec
-> https://pastebin.com/LtnYMLJt
Post fb041b598997d63c0f7d7305dfae70046bf66fe1 import_iovec
-> https://pastebin.com/BWPmXrAf
Pre fb041b598997d63c0f7d7305dfae70046bf66fe1 rw_copy_check_uvector
-> https://pastebin.com/4nSBYRbf
Post fb041b598997d63c0f7d7305dfae70046bf66fe1 rw_copy_check_uvector
-> https://pastebin.com/hPtEgaEW
I'm only able to spot minor differences ... less gets inlined than I
would have expected. But there are some smaller differences.
Maybe someone wants to have a look before we have object files as used
by Greg ...
--
Thanks,
David / dhildenb
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2020-10-23 17:59:16
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
Now, I am not a compiler expert, but as I already cited, at least on
x86-64 clang expects that the high bits were cleared by the caller - in
contrast to gcc. I suspect it's the same on arm64, but again, I am no
compiler expert.
If what I said and cites for x86-64 is correct, if the function expects
an "unsigned int", it will happily use 64bit operations without further
checks where valid when assuming high bits are zero. That's why even
converting everything to "unsigned int" as proposed by me won't work on
clang - it assumes high bits are zero (as indicated by Nick).
As I am neither a compiler experts (did I mention that already? ;) ) nor
an arm64 experts, I can't tell if this is a compiler BUG or not.
On arm64 when callee expects a 32bit argument, the caller is *not* responsible
for clearing the upper half of 64bit register used to pass the value - it only
needs to store the actual value into the lower half. The callee must consider
the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
); AFAICS, the relevant bit is
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by
the callee rather than the caller."
On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote:
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
quoted
Now, I am not a compiler expert, but as I already cited, at least on
x86-64 clang expects that the high bits were cleared by the caller - in
contrast to gcc. I suspect it's the same on arm64, but again, I am no
compiler expert.
If what I said and cites for x86-64 is correct, if the function expects
an "unsigned int", it will happily use 64bit operations without further
checks where valid when assuming high bits are zero. That's why even
converting everything to "unsigned int" as proposed by me won't work on
clang - it assumes high bits are zero (as indicated by Nick).
As I am neither a compiler experts (did I mention that already? ;) ) nor
an arm64 experts, I can't tell if this is a compiler BUG or not.
On arm64 when callee expects a 32bit argument, the caller is *not* responsible
for clearing the upper half of 64bit register used to pass the value - it only
needs to store the actual value into the lower half. The callee must consider
the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
); AFAICS, the relevant bit is
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by
the callee rather than the caller."
Or the formal rule:
C.9 If the argument is an Integral or Pointer Type, the size of the
argument is less than or equal to 8 bytes and the NGRN is less
than 8, the argument is copied to the least significant bits in
x[NGRN]. The NGRN is incremented by one. The argument has now
been allocated.
Segher
From: David Laight <hidden> Date: 2020-10-23 21:29:13
From: Segher Boessenkool
Sent: 23 October 2020 19:27
On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote:
quoted
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
quoted
Now, I am not a compiler expert, but as I already cited, at least on
x86-64 clang expects that the high bits were cleared by the caller - in
contrast to gcc. I suspect it's the same on arm64, but again, I am no
compiler expert.
If what I said and cites for x86-64 is correct, if the function expects
an "unsigned int", it will happily use 64bit operations without further
checks where valid when assuming high bits are zero. That's why even
converting everything to "unsigned int" as proposed by me won't work on
clang - it assumes high bits are zero (as indicated by Nick).
As I am neither a compiler experts (did I mention that already? ;) ) nor
an arm64 experts, I can't tell if this is a compiler BUG or not.
On arm64 when callee expects a 32bit argument, the caller is *not* responsible
for clearing the upper half of 64bit register used to pass the value - it only
needs to store the actual value into the lower half. The callee must consider
the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
); AFAICS, the relevant bit is
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by
the callee rather than the caller."
Or the formal rule:
C.9 If the argument is an Integral or Pointer Type, the size of the
argument is less than or equal to 8 bytes and the NGRN is less
than 8, the argument is copied to the least significant bits in
x[NGRN]. The NGRN is incremented by one. The argument has now
been allocated.
So, in essence, if the value is in a 64bit register the calling
code is independent of the actual type of the formal parameter.
Clearly a value might need explicit widening.
I've found a copy of the 64 bit arm instruction set.
Unfortunately it is alpha sorted and repetitive so shows none
of the symmetry and makes things difficult to find.
But, contrary to what someone suggested most register writes
(eg from arithmetic) seem to zero/extend the high bits.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Oct 23, 2020 at 09:28:59PM +0000, David Laight wrote:
From: Segher Boessenkool
quoted
Sent: 23 October 2020 19:27
On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote:
quoted
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
On arm64 when callee expects a 32bit argument, the caller is *not* responsible
for clearing the upper half of 64bit register used to pass the value - it only
needs to store the actual value into the lower half. The callee must consider
the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
); AFAICS, the relevant bit is
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by
the callee rather than the caller."
Or the formal rule:
C.9 If the argument is an Integral or Pointer Type, the size of the
argument is less than or equal to 8 bytes and the NGRN is less
than 8, the argument is copied to the least significant bits in
x[NGRN]. The NGRN is incremented by one. The argument has now
been allocated.
So, in essence, if the value is in a 64bit register the calling
code is independent of the actual type of the formal parameter.
Clearly a value might need explicit widening.
No, this says that if you pass a 32-bit integer in a 64-bit register,
then the top 32 bits of that register hold an undefined value.
I've found a copy of the 64 bit arm instruction set.
Unfortunately it is alpha sorted and repetitive so shows none
of the symmetry and makes things difficult to find.
All of this is ABI, not ISA. Look at the AAPCS64 pointed to above.
But, contrary to what someone suggested most register writes
(eg from arithmetic) seem to zero/extend the high bits.
Everything that writes a "w" does, yes. But that has nothing to do with
the parameter passing rules, that is ABI. It just means that very often
a 32-bit integer will be passed zero-extended in a 64-bit register, but
that is just luck (or not, it makes finding bugs harder ;-) )
Segher
From: David Laight <hidden> Date: 2020-10-24 21:12:24
From: Segher Boessenkool
Sent: 24 October 2020 18:29
On Fri, Oct 23, 2020 at 09:28:59PM +0000, David Laight wrote:
quoted
From: Segher Boessenkool
quoted
Sent: 23 October 2020 19:27
On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote:
quoted
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
On arm64 when callee expects a 32bit argument, the caller is *not* responsible
for clearing the upper half of 64bit register used to pass the value - it only
needs to store the actual value into the lower half. The callee must consider
the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
); AFAICS, the relevant bit is
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by
the callee rather than the caller."
Or the formal rule:
C.9 If the argument is an Integral or Pointer Type, the size of the
argument is less than or equal to 8 bytes and the NGRN is less
than 8, the argument is copied to the least significant bits in
x[NGRN]. The NGRN is incremented by one. The argument has now
been allocated.
So, in essence, if the value is in a 64bit register the calling
code is independent of the actual type of the formal parameter.
Clearly a value might need explicit widening.
No, this says that if you pass a 32-bit integer in a 64-bit register,
then the top 32 bits of that register hold an undefined value.
That's sort of what I meant.
The 'normal' junk in the hight bits will there because the variable
in the calling code is wider.
quoted
I've found a copy of the 64 bit arm instruction set.
Unfortunately it is alpha sorted and repetitive so shows none
of the symmetry and makes things difficult to find.
All of this is ABI, not ISA. Look at the AAPCS64 pointed to above.
quoted
But, contrary to what someone suggested most register writes
(eg from arithmetic) seem to zero/extend the high bits.
Everything that writes a "w" does, yes. But that has nothing to do with
the parameter passing rules, that is ABI. It just means that very often
a 32-bit integer will be passed zero-extended in a 64-bit register, but
that is just luck (or not, it makes finding bugs harder ;-) )
Working out why the code is wrong is more of an ISA issue than an ABI one.
It may be an ABI one, but the analysis is ISA.
I've written a lot of asm over the years - decoding compiler generated
asm isn't that hard.
At least ARM doesn't have annulled delay slots.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2020-11-02 09:08:10
From: 'Greg KH'
Sent: 23 October 2020 15:47
On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 23 October 2020 15:33
...
quoted
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
I've worked around the merge at the moment in the android tree, but it
is still quite reproducable, and will try to get a .o file to
disassemble on Monday or so...
Did this get properly resolved?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Nov 02, 2020 at 09:06:38AM +0000, David Laight wrote:
From: 'Greg KH'
quoted
Sent: 23 October 2020 15:47
On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 23 October 2020 15:33
...
quoted
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
I've worked around the merge at the moment in the android tree, but it
is still quite reproducable, and will try to get a .o file to
disassemble on Monday or so...
Did this get properly resolved?
For some reason, 5.10-rc2 fixed all of this up. I backed out all of the
patches I had to revert to get 5.10-rc1 to work properly, and then did
the merge and all is well.
It must have been something to do with the compat changes in this same
area that went in after 5.10-rc1, and something got reorganized in the
files somehow. I really do not know, and at the moment, don't have the
time to track it down anymore. So for now, I'd say it's all good, sorry
for the noise.
greg k-h
From: David Laight <hidden> Date: 2020-11-02 18:23:28
From: 'Greg KH'
Sent: 02 November 2020 13:52
On Mon, Nov 02, 2020 at 09:06:38AM +0000, David Laight wrote:
quoted
From: 'Greg KH'
quoted
Sent: 23 October 2020 15:47
On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
quoted
From: David Hildenbrand
quoted
Sent: 23 October 2020 15:33
...
quoted
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
I've worked around the merge at the moment in the android tree, but it
is still quite reproducable, and will try to get a .o file to
disassemble on Monday or so...
Did this get properly resolved?
For some reason, 5.10-rc2 fixed all of this up. I backed out all of the
patches I had to revert to get 5.10-rc1 to work properly, and then did
the merge and all is well.
It must have been something to do with the compat changes in this same
area that went in after 5.10-rc1, and something got reorganized in the
files somehow. I really do not know, and at the moment, don't have the
time to track it down anymore. So for now, I'd say it's all good, sorry
for the noise.
Hopefully it won't appear again.
Saved me spending a day off reading arm64 assembler.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)