From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755371AbbDTPzT (ORCPT ); Mon, 20 Apr 2015 11:55:19 -0400 Received: from cantor2.suse.de ([195.135.220.15]:44147 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755059AbbDTPzM (ORCPT ); Mon, 20 Apr 2015 11:55:12 -0400 Date: Mon, 20 Apr 2015 17:55:07 +0200 From: Jan Kara To: Boqun Feng Cc: Al Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: Is it OK to export symbols 'getname' and 'putname'? Message-ID: <20150420155507.GK17020@quack.suse.cz> 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: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150417123530.GA4249@fixme-laptop.cn.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 17-04-15 20:35:30, Boqun Feng wrote: > Hi Al, > > On Sun, Apr 12, 2015 at 02:13:18AM +0100, Al Viro wrote: > > > > BTW, looking at the __getname() callers... Lustre one sure as hell looks > > bogus: > > char *tmp = __getname(); > > > > if (!tmp) > > return ERR_PTR(-ENOMEM); > > > > len = strncpy_from_user(tmp, filename, PATH_MAX); > > if (len == 0) > > ret = -ENOENT; > > else if (len > PATH_MAX) > > ret = -ENAMETOOLONG; > > > > if (ret) { > > __putname(tmp); > > tmp = ERR_PTR(ret); > > } > > return tmp; > > > > 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 faults > > 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. > > > > IOW, these failure cases had never been tested. Name being too long ends 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 uninitialized > > array... > > > > AFAICS, the damn thing should just use getname() and quit reinventing the > > wheel, badly. > > > > 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: > > 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 ... > +#define ll_getname(filename) getname(filename) > +#define ll_putname(name) putname(name) Bonus points for getting rid of these useless defines. > 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); > > struct filename * > getname_kernel(const char * filename) > @@ -254,6 +255,7 @@ void putname(struct filename *name) > } else > __putname(name); > } > +EXPORT_SYMBOL(putname); > > static int check_acl(struct inode *inode, int mask) > { > > > > Is that a good idea to export these symbols, given that lustre may be > the only user? Yes, it is a good idea. Honza -- Jan Kara SUSE Labs, CR