On Thu, 2023-11-16 at 08:19 +0000, Al Viro wrote:
> This
> spin_lock(&dentry->d_lock);
> if (di->lease_session && di->lease_session->s_mds == mds)
> force = 1;
> if (!dir) {
> parent = dget(dentry->d_parent);
> dir = d_inode(parent);
> }
> spin_unlock(&dentry->d_lock);
> has a problem if we ever get called with dir == NULL.
>
Ouch, ok.
> The thing is, dget(dentry->d_parent) will spin if dentry->d_parent->d_lock
> is held. IOW, the whole thing is an equivalent of
> spin_lock(&dentry->d_lock);
> spin_lock(&dentry->d_parent->d_lock);
> which takes them in the wrong order.
>
> Said that, I'm not sure it ever gets called with dir == NULL;
> we have two callers -
> if (req->r_dentry_drop) {
> ret = ceph_encode_dentry_release(&p, req->r_dentry,
> req->r_parent, mds, req->r_dentry_drop,
> req->r_dentry_unless);
> if (ret < 0)
> goto out_err;
> releases += ret;
> }
> and
> if (req->r_old_dentry_drop) {
> ret = ceph_encode_dentry_release(&p, req->r_old_dentry,
> req->r_old_dentry_dir, mds,
> req->r_old_dentry_drop,
> req->r_old_dentry_unless);
> if (ret < 0)
> goto out_err;
> releases += ret;
> }
> Now, ->r_dentry_drop is set in ceph_mknod(), ceph_symlink(), ceph_mkdir(),
> ceph_link(), ceph_unlink(), all with
> req->r_parent = dir;
> ihold(dir);
> ceph_rename(), with
> req->r_parent = new_dir;
> ihold(new_dir);
> and ceph_atomic_open(), with
> req->r_parent = dir;
> if (req->r_op == CEPH_MDS_OP_CREATE)
> req->r_mnt_idmap = mnt_idmap_get(idmap);
> ihold(dir);
> All of that will oops if ->r_parent set to NULL.
> ->r_old_dentry_drop() is set in ceph_rename(), with
> struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(old_dir->i_sb);
> ...
> req->r_old_dentry_dir = old_dir;
> which also can't manage to leave ->r_old_dentry_dir set to NULL.
>
> Am I missing something subtle here? Looks like that dget() is never
> reached...
No, I think you're correct. That looks like dead code to me too.
Probably we can just remove that "if (!dir)" condition altogether.
Did you want to send a patch, or would you rather Xiubo or I do it?
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
