[bug report] nfs: make sillyrename an async operation

[bug report] nfs: make sillyrename an async operation

[Dan Carpenter]

[ Ancient code.  But NeilB changed get_cred() recently so Smatch thinks
  it understands it now.  - dan ]

Hello Jeff Layton,

The patch d3d4152a5d59: "nfs: make sillyrename an async operation"
from Sep 17, 2010, leads to the following static checker warning:

	fs/nfs/unlink.c:351 nfs_async_rename()
	warn: 'data->cred' isn't an ERR_PTR

fs/nfs/unlink.c
   330  struct rpc_task *
   331  nfs_async_rename(struct inode *old_dir, struct inode *new_dir,
   332                   struct dentry *old_dentry, struct dentry *new_dentry,
   333                   void (*complete)(struct rpc_task *, struct nfs_renamedata *))
   334  {
   335          struct nfs_renamedata *data;
   336          struct rpc_message msg = { };
   337          struct rpc_task_setup task_setup_data = {
   338                  .rpc_message = &msg,
   339                  .callback_ops = &nfs_rename_ops,
   340                  .workqueue = nfsiod_workqueue,
   341                  .rpc_client = NFS_CLIENT(old_dir),
   342                  .flags = RPC_TASK_ASYNC,
   343          };
   344  
   345          data = kzalloc(sizeof(*data), GFP_KERNEL);
   346          if (data == NULL)
   347                  return ERR_PTR(-ENOMEM);
   348          task_setup_data.callback_data = data;
   349  
   350          data->cred = get_current_cred();
   351          if (IS_ERR(data->cred)) {

get_current_cred() always returns a valid pointer, I think?  Can we
just remove this condition?

   352                  struct rpc_task *task = ERR_CAST(data->cred);
   353                  kfree(data);
   354                  return task;
   355          }

regards,
dan carpenter

Re: [bug report] nfs: make sillyrename an async operation

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2123 bytes --]

On Wed, Dec 19 2018, Dan Carpenter wrote:

> [ Ancient code.  But NeilB changed get_cred() recently so Smatch thinks
>   it understands it now.  - dan ]
>
> Hello Jeff Layton,
>
> The patch d3d4152a5d59: "nfs: make sillyrename an async operation"
> from Sep 17, 2010, leads to the following static checker warning:
>
> 	fs/nfs/unlink.c:351 nfs_async_rename()
> 	warn: 'data->cred' isn't an ERR_PTR

It would have been impressive to determine that at the time, though I
think it was true.
At the time it was rpc_lookup_cred(), that provided data->cred.
That was
        return rpcauth_lookupcred(&generic_auth, 0);
of which the key line is :
        ret = auth->au_ops->lookup_cred(auth, &acred, flags);

and generic_auth.au_ops->lookup_cred was
        return rpcauth_lookup_credcache(&generic_auth, acred, flags);

which can only return an error if
              cred->cr_ops->cr_init != NULL &&
and
                int res = cred->cr_ops->cr_init(auth, cred);
                if (res < 0) {

There is no generic_auth.au_ops->cr_init, so no error can be return.

It is much easier to determine that now.

Thanks,
NeilBrown

From: NeilBrown <neilb@suse.com>
Date: Thu, 20 Dec 2018 10:29:55 +1100
Subject: [PATCH] NFS: remove unnecessary test for IS_ERR(cred)

As gte_current_cred() cannot return an error,
this test is not necessary.
It hasn't been necessary for years, but it wasn't so obvious
before.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/unlink.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index a227ab7d6891..79b97b3c4427 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -348,11 +348,6 @@ nfs_async_rename(struct inode *old_dir, struct inode *new_dir,
 	task_setup_data.callback_data = data;
 
 	data->cred = get_current_cred();
-	if (IS_ERR(data->cred)) {
-		struct rpc_task *task = ERR_CAST(data->cred);
-		kfree(data);
-		return task;
-	}
 
 	msg.rpc_argp = &data->args;
 	msg.rpc_resp = &data->res;
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]