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... thanks, greg k-h
