diff mbox series

[v3] sunrpc: Fix error checking for d_hash_and_lookup()

Message ID 20240828044355.590260-1-yanzhen@vivo.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [v3] sunrpc: Fix error checking for d_hash_and_lookup() | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: kolga@netapp.com
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 18 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 10 this patch: 10
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-08-28--12-00 (tests: 712)

Commit Message

Yan Zhen Aug. 28, 2024, 4:43 a.m. UTC 
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(-)

Comments

Jeff Layton Aug. 28, 2024, 10:18 a.m. UTC | #1 
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.
NeilBrown Aug. 28, 2024, 10:20 p.m. UTC | #2 
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
Dan Carpenter Aug. 29, 2024, 9:49 a.m. UTC | #3 
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 mbox series

Patch

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;