On Thu, 2020-04-16 at 09:08 +0200, Ilya Dryomov wrote:
> On Thu, Apr 16, 2020 at 2:04 AM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > 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?
--
Jeff Layton <jlayton@xxxxxxxxxx>
