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

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

[Yan Zhen]

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);
 
 	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;
-- 
2.34.1

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

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>

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

[NeilBrown]

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

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

[Dan Carpenter]

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  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki