| Message ID | 20200408142125.52908-2-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix error handling bugs in async dirops callbacks | expand |
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
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)); }
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
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,
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
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.
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
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.
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)); }
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(-)