| Message ID | 20200415133929.234033-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ceph: fix potential bad pointer deref in async dirops cb's | expand |
On Thu, Apr 16, 2020 at 2:04 AM Jeff Layton <jlayton@kernel.org> wrote: > > The new async dirops callback routines can pass ERR_PTR values to > ceph_mdsc_free_path, which could cause an oops. > > Given that ceph_mdsc_build_path returns ERR_PTR values, it makes sense > to just have ceph_mdsc_free_path ignore them. Also, clean up the error > handling a bit in mdsc_show, and 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/debugfs.c | 8 ++------ > fs/ceph/dir.c | 4 ++-- > fs/ceph/file.c | 4 ++-- > fs/ceph/mds_client.h | 2 +- > 4 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c > index eebbce7c3b0c..3baec3a896ee 100644 > --- a/fs/ceph/debugfs.c > +++ b/fs/ceph/debugfs.c > @@ -83,13 +83,11 @@ 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)), > req->r_dentry, > - path ? path : ""); > + IS_ERR(path) ? "" : path); > spin_unlock(&req->r_dentry->d_lock); > ceph_mdsc_free_path(path, pathlen); > } else if (req->r_path1) { > @@ -102,14 +100,12 @@ 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 ? > ceph_ino(req->r_old_dentry_dir) : 0, > req->r_old_dentry, > - path ? path : ""); > + IS_ERR(path) ? "" : path); > spin_unlock(&req->r_old_dentry->d_lock); > ceph_mdsc_free_path(path, pathlen); > } else if (req->r_path2 && req->r_op != CEPH_MDS_OP_SYMLINK) { > 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); > > 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)); > } Hi Jeff, Following our discussion, I staged v1 (i.e. no debugfs.c changes) in commit 2a575f138d003fff0f4930b5cfae4a1c46343b8f on Monday. I see you force pushed testing, so perhaps you missed that. Please be careful when force pushing. Thanks, Ilya
On Thu, 2020-04-16 at 09:08 +0200, Ilya Dryomov wrote: > On Thu, Apr 16, 2020 at 2:04 AM Jeff Layton <jlayton@kernel.org> wrote: > > The new async dirops callback routines can pass ERR_PTR values to > > ceph_mdsc_free_path, which could cause an oops. > > > > Given that ceph_mdsc_build_path returns ERR_PTR values, it makes sense > > to just have ceph_mdsc_free_path ignore them. Also, clean up the error > > handling a bit in mdsc_show, and 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/debugfs.c | 8 ++------ > > fs/ceph/dir.c | 4 ++-- > > fs/ceph/file.c | 4 ++-- > > fs/ceph/mds_client.h | 2 +- > > 4 files changed, 7 insertions(+), 11 deletions(-) > > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c > > index eebbce7c3b0c..3baec3a896ee 100644 > > --- a/fs/ceph/debugfs.c > > +++ b/fs/ceph/debugfs.c > > @@ -83,13 +83,11 @@ 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)), > > req->r_dentry, > > - path ? path : ""); > > + IS_ERR(path) ? "" : path); > > spin_unlock(&req->r_dentry->d_lock); > > ceph_mdsc_free_path(path, pathlen); > > } else if (req->r_path1) { > > @@ -102,14 +100,12 @@ 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 ? > > ceph_ino(req->r_old_dentry_dir) : 0, > > req->r_old_dentry, > > - path ? path : ""); > > + IS_ERR(path) ? "" : path); > > spin_unlock(&req->r_old_dentry->d_lock); > > ceph_mdsc_free_path(path, pathlen); > > } else if (req->r_path2 && req->r_op != CEPH_MDS_OP_SYMLINK) { > > 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); > > > > 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)); > > } > > Hi Jeff, > > Following our discussion, I staged v1 (i.e. no debugfs.c changes) in > commit 2a575f138d003fff0f4930b5cfae4a1c46343b8f on Monday. I see you > force pushed testing, so perhaps you missed that. > > Please be careful when force pushing. My bad. It should be fixed now. This is one of the reasons I'm not a fan of sharing a git tree like we are. It's like all the "fun" of the bad, old CVS days. Part of the problem is that I don't get any clear notification when you move patches from testing into master. Perhaps we should just make you maintainer for cephfs as well, which would keep it to one person merging patches?
On Thu, Apr 16, 2020 at 12:50 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2020-04-16 at 09:08 +0200, Ilya Dryomov wrote: > > On Thu, Apr 16, 2020 at 2:04 AM Jeff Layton <jlayton@kernel.org> wrote: > > > The new async dirops callback routines can pass ERR_PTR values to > > > ceph_mdsc_free_path, which could cause an oops. > > > > > > Given that ceph_mdsc_build_path returns ERR_PTR values, it makes sense > > > to just have ceph_mdsc_free_path ignore them. Also, clean up the error > > > handling a bit in mdsc_show, and 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/debugfs.c | 8 ++------ > > > fs/ceph/dir.c | 4 ++-- > > > fs/ceph/file.c | 4 ++-- > > > fs/ceph/mds_client.h | 2 +- > > > 4 files changed, 7 insertions(+), 11 deletions(-) > > > > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c > > > index eebbce7c3b0c..3baec3a896ee 100644 > > > --- a/fs/ceph/debugfs.c > > > +++ b/fs/ceph/debugfs.c > > > @@ -83,13 +83,11 @@ 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)), > > > req->r_dentry, > > > - path ? path : ""); > > > + IS_ERR(path) ? "" : path); > > > spin_unlock(&req->r_dentry->d_lock); > > > ceph_mdsc_free_path(path, pathlen); > > > } else if (req->r_path1) { > > > @@ -102,14 +100,12 @@ 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 ? > > > ceph_ino(req->r_old_dentry_dir) : 0, > > > req->r_old_dentry, > > > - path ? path : ""); > > > + IS_ERR(path) ? "" : path); > > > spin_unlock(&req->r_old_dentry->d_lock); > > > ceph_mdsc_free_path(path, pathlen); > > > } else if (req->r_path2 && req->r_op != CEPH_MDS_OP_SYMLINK) { > > > 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); > > > > > > 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)); > > > } > > > > Hi Jeff, > > > > Following our discussion, I staged v1 (i.e. no debugfs.c changes) in > > commit 2a575f138d003fff0f4930b5cfae4a1c46343b8f on Monday. I see you > > force pushed testing, so perhaps you missed that. > > > > Please be careful when force pushing. > > My bad. It should be fixed now. > > This is one of the reasons I'm not a fan of sharing a git tree like we > are. It's like all the "fun" of the bad, old CVS days. Part of the > problem is that I don't get any clear notification when you move patches > from testing into master. Sorry, I should have replied in the thread. I didn't because we agreed on pushing this fix for -rc2 and you said you wouldn't bother reposting, so I thought you were done with it. > > Perhaps we should just make you maintainer for cephfs as well, which > would keep it to one person merging patches? I don't see a problem with sharing testing. Just don't force push by default. Try a regular push first and examine the history if it fails. I never rebase without a good reason, so it should be fairly clear -- usually either a master update or a rebase to a late -rc). If you ever force push, use --force-with-lease. Ultimately, though, it's just a testing branch. If one of us screws it, it can always be reset. Thanks, Ilya
On Thu, 2020-04-16 at 14:14 +0200, Ilya Dryomov wrote: > On Thu, Apr 16, 2020 at 12:50 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2020-04-16 at 09:08 +0200, Ilya Dryomov wrote: > > > On Thu, Apr 16, 2020 at 2:04 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > The new async dirops callback routines can pass ERR_PTR values to > > > > ceph_mdsc_free_path, which could cause an oops. > > > > > > > > Given that ceph_mdsc_build_path returns ERR_PTR values, it makes sense > > > > to just have ceph_mdsc_free_path ignore them. Also, clean up the error > > > > handling a bit in mdsc_show, and 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/debugfs.c | 8 ++------ > > > > fs/ceph/dir.c | 4 ++-- > > > > fs/ceph/file.c | 4 ++-- > > > > fs/ceph/mds_client.h | 2 +- > > > > 4 files changed, 7 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c > > > > index eebbce7c3b0c..3baec3a896ee 100644 > > > > --- a/fs/ceph/debugfs.c > > > > +++ b/fs/ceph/debugfs.c > > > > @@ -83,13 +83,11 @@ 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)), > > > > req->r_dentry, > > > > - path ? path : ""); > > > > + IS_ERR(path) ? "" : path); > > > > spin_unlock(&req->r_dentry->d_lock); > > > > ceph_mdsc_free_path(path, pathlen); > > > > } else if (req->r_path1) { > > > > @@ -102,14 +100,12 @@ 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 ? > > > > ceph_ino(req->r_old_dentry_dir) : 0, > > > > req->r_old_dentry, > > > > - path ? path : ""); > > > > + IS_ERR(path) ? "" : path); > > > > spin_unlock(&req->r_old_dentry->d_lock); > > > > ceph_mdsc_free_path(path, pathlen); > > > > } else if (req->r_path2 && req->r_op != CEPH_MDS_OP_SYMLINK) { > > > > 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); > > > > > > > > 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)); > > > > } > > > > > > Hi Jeff, > > > > > > Following our discussion, I staged v1 (i.e. no debugfs.c changes) in > > > commit 2a575f138d003fff0f4930b5cfae4a1c46343b8f on Monday. I see you > > > force pushed testing, so perhaps you missed that. > > > > > > Please be careful when force pushing. > > > > My bad. It should be fixed now. > > > > This is one of the reasons I'm not a fan of sharing a git tree like we > > are. It's like all the "fun" of the bad, old CVS days. Part of the > > problem is that I don't get any clear notification when you move patches > > from testing into master. > > Sorry, I should have replied in the thread. I didn't because > we agreed on pushing this fix for -rc2 and you said you wouldn't > bother reposting, so I thought you were done with it. > > > Perhaps we should just make you maintainer for cephfs as well, which > > would keep it to one person merging patches? > > I don't see a problem with sharing testing. Just don't force > push by default. Try a regular push first and examine the history > if it fails. I never rebase without a good reason, so it should > be fairly clear -- usually either a master update or a rebase to a > late -rc). If you ever force push, use --force-with-lease. > > Ultimately, though, it's just a testing branch. If one of us screws > it, it can always be reset. > I generally use --force-with-lease when I do have to force push but I must have done a bare --force push accidentally. Sorry about that!
diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c index eebbce7c3b0c..3baec3a896ee 100644 --- a/fs/ceph/debugfs.c +++ b/fs/ceph/debugfs.c @@ -83,13 +83,11 @@ 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)), req->r_dentry, - path ? path : ""); + IS_ERR(path) ? "" : path); spin_unlock(&req->r_dentry->d_lock); ceph_mdsc_free_path(path, pathlen); } else if (req->r_path1) { @@ -102,14 +100,12 @@ 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 ? ceph_ino(req->r_old_dentry_dir) : 0, req->r_old_dentry, - path ? path : ""); + IS_ERR(path) ? "" : path); spin_unlock(&req->r_old_dentry->d_lock); ceph_mdsc_free_path(path, pathlen); } else if (req->r_path2 && req->r_op != CEPH_MDS_OP_SYMLINK) { 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); 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)); }
The new async dirops callback routines can pass ERR_PTR values to ceph_mdsc_free_path, which could cause an oops. Given that ceph_mdsc_build_path returns ERR_PTR values, it makes sense to just have ceph_mdsc_free_path ignore them. Also, clean up the error handling a bit in mdsc_show, and 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/debugfs.c | 8 ++------ fs/ceph/dir.c | 4 ++-- fs/ceph/file.c | 4 ++-- fs/ceph/mds_client.h | 2 +- 4 files changed, 7 insertions(+), 11 deletions(-)