Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757430AbbDVFqC (ORCPT ); Wed, 22 Apr 2015 01:46:02 -0400 Received: from mga02.intel.com ([134.134.136.20]:2591 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755348AbbDVFp7 convert rfc822-to-8bit (ORCPT ); Wed, 22 Apr 2015 01:45:59 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,621,1422950400"; d="scan'208";a="484023053" From: "Dilger, Andreas" To: Boqun Feng , "linux-fsdevel@vger.kernel.org" , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" CC: Al Viro , "Drokin, Oleg" , Greg Kroah-Hartman , Jan Kara Subject: Re: [PATCH 2/2] staging: lustre: replace ll_{get,put}name() with {get,put}name() Thread-Topic: [PATCH 2/2] staging: lustre: replace ll_{get,put}name() with {get,put}name() Thread-Index: AQHQfK+Pou8oOwX4Eka5p7rSgm2p0J1YlpeA Date: Wed, 22 Apr 2015 05:45:55 +0000 Message-ID: References: <1429674624-25922-1-git-send-email-boqun.feng@gmail.com> <1429674624-25922-3-git-send-email-boqun.feng@gmail.com> In-Reply-To: <1429674624-25922-3-git-send-email-boqun.feng@gmail.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.252.137.203] Content-Type: text/plain; charset="us-ascii" Content-ID: <3CD779ABF8B81249A0BA361CE5C4A3C5@intel.com> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5505 Lines: 187 On 2015/04/21, 9:50 PM, "Boqun Feng" wrote: >As pointed by Al Viro: > >https://lkml.org/lkml/2015/4/11/243 > >There are bugs in ll_getname() because of wrong assumptions of returning >values from strncpy_from_user(). Moreover, what ll_getname want to do is >just to try copy the file name from userland. Since we already have >getname() for the same purpose, it's better to replace ll_getname() with >getname(), so is ll_putname(). > >Besides, remove unused code for checking whether namelen is 0 or not in >case LL_IOC_REMOVE_ENTRY, because zero-length file name is already >handled by getname() in the same way as ll_getname(). > >Suggested-by: Al Viro >Signed-off-by: Boqun Feng Looks good, you can add my: Reviewed-by: Andreas Dilger >--- > drivers/staging/lustre/lustre/llite/dir.c | 60 >++++++---------------- > .../staging/lustre/lustre/llite/llite_internal.h | 2 +- > drivers/staging/lustre/lustre/llite/namei.c | 2 +- > 3 files changed, 18 insertions(+), 46 deletions(-) > >diff --git a/drivers/staging/lustre/lustre/llite/dir.c >b/drivers/staging/lustre/lustre/llite/dir.c >index a182019..c75fc38 100644 >--- a/drivers/staging/lustre/lustre/llite/dir.c >+++ b/drivers/staging/lustre/lustre/llite/dir.c >@@ -1216,30 +1216,6 @@ out: > return rc; > } > >-static char * >-ll_getname(const char __user *filename) >-{ >- int ret = 0, len; >- 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; >-} >- >-#define ll_putname(filename) __putname(filename) >- > static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned >long arg) > { > struct inode *inode = file_inode(file); >@@ -1441,7 +1417,7 @@ free_lmv: > return rc; > } > case LL_IOC_REMOVE_ENTRY: { >- char *filename = NULL; >+ struct filename *name = NULL; > int namelen = 0; > int rc; > >@@ -1453,20 +1429,16 @@ free_lmv: > if (!(exp_connect_flags(sbi->ll_md_exp) & OBD_CONNECT_LVB_TYPE)) > return -ENOTSUPP; > >- filename = ll_getname((const char *)arg); >- if (IS_ERR(filename)) >- return PTR_ERR(filename); >+ name = getname((const char *)arg); >+ if (IS_ERR(name)) >+ return PTR_ERR(name); > >- namelen = strlen(filename); >- if (namelen < 1) { >- rc = -EINVAL; >- goto out_rmdir; >- } >+ namelen = strlen(name->name); >+ >+ rc = ll_rmdir_entry(inode, name->name, namelen); > >- rc = ll_rmdir_entry(inode, filename, namelen); >-out_rmdir: >- if (filename) >- ll_putname(filename); >+ if (name) >+ putname(name); > return rc; > } > case LL_IOC_LOV_SWAP_LAYOUTS: >@@ -1481,16 +1453,16 @@ out_rmdir: > struct lov_user_md *lump; > struct lov_mds_md *lmm = NULL; > struct mdt_body *body; >- char *filename = NULL; >+ struct filename *name = NULL; > int lmmsize; > > if (cmd == IOC_MDC_GETFILEINFO || > cmd == IOC_MDC_GETFILESTRIPE) { >- filename = ll_getname((const char *)arg); >- if (IS_ERR(filename)) >- return PTR_ERR(filename); >+ name = getname((const char *)arg); >+ if (IS_ERR(name)) >+ return PTR_ERR(name); > >- rc = ll_lov_getstripe_ea_info(inode, filename, &lmm, >+ rc = ll_lov_getstripe_ea_info(inode, name->name, &lmm, > &lmmsize, &request); > } else { > rc = ll_dir_getstripe(inode, &lmm, &lmmsize, &request); >@@ -1556,8 +1528,8 @@ skip_lmm: > > out_req: > ptlrpc_req_finished(request); >- if (filename) >- ll_putname(filename); >+ if (name) >+ putname(name); > return rc; > } > case IOC_LOV_GETINFO: { >diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h >b/drivers/staging/lustre/lustre/llite/llite_internal.h >index 2af1d72..0950565 100644 >--- a/drivers/staging/lustre/lustre/llite/llite_internal.h >+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h >@@ -714,7 +714,7 @@ struct inode *ll_iget(struct super_block *sb, ino_t >hash, > int ll_md_blocking_ast(struct ldlm_lock *, struct ldlm_lock_desc *, > void *data, int flag); > struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de); >-int ll_rmdir_entry(struct inode *dir, char *name, int namelen); >+int ll_rmdir_entry(struct inode *dir, const char *name, int namelen); > > /* llite/rw.c */ > int ll_prepare_write(struct file *, struct page *, unsigned from, >unsigned to); >diff --git a/drivers/staging/lustre/lustre/llite/namei.c >b/drivers/staging/lustre/lustre/llite/namei.c >index 890ac19..ec48d8d 100644 >--- a/drivers/staging/lustre/lustre/llite/namei.c >+++ b/drivers/staging/lustre/lustre/llite/namei.c >@@ -867,7 +867,7 @@ static inline void ll_get_child_fid(struct dentry >*child, struct lu_fid *fid) > /** > * Remove dir entry > **/ >-int ll_rmdir_entry(struct inode *dir, char *name, int namelen) >+int ll_rmdir_entry(struct inode *dir, const char *name, int namelen) > { > struct ptlrpc_request *request = NULL; > struct md_op_data *op_data; >-- >2.3.5 > > Cheers, Andreas -- Andreas Dilger Lustre Software Architect Intel High Performance Data Division -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/