Thanks Olga for reviewing the patch, reply inline below: On 3/8/21 10:35 AM, Olga Kornievskaia wrote:
On Tue, Mar 2, 2021 at 2:47 PM Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:This patch is to fix the resource leak problem of the source file when doing inter-server copy. The fix is to close and release the file in __nfs42_ssc_close after the copy is done. Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx> --- fs/nfs/nfs4file.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 57b3821d975a..20163fe702a7 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -405,6 +405,12 @@ static void __nfs42_ssc_close(struct file *filep) struct nfs_open_context *ctx = nfs_file_open_context(filep); ctx->state->flags = 0; + + if (!filep) + return; + get_file(filep); + filp_close(filep, NULL); + fput(filep); }I don't understand this logic. There is no reason to call filp_close()?
This is to follow the steps done in nfsd_file_put/.../nfsd_file_free. However since this is the source file the flush is probably not needed, just there to be safe. I will remove it.
All this would be done by doing a fput(). Also fput() would drop a reference on the mount point. So we are doing this then we can't call that extra disconnect that was added by another patch.
nfsd4_interssc_disconnect does not need to access the source file. I tested both patches together and did not see any problem. If there is use-after_free condition the code detects it and there would be warning messages in /var/log/messages.
Anyway I don't see why there is any reason to call anything but the fput(), I'm not sure why __nfs42_ssc_close() is a better function and doesn't lead to the "use_after_free".
Since __nfs42_ssc_open was called open the file, I think __nfs42_ssc_close is an appropriate place to close the file. -Dai
static const struct nfs4_ssc_client_ops nfs4_ssc_clnt_ops_tbl = { -- 2.9.5
