From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boqun Feng Subject: Re: [PATCH v2] vfs: avoid recopying file names in getname_flags Date: Tue, 7 Apr 2015 16:38:26 +0800 Message-ID: <20150407083826.GA1048@fixme-laptop.cn.ibm.com> References: <1427309152-25129-1-git-send-email-boqun.feng@gmail.com> <20150329042744.GA1477@fixme-laptop> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gKMricLos+KVdGMg" Cc: linux-kernel@vger.kernel.org, Al Viro , Paul Moore To: linux-fsdevel@vger.kernel.org Return-path: Content-Disposition: inline In-Reply-To: <20150329042744.GA1477@fixme-laptop> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org --gKMricLos+KVdGMg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Ping again... Thanks, Boqun Feng On Sun, Mar 29, 2015 at 12:27:44PM +0800, Boqun Feng wrote: > Ping. >=20 > This patch has been tested by 0day test bot. >=20 > Thanks, > Boqun Feng >=20 >=20 > On Thu, Mar 26, 2015 at 02:45:52AM +0800, Boqun Feng wrote: > > In the current implementation of getname_flags, a file name in the > > user-space will be recopied if it takes more space that > > EMBEDDED_NAME_MAX, however, at this moment, EMBEDDED_NAME_MAX bytes of > > the file name are already copied into kernel address space, the only > > reason why the recopy is needed is that "kname", which points to the > > string of the file name, needs to be relocated. > >=20 > > And the recopy can be avoided if we change the memory layout of the > > `names_cachep` allocation. By putting `struct filename` at the tail of > > the allocation instead of the head, relocation of kname is avoided. > > Once putting the struct at the tail, each byte in the user space will be > > copied only one time, so the recopy is avoided. > >=20 > > Of course, other functions aware of the layout of the `names_cachep` > > allocation, i.e., getname_kernel and putname also need to be modified to > > adapt to the new layout. > >=20 > > Since we change the layout of `names_cachep` allocation, in order to > > figure out whether kname and the struct are separate, we now need to > > check whether the file name string starts at the address > > EMBEDDED_NAME_MAX bytes before the address of the struct. As a result, > > `iname`, which points the end of `struct filename`, is not needed > > anymore. > >=20 > > Signed-off-by: Boqun Feng > > --- > > v1 --> v2: > > Rebase the patch onto the for-next branch of Al's vfs repo. > >=20 > >=20 > > fs/namei.c | 45 ++++++++++++++++++++++++++++----------------- > > include/linux/fs.h | 1 - > > 2 files changed, 28 insertions(+), 18 deletions(-) > >=20 > > diff --git a/fs/namei.c b/fs/namei.c > > index 7a11ec1..6d04029 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -119,7 +119,7 @@ > > * PATH_MAX includes the nul terminator --RR. > > */ > > =20 > > -#define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct filename, iname)) > > +#define EMBEDDED_NAME_MAX (PATH_MAX - sizeof(struct filename)) > > =20 > > struct filename * > > getname_flags(const char __user *filename, int flags, int *empty) > > @@ -132,44 +132,53 @@ getname_flags(const char __user *filename, int fl= ags, int *empty) > > if (result) > > return result; > > =20 > > - result =3D __getname(); > > - if (unlikely(!result)) > > + kname =3D __getname(); > > + if (unlikely(!kname)) > > return ERR_PTR(-ENOMEM); > > =20 > > /* > > * First, try to embed the struct filename inside the names_cache > > * allocation > > */ > > - kname =3D (char *)result->iname; > > + result =3D (struct filename *)(kname + EMBEDDED_NAME_MAX); > > result->name =3D kname; > > =20 > > len =3D strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX); > > if (unlikely(len < 0)) { > > - __putname(result); > > + __putname(kname); > > return ERR_PTR(len); > > } > > =20 > > /* > > * Uh-oh. We have a name that's approaching PATH_MAX. Allocate a > > * separate struct filename so we can dedicate the entire > > - * names_cache allocation for the pathname, and re-do the copy from > > + * names_cache allocation for the pathname, and continue the copy from > > * userland. > > */ > > if (unlikely(len =3D=3D EMBEDDED_NAME_MAX)) { > > - kname =3D (char *)result; > > - > > result =3D kzalloc(sizeof(*result), GFP_KERNEL); > > if (unlikely(!result)) { > > __putname(kname); > > return ERR_PTR(-ENOMEM); > > } > > result->name =3D kname; > > - len =3D strncpy_from_user(kname, filename, PATH_MAX); > > + /* we can't simply add the number of rest chars we copy to len, > > + * since strncpy_from_user may return negative to indicate > > + * something is wrong, so we do the addition later, after > > + * strncpy_from_user succeeds, and we know we already copy > > + * EMBEDDED_NAME_MAX chars. > > + */ > > + len =3D strncpy_from_user(kname + EMBEDDED_NAME_MAX, > > + filename + EMBEDDED_NAME_MAX, > > + PATH_MAX - EMBEDDED_NAME_MAX); > > + > > if (unlikely(len < 0)) { > > __putname(kname); > > kfree(result); > > return ERR_PTR(len); > > } > > + > > + len +=3D EMBEDDED_NAME_MAX; > > if (unlikely(len =3D=3D PATH_MAX)) { > > __putname(kname); > > kfree(result); > > @@ -204,26 +213,28 @@ struct filename * > > getname_kernel(const char * filename) > > { > > struct filename *result; > > + char *kname; > > int len =3D strlen(filename) + 1; > > =20 > > - result =3D __getname(); > > - if (unlikely(!result)) > > + kname =3D __getname(); > > + if (unlikely(!kname)) > > return ERR_PTR(-ENOMEM); > > =20 > > if (len <=3D EMBEDDED_NAME_MAX) { > > - result->name =3D (char *)result->iname; > > + result =3D (struct filename *)(kname + EMBEDDED_NAME_MAX); > > + result->name =3D kname; > > } else if (len <=3D PATH_MAX) { > > struct filename *tmp; > > =20 > > tmp =3D kmalloc(sizeof(*tmp), GFP_KERNEL); > > if (unlikely(!tmp)) { > > - __putname(result); > > + __putname(kname); > > return ERR_PTR(-ENOMEM); > > } > > - tmp->name =3D (char *)result; > > + tmp->name =3D kname; > > result =3D tmp; > > } else { > > - __putname(result); > > + __putname(kname); > > return ERR_PTR(-ENAMETOOLONG); > > } > > memcpy((char *)result->name, filename, len); > > @@ -242,11 +253,11 @@ void putname(struct filename *name) > > if (--name->refcnt > 0) > > return; > > =20 > > - if (name->name !=3D name->iname) { > > + if (name->name !=3D ((char *)name - EMBEDDED_NAME_MAX)) { > > __putname(name->name); > > kfree(name); > > } else > > - __putname(name); > > + __putname(name->name); > > } > > =20 > > static int check_acl(struct inode *inode, int mask) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index dfbd88a..dd67284 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -2166,7 +2166,6 @@ struct filename { > > const __user char *uptr; /* original userland pointer */ > > struct audit_names *aname; > > int refcnt; > > - const char iname[]; > > }; > > =20 > > extern long vfs_truncate(struct path *, loff_t); > > --=20 > > 2.3.3 > >=20 --gKMricLos+KVdGMg Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJVI5d7AAoJEEl56MO1B/q4uvQH/08k+CGa6Zs4mcNBZEyA4cSe hIegOQTCq0rK4eBiuVQabKyalWcYSoGFRAz7uzU7qia5ZLxiF234SBy/CxxU3m/b DO/3TtqIvOVZU4AUUeoMDziubu96jCQoS7yry7u89TKsOoyCn1bO6fIG1omlG/nj Skv7nj61Iq+Ycm03IIbmLx1i3U5nd6OT1fFVN1PNrNdA0pfSyp1Oq/g62jmCXmrV OC5u3SrWzVZoUyCT6+/IEi5WNK7CjZYPaC+fqZ4qKhJFRbQoWS1zKZGz8hNqoh0I 1iO3QKMZOgB2jhbe0Gb8sVSw6uLLz+9lqCJviS663wMO+He2VrAJQrXQ1RLs9LQ= =j5nb -----END PGP SIGNATURE----- --gKMricLos+KVdGMg--