Date: Sat, 11 Mar 2017 22:01:48 -0800
From: Eric Biggers <ebiggers3@gmail.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, David Howells <dhowells@redhat.com>,
        linux-kernel@vger.kernel.org, Eric Biggers <ebiggers@google.com>
Subject: Re: [PATCH v2] statx: optimize copy of struct statx to userspace
Message-ID: <20170312060148.GA1595@zzz>
References: <20170311214555.941-1-ebiggers3@gmail.com>
 <20170312012411.GN29622@ZenIV.linux.org.uk>
 <20170312021655.GA593@zzz>
 <20170312022923.GQ29622@ZenIV.linux.org.uk>
 <20170312040206.GA3684@zzz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170312040206.GA3684@zzz>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1807
Lines: 34

On Sat, Mar 11, 2017 at 08:02:06PM -0800, Eric Biggers wrote:
> On Sun, Mar 12, 2017 at 02:29:27AM +0000, Al Viro wrote:
> > 
> > Oh, I agree that multiple __put_user() are wrong; I also agree that bulk copy is
> > the right approach (when we get the unsafe stuff right, we can revisit that, but
> > I suspect that on quite a few architectures a bulk copy will still give better
> > time, no matter what).
> > 
> > > If padding is a concern at all (AFAICS it's not actually an issue now with
> > > struct statx, but people tend to have different opinions on how careful they
> > > want to be with padding), then I think we'll just have to start by memsetting
> > > the whole struct to 0.
> > 
> > My point is simply that it's worth a comment in that code.
> 
> Okay, thanks.  I'll add a comment about the padding assumption, and I think I'll
> take the suggestion to use a designated initializer.  Then at least all *fields*
> get initialized by default.  And if in the future someone wants to conditionally
> initialize fields, then they can use ?: or they can do it after the initializer.
> Either way, at least they won't be able to forget to zero some field.
> 
> - Eric

Okay, well, I may have changed my mind again...  I tried the designated
initializer on x86_64 with gcc 4.8 and 6.3, and also on arm64 with gcc 4.8.  In
each case, it was compiled into first zeroing all 256 bytes of the struct, just
like memset(&tmp, 0, sizeof(tmp)).  Yes, this was with
CC_OPTIMIZE_FOR_PERFORMANCE=y.  So I think we might as well just write the full
memset(), making it completely clear that everything is initialized.  (This is
especially useful for people who are auditing code paths like this for
information leaks.)  Also, a smart compiler could potentially optimize away
parts of the memset() anyway...

Eric