[bug report] ceph: attempt to do async create when possible

[bug report] ceph: attempt to do async create when possible

[Dan Carpenter]

Hello Jeff Layton,

The patch 9a8d03ca2e2c: "ceph: attempt to do async create when
possible" from Nov 27, 2019, leads to the following static checker
warning:

	fs/ceph/file.c:540 ceph_async_create_cb()
	error: uninitialized symbol 'base'.

fs/ceph/file.c
   526          mapping_set_error(req->r_parent->i_mapping, result);
   527  
   528          if (result) {
   529                  struct dentry *dentry = req->r_dentry;
   530                  int pathlen;
   531                  u64 base;
                        ^^^^^^^^
   532                  char *path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
   533                                                    &base, 0);
                                                          ^^^^^
   534  
   535                  ceph_dir_clear_complete(req->r_parent);
   536                  if (!d_unhashed(dentry))
   537                          d_drop(dentry);
   538  
   539                  /* FIXME: start returning I/O errors on all accesses? */
   540                  pr_warn("ceph: async create failure path=(%llx)%s result=%d!\n",
   541                          base, IS_ERR(path) ? "<<bad>>" : path, result);
                                ^^^^
Potentialy uninitialized on error.

   542                  ceph_mdsc_free_path(path, pathlen);
   543          }
   544  
   545          if (req->r_target_inode) {

regards,
dan carpenter

Re: [bug report] ceph: attempt to do async create when possible

[Dan Carpenter]

On Wed, Apr 08, 2020 at 02:17:34PM +0300, Dan Carpenter wrote:
> Hello Jeff Layton,
> 
> The patch 9a8d03ca2e2c: "ceph: attempt to do async create when
> possible" from Nov 27, 2019, leads to the following static checker
> warning:
> 
> 	fs/ceph/file.c:540 ceph_async_create_cb()
> 	error: uninitialized symbol 'base'.
> 
> fs/ceph/file.c
>    526          mapping_set_error(req->r_parent->i_mapping, result);
>    527  
>    528          if (result) {
>    529                  struct dentry *dentry = req->r_dentry;
>    530                  int pathlen;
>    531                  u64 base;
>                         ^^^^^^^^
>    532                  char *path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
>    533                                                    &base, 0);
>                                                           ^^^^^
>    534  
>    535                  ceph_dir_clear_complete(req->r_parent);
>    536                  if (!d_unhashed(dentry))
>    537                          d_drop(dentry);
>    538  
>    539                  /* FIXME: start returning I/O errors on all accesses? */
>    540                  pr_warn("ceph: async create failure path=(%llx)%s result=%d!\n",
>    541                          base, IS_ERR(path) ? "<<bad>>" : path, result);
>                                 ^^^^
> Potentialy uninitialized on error.
> 
>    542                  ceph_mdsc_free_path(path, pathlen);
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Also this is quite problematic.  "pathlen" can be uninitialized, but
even worse the ceph_mdsc_free_path() assumes "path" is NULL on error
instead of an error pointer.

>    543          }
>    544  
>    545          if (req->r_target_inode) {

regards,
dan carpenter

Re: [bug report] ceph: attempt to do async create when possible

[Dan Carpenter]

On Wed, Apr 08, 2020 at 02:17:34PM +0300, Dan Carpenter wrote:
> Hello Jeff Layton,
> 
> The patch 9a8d03ca2e2c: "ceph: attempt to do async create when
> possible" from Nov 27, 2019, leads to the following static checker
> warning:
> 
> 	fs/ceph/file.c:540 ceph_async_create_cb()
> 	error: uninitialized symbol 'base'.

Sorry, see the unlink function as well.

    fs/ceph/dir.c:1072 ceph_async_unlink_cb()
    error: uninitialized symbol 'pathlen'.

regards,
dan carpenter

Re: [bug report] ceph: attempt to do async create when possible

[Jeff Layton]

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

On Wed, 2020-04-08 at 14:22 +0300, Dan Carpenter wrote:
> On Wed, Apr 08, 2020 at 02:17:34PM +0300, Dan Carpenter wrote:
> > Hello Jeff Layton,
> > 
> > The patch 9a8d03ca2e2c: "ceph: attempt to do async create when
> > possible" from Nov 27, 2019, leads to the following static checker
> > warning:
> > 
> > 	fs/ceph/file.c:540 ceph_async_create_cb()
> > 	error: uninitialized symbol 'base'.
> 
> Sorry, see the unlink function as well.
> 
>     fs/ceph/dir.c:1072 ceph_async_unlink_cb()
>     error: uninitialized symbol 'pathlen'.
> 
> regards,
> dan carpenter
> 

Many thanks, Dan!

I think the right thing to do is probably to just make
ceph_mdsc_free_path not do anything when it gets a IS_ERR value. At the
same time, this is also an error path, so we might as well initialize
the values we're passing into ceph_mdsc_build_path to make sure the
warnings look sane. See the attached two patches.

Since this code is not upstream yet, I'll probably just move the first
one to be before the async create/delete patches go in, and squash the
second one into the respective patches that add that functionality so we
don't have any regressions.

Sound ok?
-- 
Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: 0001-ceph-have-ceph_mdsc_free_path-ignore-ERR_PTR-values.patch --]
[-- Type: text/x-patch, Size: 809 bytes --]

