On Wed, Apr 17, 2019 at 9:02 AM Yan, Zheng <ukernel@xxxxxxxxx> wrote:
>
> On Tue, Apr 16, 2019 at 8:54 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
