[PATCH 0/2] ceph: fix error handling bugs in async dirops callbacks

[PATCH 0/2] ceph: fix error handling bugs in async dirops callbacks

[Jeff Layton]

This fixes a couple of problems that Dan spotted with a static checker.
Additionally the first patch also simplifies some of the error handling
in other callers.

Originally, I was going to squash these into the earlier patches that
introduced those bugs, but this is new code that isn't enabled by
default and Ilya was planning to a PR soon, so it's probably best to
just handle them as follow-on fixes to what's currently in
ceph-client/master.

Jeff Layton (2):
  ceph: have ceph_mdsc_free_path ignore ERR_PTR values
  ceph: initialize base and pathlen variables in async dirops cb's

 fs/ceph/debugfs.c    | 4 ----
 fs/ceph/dir.c        | 4 ++--
 fs/ceph/file.c       | 4 ++--
 fs/ceph/mds_client.c | 6 ++----
 fs/ceph/mds_client.h | 2 +-
 5 files changed, 7 insertions(+), 13 deletions(-)

-- 
2.25.2

[PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Jeff Layton]

This makes the error handling simpler in some callers, and fixes a
couple of bugs in the new async dirops callback code.

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

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index eebbce7c3b0c..3a198e40f100 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
 		} else if (req->r_dentry) {
 			path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
 						    &pathbase, 0);
-			if (IS_ERR(path))
-				path = NULL;
 			spin_lock(&req->r_dentry->d_lock);
 			seq_printf(s, " #%llx/%pd (%s)",
 				   ceph_ino(d_inode(req->r_dentry->d_parent)),
@@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
 		if (req->r_old_dentry) {
 			path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
 						    &pathbase, 0);
-			if (IS_ERR(path))
-				path = NULL;
 			spin_lock(&req->r_old_dentry->d_lock);
 			seq_printf(s, " #%llx/%pd (%s)",
 				   req->r_old_dentry_dir ?
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index a8a5b98148ec..e25ee9fe0ee8 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2535,11 +2535,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
 	msg->hdr.data_off = cpu_to_le16(0);
 
 out_free2:
-	if (freepath2)
-		ceph_mdsc_free_path((char *)path2, pathlen2);
+	ceph_mdsc_free_path((char *)path2, pathlen2);
 out_free1:
-	if (freepath1)
-		ceph_mdsc_free_path((char *)path1, pathlen1);
+	ceph_mdsc_free_path((char *)path1, pathlen1);
 out:
 	return msg;
 }
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 1b40f30e0a8e..43111e408fa2 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 (!IS_ERR_OR_NULL(path))
 		__putname(path - (PATH_MAX - 1 - len));
 }
 
-- 
2.25.2

[PATCH 2/2] ceph: initialize base and pathlen variables in async dirops cb's

[Jeff Layton]

Ensure that the pr_warn messages look sane even if ceph_mdsc_build_path
fails.

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 10d528a455d5..93476d447a4b 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: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Ilya Dryomov]

