On Thu, Apr 16, 2020 at 12:50 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@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.
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