From 06e892961013cfbd6f48239794669ba34b812052 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@kernel.org>
Date: Wed, 8 Apr 2020 08:40:33 -0400
Subject: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

This makes the error handling simpler in some callers.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/mds_client.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 1b40f30e0a8e..754e0682398e 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -531,7 +531,7 @@ extern void ceph_mdsc_pre_umount(struct ceph_mds_client *mdsc);
 
 static inline void ceph_mdsc_free_path(char *path, int len)
 {
-	if (path)
+	if (path && !IS_ERR(path))
 		__putname(path - (PATH_MAX - 1 - len));
 }
 
-- 
2.25.2


[-- Attachment #3: 0002-SQUASH-initialize-path-and-base-values-in-async-diro.patch --]
[-- Type: text/x-patch, Size: 1302 bytes --]

From 066f69dfd78a20ad9aa5f7e387da6886c8410ce2 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@kernel.org>
Date: Wed, 8 Apr 2020 08:41:38 -0400
Subject: [PATCH 2/2] SQUASH: initialize path and base values in async dirops
 cb's

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c  | 4 ++--
 fs/ceph/file.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 9d02d4feb693..39f5311404b0 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1057,8 +1057,8 @@ static void ceph_async_unlink_cb(struct ceph_mds_client *mdsc,
 
 	/* If op failed, mark everyone involved for errors */
 	if (result) {
-		int pathlen;
-		u64 base;
+		int pathlen = 0;
+		u64 base = 0;
 		char *path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
 						  &base, 0);
 
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 3a1bd13de84f..160644ddaeed 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -529,8 +529,8 @@ static void ceph_async_create_cb(struct ceph_mds_client *mdsc,
 
 	if (result) {
 		struct dentry *dentry = req->r_dentry;
-		int pathlen;
-		u64 base;
+		int pathlen = 0;
+		u64 base = 0;
 		char *path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
 						  &base, 0);
 
-- 
2.25.2

Re: [bug report] ceph: attempt to do async create when possible

[Dan Carpenter]

On Wed, Apr 08, 2020 at 08:48:05AM -0400, Jeff Layton wrote:
>
> Sound ok?

Sounds good.

>  fs/ceph/mds_client.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 1b40f30e0a8e..754e0682398e 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -531,7 +531,7 @@ extern void ceph_mdsc_pre_umount(struct ceph_mds_client *mdsc);
>  
>  static inline void ceph_mdsc_free_path(char *path, int len)
>  {
> -	if (path)
> +	if (path && !IS_ERR(path))

if (!IS_ERR_OR_NULL(path)

>  		__putname(path - (PATH_MAX - 1 - len));
>  }
>  
> -- 
> 2.25.2
> 

regards,
dan carpenter