On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> This makes the error handling simpler in some callers, and fixes a
> couple of bugs in the new async dirops callback code.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/debugfs.c    | 4 ----
>  fs/ceph/mds_client.c | 6 ++----
>  fs/ceph/mds_client.h | 2 +-
>  3 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index eebbce7c3b0c..3a198e40f100 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
>                 } else if (req->r_dentry) {
>                         path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
>                                                     &pathbase, 0);
> -                       if (IS_ERR(path))
> -                               path = NULL;
>                         spin_lock(&req->r_dentry->d_lock);
>                         seq_printf(s, " #%llx/%pd (%s)",

Hi Jeff,

This ends up attempting to print an IS_ERR pointer as %s.

>                                    ceph_ino(d_inode(req->r_dentry->d_parent)),
> @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
>                 if (req->r_old_dentry) {
>                         path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
>                                                     &pathbase, 0);
> -                       if (IS_ERR(path))
> -                               path = NULL;
>                         spin_lock(&req->r_old_dentry->d_lock);
>                         seq_printf(s, " #%llx/%pd (%s)",

Ditto.

It looks like in newer kernels printf copes with this and outputs
"(efault)".  But anything older than 5.2 will crash.

Further, the code looks weird because ceph_mdsc_build_path() doesn't
return NULL, but path is tested for NULL in the call to seq_printf().

Why not just follow the same approach as existing mdsc_show()?  It
makes it clear that the error is handled and where the NULL pointer
comes from.  This kind of "don't handle errors and rely on everything
else being able to bail" approach is very fragile and hard to audit.

Thanks,

                Ilya

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Jeff Layton]

On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote:
> On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> wrote:
> > This makes the error handling simpler in some callers, and fixes a
> > couple of bugs in the new async dirops callback code.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/debugfs.c    | 4 ----
> >  fs/ceph/mds_client.c | 6 ++----
> >  fs/ceph/mds_client.h | 2 +-
> >  3 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > index eebbce7c3b0c..3a198e40f100 100644
> > --- a/fs/ceph/debugfs.c
> > +++ b/fs/ceph/debugfs.c
> > @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> >                 } else if (req->r_dentry) {
> >                         path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
> >                                                     &pathbase, 0);
> > -                       if (IS_ERR(path))
> > -                               path = NULL;
> >                         spin_lock(&req->r_dentry->d_lock);
> >                         seq_printf(s, " #%llx/%pd (%s)",
> 
> Hi Jeff,
> 
> This ends up attempting to print an IS_ERR pointer as %s.
> 
> >                                    ceph_ino(d_inode(req->r_dentry->d_parent)),
> > @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> >                 if (req->r_old_dentry) {
> >                         path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
> >                                                     &pathbase, 0);
> > -                       if (IS_ERR(path))
> > -                               path = NULL;
> >                         spin_lock(&req->r_old_dentry->d_lock);
> >                         seq_printf(s, " #%llx/%pd (%s)",
> 
> Ditto.
> 
> It looks like in newer kernels printf copes with this and outputs
> "(efault)".  But anything older than 5.2 will crash.
> 
> Further, the code looks weird because ceph_mdsc_build_path() doesn't
> return NULL, but path is tested for NULL in the call to seq_printf().
> 
> Why not just follow the same approach as existing mdsc_show()?  It
> makes it clear that the error is handled and where the NULL pointer
> comes from.  This kind of "don't handle errors and rely on everything
> else being able to bail" approach is very fragile and hard to audit.
> 

I don't see a problem with having a "free" routine ignore IS_ERR values
just like it does NULL values. How about I just trim off the other
deltas in this patch? Something like this?

------------------8<-----------------

[PATCH] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

This fixes a couple of bugs in the new async dirops callback code.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
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..43111e408fa2 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 (!IS_ERR_OR_NULL(path))
 		__putname(path - (PATH_MAX - 1 - len));
 }
 
-- 
2.25.2

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Ilya Dryomov]

On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote:
> > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > This makes the error handling simpler in some callers, and fixes a
> > > couple of bugs in the new async dirops callback code.
> > >
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/debugfs.c    | 4 ----
> > >  fs/ceph/mds_client.c | 6 ++----
> > >  fs/ceph/mds_client.h | 2 +-
> > >  3 files changed, 3 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > > index eebbce7c3b0c..3a198e40f100 100644
> > > --- a/fs/ceph/debugfs.c
> > > +++ b/fs/ceph/debugfs.c
> > > @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > >                 } else if (req->r_dentry) {
> > >                         path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
> > >                                                     &pathbase, 0);
> > > -                       if (IS_ERR(path))
> > > -                               path = NULL;
> > >                         spin_lock(&req->r_dentry->d_lock);
> > >                         seq_printf(s, " #%llx/%pd (%s)",
> >
> > Hi Jeff,
> >
> > This ends up attempting to print an IS_ERR pointer as %s.
> >
> > >                                    ceph_ino(d_inode(req->r_dentry->d_parent)),
> > > @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > >                 if (req->r_old_dentry) {
> > >                         path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
> > >                                                     &pathbase, 0);
> > > -                       if (IS_ERR(path))
> > > -                               path = NULL;
> > >                         spin_lock(&req->r_old_dentry->d_lock);
> > >                         seq_printf(s, " #%llx/%pd (%s)",
> >
> > Ditto.
> >
> > It looks like in newer kernels printf copes with this and outputs
> > "(efault)".  But anything older than 5.2 will crash.
> >
> > Further, the code looks weird because ceph_mdsc_build_path() doesn't
> > return NULL, but path is tested for NULL in the call to seq_printf().
> >
> > Why not just follow the same approach as existing mdsc_show()?  It
> > makes it clear that the error is handled and where the NULL pointer
> > comes from.  This kind of "don't handle errors and rely on everything
> > else being able to bail" approach is very fragile and hard to audit.
> >
>
> I don't see a problem with having a "free" routine ignore IS_ERR values
> just like it does NULL values. How about I just trim off the other
> deltas in this patch? Something like this?

I think it encourages fragile code.  Less so than functions that
return pointer, NULL or IS_ERR pointer, but still.  You yourself
almost fell into one of these traps while editing debugfs.c ;)

That said, I won't stand in the way here.  If you trim off everything
else, merge it together with the other patch so that it is introduced
along with the two users.

>
> ------------------8<-----------------
>
> [PATCH] ceph: have ceph_mdsc_free_path ignore ERR_PTR values
>
> This fixes a couple of bugs in the new async dirops callback code.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 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..43111e408fa2 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 (!IS_ERR_OR_NULL(path))
>                 __putname(path - (PATH_MAX - 1 - len));
>  }

