From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751177AbdGOOkt (ORCPT ); Sat, 15 Jul 2017 10:40:49 -0400 Received: from casper.infradead.org ([85.118.1.10]:33652 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750985AbdGOOkr (ORCPT ); Sat, 15 Jul 2017 10:40:47 -0400 Date: Sat, 15 Jul 2017 15:40:29 +0100 (BST) From: James Simmons To: Al Viro cc: Arnd Bergmann , Oleg Drokin , Andreas Dilger , "# 3.4.x" , Greg Kroah-Hartman , Doug Oucharek , Dmitry Eremin , Liang Zhen , Nicholas Hanley , Lustre Development List , devel@driverdev.osuosl.org, Linux Kernel Mailing List Subject: Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code In-Reply-To: <20170714015034.GE13102@ZenIV.linux.org.uk> Message-ID: References: <20170710130833.1834210-1-arnd@arndb.de> <20170714015034.GE13102@ZenIV.linux.org.uk> User-Agent: Alpine 2.20 (LFD 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170715_154029_343025_A80310DE X-CRM114-Status: GOOD ( 26.62 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.4.1 on casper.infradead.org summary: Content analysis details: (-1.9 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 NO_RELAYS Informational: message was not relayed via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 14 Jul 2017, Al Viro wrote: > On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote: > > > Thanks for testing it! > > > > That means we did not copy any data and the kernel continues with > > an uninitialized buffer, right? The problem may be the definition of > > > > struct kib_immediate_msg { > > struct lnet_hdr ibim_hdr; /* portals header */ > > char ibim_payload[0]; /* piggy-backed payload */ > > } WIRE_ATTR; > > > > The check that Al added will try to ensure that we don't write > > beyond the size of the ibim_payload[] array, which unfortunately > > is defined as a zero-byte array, so I can see why it will now > > fail. However, it's already broken in mainline now, with or without > > my patch. > > > > Are you able to come up with a fix that avoids the warning in > > 'allmodconfig' and makes the function do something reasonable > > again? Yes, I'm testing a fix right now which I will merge with the original patch. Greg this patch will need to be sent to Linus as well so the kernel release isn't broken for users. > Might make sense to try and use valid C99 for "array of indefinite > size as the last member", i.e. > struct kib_immediate_msg { > struct lnet_hdr ibim_hdr; /* portals header */ > char ibim_payload[]; /* piggy-backed payload */ > } WIRE_ATTR; > > Zero-sized array as the last member is gcc hack predating that; > looks like gcc gets confused into deciding that it knows the distance > from the end of object... I did some profiling and found gcc was doing the right thing. That should be updated to a C99 flexable array in a latter patch. > Said that, are we really guaranteed the IBLND_MSG_SIZE bytes > in there? This is what the real bug was. In the current code we are telling copy_from_iter and copy_to_iter that the number of bytes are always IBLND_MSG_SIZE. Arnd thought this was always the size so in his patch he was testing the returned result of copy_[from|to]_iter to IBLND_MSG_SIZE. This nearly always failed since variable sized messages are being created. The zero size I initially saw was from doing pings. When I later tested with pushing I/O packets of other sizes were observed but none of them were IBLND_MSG_SIZE in size so they failed to transmit. As soon as I'm done testing I will send a patch.