| Message ID | 20240828044355.590260-1-yanzhen@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] sunrpc: Fix error checking for d_hash_and_lookup() | expand |
On Wed, 2024-08-28 at 12:43 +0800, Yan Zhen wrote: > The d_hash_and_lookup() function returns either an error pointer or NULL. > > It might be more appropriate to check error using IS_ERR_OR_NULL(). > > Fixes: 4b9a445e3eeb ("sunrpc: create a new dummy pipe for gssd to hold open") > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > --- > > Changes in v3: > - Rewrite the "fixes". > - Using ERR_CAST(gssd_dentry) instead of ERR_PTR(-ENOENT). > > net/sunrpc/rpc_pipe.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c > index 910a5d850d04..13e905f34359 100644 > --- a/net/sunrpc/rpc_pipe.c > +++ b/net/sunrpc/rpc_pipe.c > @@ -1306,8 +1306,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) > > /* We should never get this far if "gssd" doesn't exist */ > gssd_dentry = d_hash_and_lookup(root, &q); > - if (!gssd_dentry) > - return ERR_PTR(-ENOENT); > + if (IS_ERR_OR_NULL(gssd_dentry)) > + return ERR_CAST(gssd_dentry); If you get back a NULL, then ERR_CAST will just make this return a NULL pointer. > > ret = rpc_populate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1, NULL); > if (ret) { > @@ -1318,7 +1318,7 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) > q.name = gssd_dummy_clnt_dir[0].name; > q.len = strlen(gssd_dummy_clnt_dir[0].name); > clnt_dentry = d_hash_and_lookup(gssd_dentry, &q); > - if (!clnt_dentry) { > + if (IS_ERR_OR_NULL(clnt_dentry)) { > __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1); > pipe_dentry = ERR_PTR(-ENOENT); > goto out; ...you probably also want to make this return the error from d_hash_and_lookup as well when there is one.
On Wed, 28 Aug 2024, Jeff Layton wrote: > On Wed, 2024-08-28 at 12:43 +0800, Yan Zhen wrote: > > The d_hash_and_lookup() function returns either an error pointer or NULL. > > > > It might be more appropriate to check error using IS_ERR_OR_NULL(). > > > > Fixes: 4b9a445e3eeb ("sunrpc: create a new dummy pipe for gssd to hold open") > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > > --- > > > > Changes in v3: > > - Rewrite the "fixes". > > - Using ERR_CAST(gssd_dentry) instead of ERR_PTR(-ENOENT). > > > > net/sunrpc/rpc_pipe.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c > > index 910a5d850d04..13e905f34359 100644 > > --- a/net/sunrpc/rpc_pipe.c > > +++ b/net/sunrpc/rpc_pipe.c > > @@ -1306,8 +1306,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) > > > > /* We should never get this far if "gssd" doesn't exist */ > > gssd_dentry = d_hash_and_lookup(root, &q); > > - if (!gssd_dentry) > > - return ERR_PTR(-ENOENT); > > + if (IS_ERR_OR_NULL(gssd_dentry)) > > + return ERR_CAST(gssd_dentry); > > If you get back a NULL, then ERR_CAST will just make this return a NULL > pointer. > > > > > ret = rpc_populate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1, NULL); > > if (ret) { > > @@ -1318,7 +1318,7 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) > > q.name = gssd_dummy_clnt_dir[0].name; > > q.len = strlen(gssd_dummy_clnt_dir[0].name); > > clnt_dentry = d_hash_and_lookup(gssd_dentry, &q); > > - if (!clnt_dentry) { > > + if (IS_ERR_OR_NULL(clnt_dentry)) { > > __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1); > > pipe_dentry = ERR_PTR(-ENOENT); > > goto out; > > ...you probably also want to make this return the error from > d_hash_and_lookup as well when there is one. I'd like to just throw in here that in this circumstance, d_hash_and_lookup() will never return an error. It only ever returns an error that it gets from ->d_hash, and ->d_hash is specific to the filesystem, and the filesystem here is the rpc_pipe virtual filesystem which doesn't define a ->d_hash. So errors are impossible. While I'm generally in favour of making code more robust and don't object to the IS_ERR_OR_NULL conversion, I think we should be *very* cautious not to introduce a bug where no bug currently exists. I would rather the return values were no changed. NeilBrown
Hi Yan,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yan-Zhen/sunrpc-Fix-error-checking-for-d_hash_and_lookup/20240828-124615
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link: https://lore.kernel.org/r/20240828044355.590260-1-yanzhen%40vivo.com
patch subject: [PATCH v3] sunrpc: Fix error checking for d_hash_and_lookup()
config: i386-randconfig-141-20240829 (https://download.01.org/0day-ci/archive/20240829/202408290616.Ke1JxTcE-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408290616.Ke1JxTcE-lkp@intel.com/
smatch warnings:
net/sunrpc/rpc_pipe.c:1310 rpc_gssd_dummy_populate() warn: passing zero to 'ERR_CAST'
vim +/ERR_CAST +1310 net/sunrpc/rpc_pipe.c
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1297 static struct dentry *
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1298 rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1299 {
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1300 int ret = 0;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1301 struct dentry *gssd_dentry;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1302 struct dentry *clnt_dentry = NULL;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1303 struct dentry *pipe_dentry = NULL;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1304 struct qstr q = QSTR_INIT(files[RPCAUTH_gssd].name,
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1305 strlen(files[RPCAUTH_gssd].name));
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1306
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1307 /* We should never get this far if "gssd" doesn't exist */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1308 gssd_dentry = d_hash_and_lookup(root, &q);
cf4564e657c6275 Yan Zhen 2024-08-28 1309 if (IS_ERR_OR_NULL(gssd_dentry))
cf4564e657c6275 Yan Zhen 2024-08-28 @1310 return ERR_CAST(gssd_dentry);
The callers are not expecting a NULL return from rpc_gssd_dummy_populate() so
this will lead to a crash.
The comments to d_hash_and_lookup() say it returns NULL if the file doesn't
exist and the error pointers if the filename is invalid. Neither one should be
possible here according to the comments on line 1307. So we're debating about
how to handle impossible situations.
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1311
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1312 ret = rpc_populate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1, NULL);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1313 if (ret) {
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1314 pipe_dentry = ERR_PTR(ret);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1315 goto out;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1316 }
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1317
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1318 q.name = gssd_dummy_clnt_dir[0].name;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1319 q.len = strlen(gssd_dummy_clnt_dir[0].name);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1320 clnt_dentry = d_hash_and_lookup(gssd_dentry, &q);
cf4564e657c6275 Yan Zhen 2024-08-28 1321 if (IS_ERR_OR_NULL(clnt_dentry)) {
b7ade38165ca000 Vasily Averin 2020-06-01 1322 __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1323 pipe_dentry = ERR_PTR(-ENOENT);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1324 goto out;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1325 }
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1326
e2f0c83a9de331d Jeff Layton 2013-12-05 1327 ret = rpc_populate(clnt_dentry, gssd_dummy_info_file, 0, 1, NULL);
e2f0c83a9de331d Jeff Layton 2013-12-05 1328 if (ret) {
e2f0c83a9de331d Jeff Layton 2013-12-05 1329 __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
e2f0c83a9de331d Jeff Layton 2013-12-05 1330 pipe_dentry = ERR_PTR(ret);
e2f0c83a9de331d Jeff Layton 2013-12-05 1331 goto out;
e2f0c83a9de331d Jeff Layton 2013-12-05 1332 }
e2f0c83a9de331d Jeff Layton 2013-12-05 1333
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1334 pipe_dentry = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
e2f0c83a9de331d Jeff Layton 2013-12-05 1335 if (IS_ERR(pipe_dentry)) {
e2f0c83a9de331d Jeff Layton 2013-12-05 1336 __rpc_depopulate(clnt_dentry, gssd_dummy_info_file, 0, 1);
3396f92f8be606e Jeff Layton 2013-12-05 1337 __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
e2f0c83a9de331d Jeff Layton 2013-12-05 1338 }
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1339 out:
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1340 dput(clnt_dentry);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1341 dput(gssd_dentry);
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1342 return pipe_dentry;
4b9a445e3eeb8bd Jeff Layton 2013-11-14 1343 }
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c index 910a5d850d04..13e905f34359 100644 --- a/net/sunrpc/rpc_pipe.c +++ b/net/sunrpc/rpc_pipe.c @@ -1306,8 +1306,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) /* We should never get this far if "gssd" doesn't exist */ gssd_dentry = d_hash_and_lookup(root, &q); - if (!gssd_dentry) - return ERR_PTR(-ENOENT); + if (IS_ERR_OR_NULL(gssd_dentry)) + return ERR_CAST(gssd_dentry); ret = rpc_populate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1, NULL); if (ret) { @@ -1318,7 +1318,7 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) q.name = gssd_dummy_clnt_dir[0].name; q.len = strlen(gssd_dummy_clnt_dir[0].name); clnt_dentry = d_hash_and_lookup(gssd_dentry, &q); - if (!clnt_dentry) { + if (IS_ERR_OR_NULL(clnt_dentry)) { __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1); pipe_dentry = ERR_PTR(-ENOENT); goto out;
The d_hash_and_lookup() function returns either an error pointer or NULL. It might be more appropriate to check error using IS_ERR_OR_NULL(). Fixes: 4b9a445e3eeb ("sunrpc: create a new dummy pipe for gssd to hold open") Signed-off-by: Yan Zhen <yanzhen@vivo.com> --- Changes in v3: - Rewrite the "fixes". - Using ERR_CAST(gssd_dentry) instead of ERR_PTR(-ENOENT). net/sunrpc/rpc_pipe.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)