Thanks,

                Ilya

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Jeff Layton]

On Mon, 2020-04-13 at 14:35 +0200, Ilya Dryomov wrote:
> On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote:
> > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > This makes the error handling simpler in some callers, and fixes a
> > > > couple of bugs in the new async dirops callback code.
> > > > 
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/debugfs.c    | 4 ----
> > > >  fs/ceph/mds_client.c | 6 ++----
> > > >  fs/ceph/mds_client.h | 2 +-
> > > >  3 files changed, 3 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > > > index eebbce7c3b0c..3a198e40f100 100644
> > > > --- a/fs/ceph/debugfs.c
> > > > +++ b/fs/ceph/debugfs.c
> > > > @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > > >                 } else if (req->r_dentry) {
> > > >                         path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
> > > >                                                     &pathbase, 0);
> > > > -                       if (IS_ERR(path))
> > > > -                               path = NULL;
> > > >                         spin_lock(&req->r_dentry->d_lock);
> > > >                         seq_printf(s, " #%llx/%pd (%s)",
> > > 
> > > Hi Jeff,
> > > 
> > > This ends up attempting to print an IS_ERR pointer as %s.
> > > 
> > > >                                    ceph_ino(d_inode(req->r_dentry->d_parent)),
> > > > @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > > >                 if (req->r_old_dentry) {
> > > >                         path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
> > > >                                                     &pathbase, 0);
> > > > -                       if (IS_ERR(path))
> > > > -                               path = NULL;
> > > >                         spin_lock(&req->r_old_dentry->d_lock);
> > > >                         seq_printf(s, " #%llx/%pd (%s)",
> > > 
> > > Ditto.
> > > 
> > > It looks like in newer kernels printf copes with this and outputs
> > > "(efault)".  But anything older than 5.2 will crash.
> > > 
> > > Further, the code looks weird because ceph_mdsc_build_path() doesn't
> > > return NULL, but path is tested for NULL in the call to seq_printf().
> > > 
> > > Why not just follow the same approach as existing mdsc_show()?  It
> > > makes it clear that the error is handled and where the NULL pointer
> > > comes from.  This kind of "don't handle errors and rely on everything
> > > else being able to bail" approach is very fragile and hard to audit.
> > > 
> > 
> > I don't see a problem with having a "free" routine ignore IS_ERR values
> > just like it does NULL values. How about I just trim off the other
> > deltas in this patch? Something like this?
> 
> I think it encourages fragile code.  Less so than functions that
> return pointer, NULL or IS_ERR pointer, but still.  You yourself
> almost fell into one of these traps while editing debugfs.c ;)
> 

We'll have to agree to disagree here. Having a free routine ignore
ERR_PTR values seems perfectly reasonable to me.

> That said, I won't stand in the way here.  If you trim off everything
> else, merge it together with the other patch so that it is introduced
> along with the two users.
> 

Do you mean that I should merge it with this?

    ceph: initialize base and pathlen variables in async dirops cb's

I'm not sure I see the point in that.

In any case, I pushed the updated patch into ceph-client/testing so let
me know if you see anything else amiss with it.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Ilya Dryomov]

