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

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

[Al Viro]

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.

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...

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

[Jeff Layton]

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@kernel.org>

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

[Al Viro]

On Thu, Nov 16, 2023 at 07:50:03AM -0500, Jeff Layton wrote:

> > 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?

Up to you...  AFAICS, it had been dead code since ca6c8ae0f793 "ceph: pass
parent inode info to ceph_encode_dentry_release if we have it".  In other
words, that "if we have it" had already been true at that point.  Prior
to that commit dget() in there had been unconditional (and really a deadlock
fodder); making it conditional had made it actually unreachable and that
fixed the actual bug.

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

[Jeff Layton]

On Thu, 2023-11-16 at 16:28 +0000, Al Viro wrote:
> On Thu, Nov 16, 2023 at 07:50:03AM -0500, Jeff Layton wrote:
> 
> > > 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?
> 
> Up to you...  AFAICS, it had been dead code since ca6c8ae0f793 "ceph: pass
> parent inode info to ceph_encode_dentry_release if we have it".  In other
> words, that "if we have it" had already been true at that point.  Prior
> to that commit dget() in there had been unconditional (and really a deadlock
> fodder); making it conditional had made it actually unreachable and that
> fixed the actual bug.

That makes sense.

Xiubo, would you mind spinning up a patch for this? You're probably in
the best position to make sure it gets tested these days.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>

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

[Xiubo Li]

On 11/17/23 00:50, Jeff Layton wrote:
> On Thu, 2023-11-16 at 16:28 +0000, Al Viro wrote:
>> On Thu, Nov 16, 2023 at 07:50:03AM -0500, Jeff Layton wrote:
>>
>>>> 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?
>> Up to you...  AFAICS, it had been dead code since ca6c8ae0f793 "ceph: pass
>> parent inode info to ceph_encode_dentry_release if we have it".  In other
>> words, that "if we have it" had already been true at that point.  Prior
>> to that commit dget() in there had been unconditional (and really a deadlock
>> fodder); making it conditional had made it actually unreachable and that
>> fixed the actual bug.
> That makes sense.
>
> Xiubo, would you mind spinning up a patch for this? You're probably in
> the best position to make sure it gets tested these days.

Hi Jeff, Al

Sure, I will fix this. Thanks very much.

- Xiubo


> Thanks!