> On Oct 11, 2018, at 22:49, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Fri, 2018-09-28 at 17:43 +0800, Yan, Zheng wrote:
>> This reverts commit 8b8f53af1ed9df88a4c0fbfdf3db58f62060edf3.
>>
>> splice_dentry() is used by three places. For two places, req->r_dentry
>> is passed to splice_dentry(). In the case of error, req->r_dentry does
>> not get updated. So splice_dentry() should not drop reference.
>>
>> Cc: stable@xxxxxxxxxxxxxxx #4.18
>> Signed-off-by: "Yan, Zheng" <zyan@xxxxxxxxxx>
>> ---
>> fs/ceph/inode.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index c6bbb7aa99e4..375924b2bc86 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -1140,7 +1140,6 @@ static struct dentry *splice_dentry(struct dentry *dn, struct inode *in)
>> if (IS_ERR(realdn)) {
>> pr_err("splice_dentry error %ld %p inode %p ino %llx.%llx\n",
>> PTR_ERR(realdn), dn, in, ceph_vinop(in));
>> - dput(dn);
>> dn = realdn; /* note realdn contains the error */
>> goto out;
>> } else if (realdn) {
>
> This might be ok, buI have some real concerns about splice_dentry and
> its callers -- particularly ceph_fill_trace:
>
> We hold a reference to dn on entry to splice_dentry. We then call
> d_splice_alias and get back an error, and now we don't put the old
> dentry.
>
> Fine -- we have to then expect the caller to do it. Unfortunately, the
> callers in ceph_fill_trace do this:
>
> dn = splice_dentry(dn, in);
> if (IS_ERR(dn)) {
> err = PTR_ERR(dn);
> goto done;
> }
> req->r_dentry = dn; /* may have spliced */
>
> The old value of dn gets clobbered once that comes back with an ERR_PTR.
> I guess we could claim that r_dentry will still be set to the old value
> at that point and that it would get cleaned up when it gets cleaned up.
>
> But...I see this higher up in ceph_fill_trace at the end of the
> CEPH_MFS_OP_RENAME condition block:
>
> dn = req->r_old_dentry; /* use old_dentry */
>
> So now I'm worried about the case where the splice succeeds. ISTM that
> "dn" can represent either r_dentry or r_old_dentry at the point where
> splice_dentry gets called, but we only ever reset the value of r_dentry
> there.
>
> If dn == r_old_dentry at the time that splice_dentry is called, and then
> that succeeds, we'll end up leaking the reference to r_dentry and then
> doing an overput on r_old_dentry.
>
Good catch
> I think it might help to establish clear "ownership" of the dentry
> references throughout that function. Consider zeroing out r_dentry and
> r_old_dentry at the time that you set the local variables? That might
> make this whole thing less fragile.
Before the function return, we need to set req->r_dentry again. This will introduce duplicated code because there are several ‘goto’ in the function. How about following change.
- dn = req->r_old_dentry; /* use old_dentry */
+ /* swap r_dentry and r_old_dentry */
+ req->r_dentry = req->r_old_dentry;
+ req->r_old_dentry = dn;
+ dn = req->r_dentry;
Regards
Yan, Zheng
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
