lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Oct 2020 09:28:13 +0000 From: David Laight <David.Laight@...LAB.COM> To: 'David Hildenbrand' <david@...hat.com>, Greg KH <gregkh@...uxfoundation.org> CC: Al Viro <viro@...iv.linux.org.uk>, Nick Desaulniers <ndesaulniers@...gle.com>, Christoph Hellwig <hch@....de>, "kernel-team@...roid.com" <kernel-team@...roid.com>, Andrew Morton <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>, Arnd Bergmann <arnd@...db.de>, David Howells <dhowells@...hat.com>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>, "linux-parisc@...r.kernel.org" <linux-parisc@...r.kernel.org>, "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>, "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>, "sparclinux@...r.kernel.org" <sparclinux@...r.kernel.org>, "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>, "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>, "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, "linux-aio@...ck.org" <linux-aio@...ck.org>, "io-uring@...r.kernel.org" <io-uring@...r.kernel.org>, "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "keyrings@...r.kernel.org" <keyrings@...r.kernel.org>, "linux-security-module@...r.kernel.org" <linux-security-module@...r.kernel.org> Subject: RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c" From: David Hildenbrand > Sent: 22 October 2020 10:19 > > On 22.10.20 11:01, Greg KH wrote: > > On Thu, Oct 22, 2020 at 10:48:59AM +0200, David Hildenbrand wrote: > >> On 22.10.20 10:40, David Laight wrote: > >>> From: David Hildenbrand > >>>> Sent: 22 October 2020 09:35 > >>>> > >>>> On 22.10.20 10:26, Greg KH wrote: > >>>>> 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: > >>>>>>> On Fri, Sep 25, 2020 at 06:51:39AM +0200, Christoph Hellwig wrote: > >>>>>>>> From: David Laight <David.Laight@...LAB.COM> > >>>>>>>> > >>>>>>>> This lets the compiler inline it into import_iovec() generating > >>>>>>>> much better code. > >>>>>>>> > >>>>>>>> Signed-off-by: David Laight <david.laight@...lab.com> > >>>>>>>> Signed-off-by: Christoph Hellwig <hch@....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)
Powered by blists - more mailing lists