Re: [deadlock or dead code] ceph_encode_dentry_release() misuse of dget()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>





[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux