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: > > On 22.10.20 14:18, Greg KH wrote: > > > 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: > > >>> On 22.10.20 11:32, David Laight wrote: > > >>>> 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. > > >>> > > >>> 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): > > > > > > diff --git a/include/linux/uio.h b/include/linux/uio.h > > index 72d88566694e..7527298c6b56 100644 > > --- a/include/linux/uio.h > > +++ b/include/linux/uio.h > > @@ -267,7 +267,7 @@ size_t hash_and_copy_to_iter(const void *addr, > > size_t bytes, void *hashp, > > struct iov_iter *i); > > > > struct iovec *iovec_from_user(const struct iovec __user *uvector, > > - unsigned long nr_segs, unsigned long fast_segs, > > + unsigned nr_segs, unsigned fast_segs, > > struct iovec *fast_iov, bool compat); > > ssize_t import_iovec(int type, const struct iovec __user *uvec, > > unsigned nr_segs, unsigned fast_segs, struct iovec **iovp, > > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > > index 1635111c5bd2..58417f1916dc 100644 > > --- a/lib/iov_iter.c > > +++ b/lib/iov_iter.c > > @@ -1652,7 +1652,7 @@ const void *dup_iter(struct iov_iter *new, struct > > iov_iter *old, gfp_t flags) > > EXPORT_SYMBOL(dup_iter); > > > > static int copy_compat_iovec_from_user(struct iovec *iov, > > - const struct iovec __user *uvec, unsigned long nr_segs) > > + const struct iovec __user *uvec, unsigned nr_segs) > > { > > const struct compat_iovec __user *uiov = > > (const struct compat_iovec __user *)uvec; > > @@ -1684,7 +1684,7 @@ static int copy_compat_iovec_from_user(struct > > iovec *iov, > > } > > > > static int copy_iovec_from_user(struct iovec *iov, > > - const struct iovec __user *uvec, unsigned long nr_segs) > > + const struct iovec __user *uvec, unsigned nr_segs) > > { > > unsigned long seg; > > > > @@ -1699,7 +1699,7 @@ static int copy_iovec_from_user(struct iovec *iov, > > } > > > > struct iovec *iovec_from_user(const struct iovec __user *uvec, > > - unsigned long nr_segs, unsigned long fast_segs, > > + unsigned nr_segs, unsigned fast_segs, > > struct iovec *fast_iov, bool compat) > > { > > struct iovec *iov = fast_iov; > > @@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct > > iovec __user *uvec, > > struct iov_iter *i, bool compat) > > { > > ssize_t total_len = 0; > > - unsigned long seg; > > + unsigned seg; > > struct iovec *iov; > > > > iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat); > > > > 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? thanks, greg k-h
