Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201029190716.70481-1-dai.ngo@oracle.com> <20201029190716.70481-2-dai.ngo@oracle.com>
 <CAN-5tyFnTSuMivnBPD9Aur+KDxX8fCOuSaF7qGKe6bFB7roK6Q@mail.gmail.com>
 <20210220010903.GE5357@fieldses.org> <69ea46ff-80d1-acfa-22a5-3d1b6230728f@oracle.com>
 <20210220032057.GA25183@fieldses.org>
In-Reply-To: <20210220032057.GA25183@fieldses.org>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Sat, 20 Feb 2021 09:08:43 -0500
Message-ID: <CAN-5tyHq2NcQRbx01cSyJob=72MDUnwjK_t6GiDjTc3twbnvwA@mail.gmail.com>
Subject: Re: [PATCH 1/2] NFSD: Fix use-after-free warning when doing
 inter-server copy
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     Dai Ngo <dai.ngo@oracle.com>, linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 19, 2021 at 10:21 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Feb 19, 2021 at 05:31:58PM -0800, dai.ngo@oracle.com wrote:
> > If this is the cause why we don't drop the mount after the copy
> > then I can restore the patch and look into this problem. Unfortunately,
> > all my test machines are down for maintenance until Sunday/Monday.
>
> I think we can take some time to figure out what's actually going on
> here before reverting anything.

Yes I agree. We need to fix the use-after-free and also make sure that
reference will go away. I'm assuming to reproduce the use-after-free I
need to run with kazan, is that what you did Dai?

>
> --b.
>
> >
> > -Dai
> >
> > On 2/19/21 5:09 PM, J. Bruce Fields wrote:
> > >Dai, do you have a copy of the original use-after-free warning?
> > >
> > >--b.
> > >
> > >On Fri, Feb 19, 2021 at 07:18:53PM -0500, Olga Kornievskaia wrote:
> > >>Hi Dai (Bruce),
> > >>
> > >>This patch is what broke the mount that's now left behind between the
> > >>source server and the destination server. We are no longer dropping
> > >>the necessary reference on the mount to go away. I haven't been paying
> > >>as much attention as I should have been to the changes. The original
> > >>code called fput(src) so a simple refcount of the file. Then things
> > >>got complicated and moved to nfsd_file_put(). So I don't understand
> > >>complexity. But we need to do some kind of put to decrement the needed
> > >>reference on the superblock. Bruce any ideas? Can we go back to
> > >>fput()?
> > >>
> > >>On Thu, Oct 29, 2020 at 3:08 PM Dai Ngo <dai.ngo@oracle.com> wrote:
> > >>>The source file nfsd_file is not constructed the same as other
> > >>>nfsd_file's via nfsd_file_alloc. nfsd_file_put should not be
> > >>>called to free the object; nfsd_file_put is not the inverse of
> > >>>kzalloc, instead kfree is called by nfsd4_do_async_copy when done.
> > >>>
> > >>>Fixes: ce0887ac96d3 ("NFSD add nfs4 inter ssc to nfsd4_copy")
> > >>>Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > >>>---
> > >>>  fs/nfsd/nfs4proc.c | 2 +-
> > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > >>>index ad2fa1a8e7ad..9c43cad7e408 100644
> > >>>--- a/fs/nfsd/nfs4proc.c
> > >>>+++ b/fs/nfsd/nfs4proc.c
> > >>>@@ -1299,7 +1299,7 @@ nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct nfsd_file *src,
> > >>>                         struct nfsd_file *dst)
> > >>>  {
> > >>>         nfs42_ssc_close(src->nf_file);
> > >>>-       nfsd_file_put(src);
> > >>>+       /* 'src' is freed by nfsd4_do_async_copy */
> > >>>         nfsd_file_put(dst);
> > >>>         mntput(ss_mnt);
> > >>>  }
> > >>>--
> > >>>2.20.1.1226.g1595ea5.dirty
> > >>>