On Mon, Apr 13, 2020 at 3:23 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2020-04-13 at 14:35 +0200, Ilya Dryomov wrote:
> > On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote:
> > > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > This makes the error handling simpler in some callers, and fixes a
> > > > > couple of bugs in the new async dirops callback code.
> > > > >
> > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/ceph/debugfs.c    | 4 ----
> > > > >  fs/ceph/mds_client.c | 6 ++----
> > > > >  fs/ceph/mds_client.h | 2 +-
> > > > >  3 files changed, 3 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > > > > index eebbce7c3b0c..3a198e40f100 100644
> > > > > --- a/fs/ceph/debugfs.c
> > > > > +++ b/fs/ceph/debugfs.c
> > > > > @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > > > >                 } else if (req->r_dentry) {
> > > > >                         path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
> > > > >                                                     &pathbase, 0);
> > > > > -                       if (IS_ERR(path))
> > > > > -                               path = NULL;
> > > > >                         spin_lock(&req->r_dentry->d_lock);
> > > > >                         seq_printf(s, " #%llx/%pd (%s)",
> > > >
> > > > Hi Jeff,
> > > >
> > > > This ends up attempting to print an IS_ERR pointer as %s.
> > > >
> > > > >                                    ceph_ino(d_inode(req->r_dentry->d_parent)),
> > > > > @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > > > >                 if (req->r_old_dentry) {
> > > > >                         path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
> > > > >                                                     &pathbase, 0);
> > > > > -                       if (IS_ERR(path))
> > > > > -                               path = NULL;
> > > > >                         spin_lock(&req->r_old_dentry->d_lock);
> > > > >                         seq_printf(s, " #%llx/%pd (%s)",
> > > >
> > > > Ditto.
> > > >
> > > > It looks like in newer kernels printf copes with this and outputs
> > > > "(efault)".  But anything older than 5.2 will crash.
> > > >
> > > > Further, the code looks weird because ceph_mdsc_build_path() doesn't
> > > > return NULL, but path is tested for NULL in the call to seq_printf().
> > > >
> > > > Why not just follow the same approach as existing mdsc_show()?  It
> > > > makes it clear that the error is handled and where the NULL pointer
> > > > comes from.  This kind of "don't handle errors and rely on everything
> > > > else being able to bail" approach is very fragile and hard to audit.
> > > >
> > >
> > > I don't see a problem with having a "free" routine ignore IS_ERR values
> > > just like it does NULL values. How about I just trim off the other
> > > deltas in this patch? Something like this?
> >
> > I think it encourages fragile code.  Less so than functions that
> > return pointer, NULL or IS_ERR pointer, but still.  You yourself
> > almost fell into one of these traps while editing debugfs.c ;)
> >
>
> We'll have to agree to disagree here. Having a free routine ignore
> ERR_PTR values seems perfectly reasonable to me.
>
> > That said, I won't stand in the way here.  If you trim off everything
> > else, merge it together with the other patch so that it is introduced
> > along with the two users.
> >
>
> Do you mean that I should merge it with this?
>
>     ceph: initialize base and pathlen variables in async dirops cb's
>
> I'm not sure I see the point in that.

Yes, because ultimately this is all about those two pr_warn messages.
path is built just to be printed, base and pathlen are a part of that.
ceph_mdsc_free_path() didn't need to handle IS_ERR pointers before so
it should be a single patch.

Thanks,

                Ilya

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Jeff Layton]

