| Message ID | 20190416125255.10212-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: use d_lock to protect dentry name in ceph_fill_trace | expand |
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 >
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 > >
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>
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>
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.
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
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(-)