ceph: use d_lock to protect dentry name in ceph_fill_trace
diff mbox series

Message ID 20190416125255.10212-1-jlayton@kernel.org
State New
Headers show
Series
  • ceph: use d_lock to protect dentry name in ceph_fill_trace
Related show

Commit Message

Jeff Layton April 16, 2019, 12:52 p.m. UTC
It's not safe to run strncmp on the dentry name locklessly like this,
as it could end up being altered via rename. Add a helper function
that takes the d_lock in order to do the comparison.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/inode.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Yan, Zheng April 16, 2019, 1:31 p.m. UTC | #1
On Tue, Apr 16, 2019 at 8:54 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> It's not safe to run strncmp on the dentry name locklessly like this,
> as it could end up being altered via rename. Add a helper function
> that takes the d_lock in order to do the comparison.
>

snapdir can not be renamed ;)

Yan, Zheng

> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/inode.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index b33ba16f7ae8..9c93c108180d 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1163,6 +1163,18 @@ static int splice_dentry(struct dentry **pdn, struct inode *in)
>         return 0;
>  }
>
> +static bool
> +dentry_is_snapdir(struct ceph_fs_client *fsc, struct dentry *dentry)
> +{
> +       int ret;
> +
> +       spin_lock(&dentry->d_lock);
> +       ret = strncmp(dentry->d_name.name, fsc->mount_options->snapdir_name,
> +                     dentry->d_name.len);
> +       spin_unlock(&dentry->d_lock);
> +       return !ret;
> +}
> +
>  /*
>   * Incorporate results into the local cache.  This is either just
>   * one inode, or a directory, dentry, and possibly linked-to inode (e.g.,
> @@ -1285,9 +1297,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>         if (rinfo->head->is_dentry &&
>              !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
>             test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
> -           (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
> -                                              fsc->mount_options->snapdir_name,
> -                                              req->r_dentry->d_name.len))) {
> +           (rinfo->head->is_target ||
> +            !dentry_is_snapdir(fsc, req->r_dentry))) {
>                 /*
>                  * lookup link rename   : null -> possibly existing inode
>                  * mknod symlink mkdir  : null -> new inode
> --
> 2.20.1
>
Jeff Layton April 16, 2019, 1:51 p.m. UTC | #2
On Tue, Apr 16, 2019 at 9:31 AM Yan, Zheng <ukernel@gmail.com> wrote:
>
> On Tue, Apr 16, 2019 at 8:54 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > It's not safe to run strncmp on the dentry name locklessly like this,
> > as it could end up being altered via rename. Add a helper function
> > that takes the d_lock in order to do the comparison.
> >
>
> snapdir can not be renamed ;)
>
> Yan, Zheng
>

Do we know that r_dentry refers to a snapdir at this point?

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/inode.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index b33ba16f7ae8..9c93c108180d 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -1163,6 +1163,18 @@ static int splice_dentry(struct dentry **pdn, struct inode *in)
> >         return 0;
> >  }
> >
> > +static bool
> > +dentry_is_snapdir(struct ceph_fs_client *fsc, struct dentry *dentry)
> > +{
> > +       int ret;
> > +
> > +       spin_lock(&dentry->d_lock);
> > +       ret = strncmp(dentry->d_name.name, fsc->mount_options->snapdir_name,
> > +                     dentry->d_name.len);
> > +       spin_unlock(&dentry->d_lock);
> > +       return !ret;
> > +}
> > +
> >  /*
> >   * Incorporate results into the local cache.  This is either just
> >   * one inode, or a directory, dentry, and possibly linked-to inode (e.g.,
> > @@ -1285,9 +1297,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> >         if (rinfo->head->is_dentry &&
> >              !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
> >             test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
> > -           (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
> > -                                              fsc->mount_options->snapdir_name,
> > -                                              req->r_dentry->d_name.len))) {
> > +           (rinfo->head->is_target ||
> > +            !dentry_is_snapdir(fsc, req->r_dentry))) {
> >                 /*
> >                  * lookup link rename   : null -> possibly existing inode
> >                  * mknod symlink mkdir  : null -> new inode
> > --
> > 2.20.1
> >
Yan, Zheng April 16, 2019, 2:09 p.m. UTC | #3
On Tue, Apr 16, 2019 at 9:52 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, Apr 16, 2019 at 9:31 AM Yan, Zheng <ukernel@gmail.com> wrote:
> >
> > On Tue, Apr 16, 2019 at 8:54 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > It's not safe to run strncmp on the dentry name locklessly like this,
> > > as it could end up being altered via rename. Add a helper function
> > > that takes the d_lock in order to do the comparison.
> > >
> >
> > snapdir can not be renamed ;)
> >
> > Yan, Zheng
> >
>
> Do we know that r_dentry refers to a snapdir at this point?
>

my fault. thanks for explanation



> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/inode.c | 17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index b33ba16f7ae8..9c93c108180d 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -1163,6 +1163,18 @@ static int splice_dentry(struct dentry **pdn, struct inode *in)
> > >         return 0;
> > >  }
> > >
> > > +static bool
> > > +dentry_is_snapdir(struct ceph_fs_client *fsc, struct dentry *dentry)
> > > +{
> > > +       int ret;
> > > +
> > > +       spin_lock(&dentry->d_lock);
> > > +       ret = strncmp(dentry->d_name.name, fsc->mount_options->snapdir_name,
> > > +                     dentry->d_name.len);
> > > +       spin_unlock(&dentry->d_lock);
> > > +       return !ret;
> > > +}
> > > +
> > >  /*
> > >   * Incorporate results into the local cache.  This is either just
> > >   * one inode, or a directory, dentry, and possibly linked-to inode (e.g.,
> > > @@ -1285,9 +1297,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> > >         if (rinfo->head->is_dentry &&
> > >              !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
> > >             test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
> > > -           (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
> > > -                                              fsc->mount_options->snapdir_name,
> > > -                                              req->r_dentry->d_name.len))) {
> > > +           (rinfo->head->is_target ||
> > > +            !dentry_is_snapdir(fsc, req->r_dentry))) {
> > >                 /*
> > >                  * lookup link rename   : null -> possibly existing inode
> > >                  * mknod symlink mkdir  : null -> new inode
> > > --
> > > 2.20.1
> > >
>
>
>
> --
> Jeff Layton <jlayton@kernel.org>
Yan, Zheng April 17, 2019, 1:01 p.m. UTC | #4
On Tue, Apr 16, 2019 at 8:54 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> It's not safe to run strncmp on the dentry name locklessly like this,
> as it could end up being altered via rename. Add a helper function
> that takes the d_lock in order to do the comparison.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/inode.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index b33ba16f7ae8..9c93c108180d 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1163,6 +1163,18 @@ static int splice_dentry(struct dentry **pdn, struct inode *in)
>         return 0;
>  }
>
> +static bool
> +dentry_is_snapdir(struct ceph_fs_client *fsc, struct dentry *dentry)
> +{
> +       int ret;
> +
> +       spin_lock(&dentry->d_lock);
> +       ret = strncmp(dentry->d_name.name, fsc->mount_options->snapdir_name,
> +                     dentry->d_name.len);
> +       spin_unlock(&dentry->d_lock);
> +       return !ret;
> +}
> +
>  /*
>   * Incorporate results into the local cache.  This is either just
>   * one inode, or a directory, dentry, and possibly linked-to inode (e.g.,
> @@ -1285,9 +1297,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
>         if (rinfo->head->is_dentry &&
>              !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
>             test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
> -           (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
> -                                              fsc->mount_options->snapdir_name,
> -                                              req->r_dentry->d_name.len))) {
> +           (rinfo->head->is_target ||
> +            !dentry_is_snapdir(fsc, req->r_dentry))) {
>                 /*
>                  * lookup link rename   : null -> possibly existing inode
>                  * mknod symlink mkdir  : null -> new inode
> --
> 2.20.1
>

Reviewed-by: "Yan, Zheng" <zyan@redhat.com>
Jeff Layton April 17, 2019, 2:56 p.m. UTC | #5
On Wed, Apr 17, 2019 at 9:02 AM Yan, Zheng <ukernel@gmail.com> wrote:
>
> On Tue, Apr 16, 2019 at 8:54 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > It's not safe to run strncmp on the dentry name locklessly like this,
> > as it could end up being altered via rename. Add a helper function
> > that takes the d_lock in order to do the comparison.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/inode.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index b33ba16f7ae8..9c93c108180d 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -1163,6 +1163,18 @@ static int splice_dentry(struct dentry **pdn, struct inode *in)
> >         return 0;
> >  }
> >
> > +static bool
> > +dentry_is_snapdir(struct ceph_fs_client *fsc, struct dentry *dentry)
> > +{
> > +       int ret;
> > +
> > +       spin_lock(&dentry->d_lock);
> > +       ret = strncmp(dentry->d_name.name, fsc->mount_options->snapdir_name,
> > +                     dentry->d_name.len);
> > +       spin_unlock(&dentry->d_lock);
> > +       return !ret;
> > +}
> > +
> >  /*
> >   * Incorporate results into the local cache.  This is either just
> >   * one inode, or a directory, dentry, and possibly linked-to inode (e.g.,
> > @@ -1285,9 +1297,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> >         if (rinfo->head->is_dentry &&
> >              !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
> >             test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
> > -           (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
> > -                                              fsc->mount_options->snapdir_name,
> > -                                              req->r_dentry->d_name.len))) {
> > +           (rinfo->head->is_target ||
> > +            !dentry_is_snapdir(fsc, req->r_dentry))) {
> >                 /*
> >                  * lookup link rename   : null -> possibly existing inode
> >                  * mknod symlink mkdir  : null -> new inode
> > --
> > 2.20.1
> >
>
> Reviewed-by: "Yan, Zheng" <zyan@redhat.com>

Now that I look again, I think this may not be needed after all. We
know that we have the parent locked here, so no d_move should be
occurring. I think I'm going to drop this patch for now, but I'll see
about fixing things up when we don't have the parent locked.

Patch
diff mbox series

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index b33ba16f7ae8..9c93c108180d 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1163,6 +1163,18 @@  static int splice_dentry(struct dentry **pdn, struct inode *in)
 	return 0;
 }
 
+static bool
+dentry_is_snapdir(struct ceph_fs_client *fsc, struct dentry *dentry)
+{
+	int ret;
+
+	spin_lock(&dentry->d_lock);
+	ret = strncmp(dentry->d_name.name, fsc->mount_options->snapdir_name,
+		      dentry->d_name.len);
+	spin_unlock(&dentry->d_lock);
+	return !ret;
+}
+
 /*
  * Incorporate results into the local cache.  This is either just
  * one inode, or a directory, dentry, and possibly linked-to inode (e.g.,
@@ -1285,9 +1297,8 @@  int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 	if (rinfo->head->is_dentry &&
             !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
 	    test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
-	    (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
-					       fsc->mount_options->snapdir_name,
-					       req->r_dentry->d_name.len))) {
+	    (rinfo->head->is_target ||
+	     !dentry_is_snapdir(fsc, req->r_dentry))) {
 		/*
 		 * lookup link rename   : null -> possibly existing inode
 		 * mknod symlink mkdir  : null -> new inode