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>
In-Reply-To: <20201029190716.70481-2-dai.ngo@oracle.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Fri, 19 Feb 2021 19:18:53 -0500
Message-ID: <CAN-5tyFnTSuMivnBPD9Aur+KDxX8fCOuSaF7qGKe6bFB7roK6Q@mail.gmail.com>
Subject: Re: [PATCH 1/2] NFSD: Fix use-after-free warning when doing
 inter-server copy
To:     Dai Ngo <dai.ngo@oracle.com>
Cc:     "J. Bruce Fields" <bfields@fieldses.org>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>