On Mon, 2020-04-13 at 17:48 +0200, Ilya Dryomov wrote:
> On Mon, Apr 13, 2020 at 3:23 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Mon, 2020-04-13 at 14:35 +0200, Ilya Dryomov wrote:
> > > On Mon, Apr 13, 2020 at 1:15 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > On Mon, 2020-04-13 at 10:09 +0200, Ilya Dryomov wrote:
> > > > > On Wed, Apr 8, 2020 at 4:21 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > > This makes the error handling simpler in some callers, and fixes a
> > > > > > couple of bugs in the new async dirops callback code.
> > > > > > 
> > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > ---
> > > > > >  fs/ceph/debugfs.c    | 4 ----
> > > > > >  fs/ceph/mds_client.c | 6 ++----
> > > > > >  fs/ceph/mds_client.h | 2 +-
> > > > > >  3 files changed, 3 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > > > > > index eebbce7c3b0c..3a198e40f100 100644
> > > > > > --- a/fs/ceph/debugfs.c
> > > > > > +++ b/fs/ceph/debugfs.c
> > > > > > @@ -83,8 +83,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > > > > >                 } else if (req->r_dentry) {
> > > > > >                         path = ceph_mdsc_build_path(req->r_dentry, &pathlen,
> > > > > >                                                     &pathbase, 0);
> > > > > > -                       if (IS_ERR(path))
> > > > > > -                               path = NULL;
> > > > > >                         spin_lock(&req->r_dentry->d_lock);
> > > > > >                         seq_printf(s, " #%llx/%pd (%s)",
> > > > > 
> > > > > Hi Jeff,
> > > > > 
> > > > > This ends up attempting to print an IS_ERR pointer as %s.
> > > > > 
> > > > > >                                    ceph_ino(d_inode(req->r_dentry->d_parent)),
> > > > > > @@ -102,8 +100,6 @@ static int mdsc_show(struct seq_file *s, void *p)
> > > > > >                 if (req->r_old_dentry) {
> > > > > >                         path = ceph_mdsc_build_path(req->r_old_dentry, &pathlen,
> > > > > >                                                     &pathbase, 0);
> > > > > > -                       if (IS_ERR(path))
> > > > > > -                               path = NULL;
> > > > > >                         spin_lock(&req->r_old_dentry->d_lock);
> > > > > >                         seq_printf(s, " #%llx/%pd (%s)",
> > > > > 
> > > > > Ditto.
> > > > > 
> > > > > It looks like in newer kernels printf copes with this and outputs
> > > > > "(efault)".  But anything older than 5.2 will crash.
> > > > > 
> > > > > Further, the code looks weird because ceph_mdsc_build_path() doesn't
> > > > > return NULL, but path is tested for NULL in the call to seq_printf().
> > > > > 
> > > > > Why not just follow the same approach as existing mdsc_show()?  It
> > > > > makes it clear that the error is handled and where the NULL pointer
> > > > > comes from.  This kind of "don't handle errors and rely on everything
> > > > > else being able to bail" approach is very fragile and hard to audit.
> > > > > 
> > > > 
> > > > I don't see a problem with having a "free" routine ignore IS_ERR values
> > > > just like it does NULL values. How about I just trim off the other
> > > > deltas in this patch? Something like this?
> > > 
> > > I think it encourages fragile code.  Less so than functions that
> > > return pointer, NULL or IS_ERR pointer, but still.  You yourself
> > > almost fell into one of these traps while editing debugfs.c ;)
> > > 
> > 
> > We'll have to agree to disagree here. Having a free routine ignore
> > ERR_PTR values seems perfectly reasonable to me.
> > 
> > > That said, I won't stand in the way here.  If you trim off everything
> > > else, merge it together with the other patch so that it is introduced
> > > along with the two users.
> > > 
> > 
> > Do you mean that I should merge it with this?
> > 
> >     ceph: initialize base and pathlen variables in async dirops cb's
> > 
> > I'm not sure I see the point in that.
> 
> Yes, because ultimately this is all about those two pr_warn messages.
> path is built just to be printed, base and pathlen are a part of that.
> ceph_mdsc_free_path() didn't need to handle IS_ERR pointers before so
> it should be a single patch.
> 

Ok, done, and cleaned up the changelog. I won't bother re-posting since
it's a fairly trivial squashing of two patches.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Dan Carpenter]

On Mon, Apr 13, 2020 at 09:23:22AM -0400, Jeff Layton wrote:
> > > I don't see a problem with having a "free" routine ignore IS_ERR values
> > > just like it does NULL values. How about I just trim off the other
> > > deltas in this patch? Something like this?
> > 
> > I think it encourages fragile code.  Less so than functions that
> > return pointer, NULL or IS_ERR pointer, but still.  You yourself
> > almost fell into one of these traps while editing debugfs.c ;)
> > 
> 
> We'll have to agree to disagree here. Having a free routine ignore
> ERR_PTR values seems perfectly reasonable to me.

Freeing things which haven't been allocated is a constant source bugs.

err:
	kfree(foo->bar);
	kfree(foo);

Oops...  "foo" wasn't allocated so the first line will crash.  Every
other day someone commits code like that.

regards,
dan carpenter

Re: [PATCH 1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values

[Jeff Layton]

On Mon, 2020-04-13 at 22:41 +0300, Dan Carpenter wrote:
> On Mon, Apr 13, 2020 at 09:23:22AM -0400, Jeff Layton wrote:
> > > > I don't see a problem with having a "free" routine ignore IS_ERR values
> > > > just like it does NULL values. How about I just trim off the other
> > > > deltas in this patch? Something like this?
> > > 
> > > I think it encourages fragile code.  Less so than functions that
> > > return pointer, NULL or IS_ERR pointer, but still.  You yourself
> > > almost fell into one of these traps while editing debugfs.c ;)
> > > 
> > 
> > We'll have to agree to disagree here. Having a free routine ignore
> > ERR_PTR values seems perfectly reasonable to me.
> 
> Freeing things which haven't been allocated is a constant source bugs.
> 
> err:
> 	kfree(foo->bar);
> 	kfree(foo);
> 
> Oops...  "foo" wasn't allocated so the first line will crash.  Every
> other day someone commits code like that.
> 

It's not clear to me whether you're advocating for Ilya's approach or
mine (neither? both?). Which approach do you think is best?

FWIW, my rationale for doing it this way is that the "allocator" for
ceph_mdsc_free_path is ceph_mdsc_build_path. That routine returns an
ERR_PTR value on failure, not a NULL pointer, so it makes sense to me
to have the free routine accept and ignore those values.

I don't quite follow the rationale that that encourages fragile code.
-- 
Jeff Layton <jlayton@kernel.org>