On Mon, 2018-10-15 at 20:37 +0800, Yan, Zheng wrote:
> > On Oct 15, 2018, at 19:29, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Mon, 2018-10-15 at 09:58 +0800, Yan, Zheng wrote:
> > > > On Oct 12, 2018, at 18:56, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, 2018-10-12 at 10:39 +0800, Yan, Zheng wrote:
> > > > > > 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;
> > > > >
> > > >
> > > > Why do we need to reset r_dentry? I don't see where it gets used after
> > > > ceph_fill_trace returns, so I gather we're just resetting it to clean up
> > > > the references? Your proposed fix looks like it would fix that bug. It
> > > > will mean that the values in the req will change but maybe that's ok.
> > > >
> > >
> > > It’s used by ceph_finish_lookup
> > >
> >
> > In that case, it might be bad idea to overwrite the value of r_dentry
> > with the r_old_dentry pointer?
>
> ceph_finish_lookup() is not used in the rename case ;)
>
>
Ok, but still...I worry about this sort of special-casing, as it can
lead to people making incorrect assumptions later. I think it would be
best to just take a dentry ref for "dn" and just put it before it goes
out of scope.
At the very least, if you are going to override r_dentry like this,
please add a comment explaining why it's safe to do this in the rename case.
> >
> > >
> > > > Another idea might be to just keep an extra reference to "dn", and not
> > > > reset r_dentry and r_old_dentry? It's a bit more code, but it might be a
> > > > cleaner fix, long-term.
> > >
> > > I will check, Thanks
> > >
> > > Yan, Zheng
> > >
> > > > --
> > > > Jeff Layton <jlayton@xxxxxxxxxx>
> > >
> > >
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
> >
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
