[1/2] ceph: have ceph_mdsc_free_path ignore ERR_PTR values
diff mbox series

Message ID 20200408142125.52908-2-jlayton@kernel.org
State New
Headers show
Series
  • ceph: fix error handling bugs in async dirops callbacks
Related show

Commit Message

Jeff Layton April 8, 2020, 2:21 p.m. UTC
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(-)

Comments

Ilya Dryomov April 13, 2020, 8:09 a.m. UTC | #1
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
Jeff Layton April 13, 2020, 11:15 a.m. UTC | #2
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));
 }
Ilya Dryomov April 13, 2020, 12:35 p.m. UTC | #3
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
Jeff Layton April 13, 2020, 1:23 p.m. UTC | #4
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,
Ilya Dryomov April 13, 2020, 3:48 p.m. UTC | #5
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
Jeff Layton April 13, 2020, 4:03 p.m. UTC | #6
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.
Dan Carpenter April 13, 2020, 7:41 p.m. UTC | #7
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
Jeff Layton April 14, 2020, 10:43 a.m. UTC | #8
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.

Patch
diff mbox series

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