Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750829AbdCMEe4 (ORCPT ); Mon, 13 Mar 2017 00:34:56 -0400 Received: from mail-io0-f193.google.com ([209.85.223.193]:36757 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbdCMEeo (ORCPT ); Mon, 13 Mar 2017 00:34:44 -0400 From: Andreas Dilger Message-Id: Content-Type: multipart/signed; boundary="Apple-Mail=_846CAF68-2C68-4A9C-805B-07A8E1128325"; protocol="application/pgp-signature"; micalg=pgp-sha1 Mime-Version: 1.0 (Mac OS X Mail 10.2 \(3259\)) Subject: Re: [PATCH v2] statx: optimize copy of struct statx to userspace Date: Sun, 12 Mar 2017 22:34:38 -0600 In-Reply-To: <20170312060148.GA1595@zzz> Cc: Al Viro , linux-fsdevel , David Howells , linux-kernel@vger.kernel.org, Eric Biggers To: Eric Biggers 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> <20170312060148.GA1595@zzz> X-Mailer: Apple Mail (2.3259) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4377 Lines: 131 --Apple-Mail=_846CAF68-2C68-4A9C-805B-07A8E1128325 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii On Mar 11, 2017, at 11:01 PM, Eric Biggers wrote: >=20 > 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: >>>=20 >>> 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). >>>=20 >>>> 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. >>>=20 >>> My point is simply that it's worth a comment in that code. >>=20 >> 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. >=20 > 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=3Dy. 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... Not that it is a huge deal either way, but I'd think it is harder for = the compiler to optimize across a function call boundary like memset() vs. a struct initialization in the same function where it can see that all but a few of the fields are being overwritten immediately before they are = used. I don't think the designated initializer is any less clear to the reader that the struct is zeroed out compared to using memset(). Possibly the best compromise is to use a designated initializer that specifies all of the known fields, and leaves it to the compiler to initialize unset = fields or padding. That avoids double zeroing without any risk of exposing = unset fields to userspace: static int cp_statx(const struct kstat *stat, struct statx __user = *buffer) { struct statx tmp =3D { .stx_mask =3D stat->result_mask; .stx_blksize =3D stat->blksize; .stx_attributes =3D stat->attributes; .stx_nlink =3D stat->nlink; .stx_uid =3D from_kuid_munged(current_user_ns(), = stat->uid); .stx_gid =3D from_kgid_munged(current_user_ns(), = stat->gid); .stx_mode =3D stat->mode; .stx_ino =3D stat->ino; .stx_size =3D stat->size; .stx_blocks =3D stat->blocks; .stx_atime.tv_sec =3D stat->atime.tv_sec; .stx_atime.tv_nsec =3D stat->atime.tv_nsec; .stx_btime.tv_sec =3D stat->btime.tv_sec; .stx_btime.tv_nsec =3D stat->btime.tv_nsec; .stx_ctime.tv_sec =3D stat->ctime.tv_sec; .stx_ctime.tv_nsec =3D stat->ctime.tv_nsec; .stx_mtime.tv_sec =3D stat->mtime.tv_sec; .stx_mtime.tv_nsec =3D stat->mtime.tv_nsec; .stx_rdev_major =3D MAJOR(stat->rdev); .stx_rdev_minor =3D MINOR(stat->rdev); .stx_dev_major =3D MAJOR(stat->dev); .stx_dev_minor =3D MINOR(stat->dev); }; return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0; } Cheers, Andreas --Apple-Mail=_846CAF68-2C68-4A9C-805B-07A8E1128325 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- Comment: GPGTools - http://gpgtools.org iD8DBQFYxiFfpIg59Q01vtYRAkYWAKDi+hFwZkMVPv6avU/PP930Z8kvdACfYj9w CNH/g/PVHGIZ2e1zOhsBQQo= =fAQ5 -----END PGP SIGNATURE----- --Apple-Mail=_846CAF68-2C68-4A9C-805B-07A8E1128325--