From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boqun Feng Subject: Re: Is it OK to export symbols 'getname' and 'putname'? Date: Sat, 18 Apr 2015 08:20:13 +0800 Message-ID: <20150418002012.GA6341@fixme-laptop> References: <1427309152-25129-1-git-send-email-boqun.feng@gmail.com> <20150329042744.GA1477@fixme-laptop> <20150407083826.GA1048@fixme-laptop.cn.ibm.com> <20150411235655.GK889@ZenIV.linux.org.uk> <20150412011318.GL889@ZenIV.linux.org.uk> <20150417123530.GA4249@fixme-laptop.cn.ibm.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9jxsPFA5p3P2qPhR" Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Al Viro Return-path: Content-Disposition: inline In-Reply-To: <20150417123530.GA4249@fixme-laptop.cn.ibm.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Apr 17, 2015 at 08:35:30PM +0800, Boqun Feng wrote: > Hi Al, >=20 > On Sun, Apr 12, 2015 at 02:13:18AM +0100, Al Viro wrote: > >=20 > > BTW, looking at the __getname() callers... Lustre one sure as hell loo= ks > > bogus: > > char *tmp =3D __getname(); > >=20 > > if (!tmp) > > return ERR_PTR(-ENOMEM); > >=20 > > len =3D strncpy_from_user(tmp, filename, PATH_MAX); > > if (len =3D=3D 0) > > ret =3D -ENOENT; > > else if (len > PATH_MAX) > > ret =3D -ENAMETOOLONG; > >=20 > > if (ret) { > > __putname(tmp); > > tmp =3D ERR_PTR(ret); > > } > > return tmp; > >=20 > > Note that > > * strncpy_from_user(p, u, n) can return a negative (-EFAULT) > > * strncpy_from_user(p, u, n) cannot return a positive greater than > > n. In case of missing NUL among the n bytes starting at u (and no faul= ts > > accessing those) we get exactly n bytes copied and n returned. In case > > when NUL is there, we copy everything up to and including that NUL and > > return number of non-NUL bytes copied. > >=20 > > IOW, these failure cases had never been tested. Name being too long en= ds up > > with non-NUL-terminated array of characters returned, and the very first > > caller of ll_getname() feeds it to strlen(). Fault ends up with uninit= ialized > > array... > >=20 > > AFAICS, the damn thing should just use getname() and quit reinventing t= he > > wheel, badly. > >=20 >=20 > I'm trying to clean that part of code you mentioned, and I found I have > to export the symbols 'getname' and 'putname' as follow to replace that > __getname() caller: >=20 > diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/= lustre/lustre/llite/dir.c > index a182019..014f51a 100644 > --- a/drivers/staging/lustre/lustre/llite/dir.c > +++ b/drivers/staging/lustre/lustre/llite/dir.c > @@ -1216,29 +1216,8 @@ out: > return rc; > } > =20 > -static char * > -ll_getname(const char __user *filename) > -{ > - int ret =3D 0, len; > - char *tmp =3D __getname(); > - > - if (!tmp) > - return ERR_PTR(-ENOMEM); > - > - len =3D strncpy_from_user(tmp, filename, PATH_MAX); > - if (len =3D=3D 0) > - ret =3D -ENOENT; > - else if (len > PATH_MAX) > - ret =3D -ENAMETOOLONG; > - > - if (ret) { > - __putname(tmp); > - tmp =3D ERR_PTR(ret); > - } > - return tmp; > -} > - > -#define ll_putname(filename) __putname(filename) > +#define ll_getname(filename) getname(filename) > +#define ll_putname(name) putname(name) > =20 > static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned l= ong arg) > { > @@ -1441,6 +1420,7 @@ free_lmv: > return rc; > } > case LL_IOC_REMOVE_ENTRY: { > + struct filename *name =3D NULL; > char *filename =3D NULL; > int namelen =3D 0; > int rc; > @@ -1453,20 +1433,17 @@ free_lmv: > if (!(exp_connect_flags(sbi->ll_md_exp) & OBD_CONNECT_LVB_TYPE)) > return -ENOTSUPP; > =20 > - filename =3D ll_getname((const char *)arg); > - if (IS_ERR(filename)) > - return PTR_ERR(filename); > + name =3D ll_getname((const char *)arg); > + if (IS_ERR(name)) > + return PTR_ERR(name); > =20 > + filename =3D name->name; > namelen =3D strlen(filename); > - if (namelen < 1) { > - rc =3D -EINVAL; > - goto out_rmdir; > - } > =20 > rc =3D ll_rmdir_entry(inode, filename, namelen); > out_rmdir: > - if (filename) > - ll_putname(filename); > + if (name) > + ll_putname(name); > return rc; > } > case LL_IOC_LOV_SWAP_LAYOUTS: > @@ -1481,15 +1458,17 @@ out_rmdir: > struct lov_user_md *lump; > struct lov_mds_md *lmm =3D NULL; > struct mdt_body *body; > + struct filename *name; > char *filename =3D NULL; > int lmmsize; > =20 > if (cmd =3D=3D IOC_MDC_GETFILEINFO || > cmd =3D=3D IOC_MDC_GETFILESTRIPE) { > - filename =3D ll_getname((const char *)arg); > - if (IS_ERR(filename)) > - return PTR_ERR(filename); > + name =3D ll_getname((const char *)arg); > + if (IS_ERR(name)) > + return PTR_ERR(name); > =20 > + filename =3D name->name; > rc =3D ll_lov_getstripe_ea_info(inode, filename, &lmm, > &lmmsize, &request); > } else { Sorry.. one modification is missing here: @@ -1535,8 +1535,8 @@ skip_lmm: =20 out_req: ptlrpc_req_finished(request); - if (filename) - ll_putname(filename); + if (name) + ll_putname(name); return rc; } case IOC_LOV_GETINFO: { Regards, Boqun > diff --git a/fs/namei.c b/fs/namei.c > index ffab2e0..7a0948c 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -205,6 +205,7 @@ getname(const char __user * filename) > { > return getname_flags(filename, 0, NULL); > } > +EXPORT_SYMBOL(getname); > =20 > struct filename * > getname_kernel(const char * filename) > @@ -254,6 +255,7 @@ void putname(struct filename *name) > } else > __putname(name); > } > +EXPORT_SYMBOL(putname); > =20 > static int check_acl(struct inode *inode, int mask) > { >=20 >=20 >=20 > Is that a good idea to export these symbols, given that lustre may be > the only user?=20 >=20 > Looking forward to your insight. >=20 > Thanks, > Boqun --9jxsPFA5p3P2qPhR Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJVMaM2AAoJEEl56MO1B/q42M8IAIhfeUjV/c7y1+ZdUb4xvJSY Hje0MUTlnSNMwpfceR9l8IBGXryiueHT17bdlHyui5gh//AHjmsmiMc2N7ay0lfZ +AzOapNZ7xFAkY36UD3nX2qftZQOrYcGKQQE86rrEKGoixUEHLOxv/fojCl3QGhu z+tShlFykHdhybwq36E7acL42nn+zqIDFRh7Jrj1JOKrBhH4lW2768Iey7uPpFv/ 5XJH6xfzCaDji1TPbznG3alqe7cOaHU2k8Ps/rrqhekKlIgNtqLadjSga+JKlX92 gPxC3JIdCFEr/S+53YhSs0gwNnz8oblaexTlJyCaeT9kp5lR7r1lXkP0QghrOBc= =1E0S -----END PGP SIGNATURE----- --9jxsPFA5p3P2qPhR--