From: Jeff Layton <jlayton@kernel.org>
To: chuck.lever@oracle.com
Cc: linux-nfs@vger.kernel.org, dai.ngo@oracle.com, aglo@umich.edu
Subject: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath
Date: Tue, 17 Jan 2023 14:38:31 -0500 [thread overview]
Message-ID: <20230117193831.75201-3-jlayton@kernel.org> (raw)
In-Reply-To: <20230117193831.75201-1-jlayton@kernel.org>
There are two different flavors of the nfsd4_copy struct. One is
embedded in the compound and is used directly in synchronous copies. The
other is dynamically allocated, refcounted and tracked in the client
struture. For the embedded one, the cleanup just involves releasing any
nfsd_files held on its behalf. For the async one, the cleanup is a bit
more involved, and we need to dequeue it from lists, unhash it, etc.
There is at least one potential refcount leak in this code now. If the
kthread_create call fails, then both the src and dst nfsd_files in the
original nfsd4_copy object are leaked.
The cleanup in this codepath is also sort of weird. In the async copy
case, we'll have up to four nfsd_file references (src and dst for both
flavors of copy structure). They are both put at the end of
nfsd4_do_async_copy, even though the ones held on behalf of the embedded
one outlive that structure.
Change it so that we always clean up the nfsd_file refs held by the
embedded copy structure before nfsd4_copy returns. Rework
cleanup_async_copy to handle both inter and intra copies. Eliminate
nfsd4_cleanup_intra_ssc since it now becomes a no-op.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 37a9cc8ae7ae..62b9d6c1b18b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
nfs42_ssc_close(filp);
- nfsd_file_put(dst);
fput(filp);
spin_lock(&nn->nfsd_ssc_lock);
@@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
©->nf_dst);
}
-static void
-nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
-{
- nfsd_file_put(src);
- nfsd_file_put(dst);
-}
-
static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
{
struct nfsd4_cb_offload *cbo =
@@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
dst->ss_nsui = src->ss_nsui;
}
+static void release_copy_files(struct nfsd4_copy *copy)
+{
+ if (copy->nf_src)
+ nfsd_file_put(copy->nf_src);
+ if (copy->nf_dst)
+ nfsd_file_put(copy->nf_dst);
+}
+
static void cleanup_async_copy(struct nfsd4_copy *copy)
{
nfs4_free_copy_state(copy);
- nfsd_file_put(copy->nf_dst);
- if (!nfsd4_ssc_is_inter(copy))
- nfsd_file_put(copy->nf_src);
+ release_copy_files(copy);
spin_lock(©->cp_clp->async_lock);
list_del(©->copies);
spin_unlock(©->cp_clp->async_lock);
@@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
} else {
nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
copy->nf_dst->nf_file, false);
- nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
}
do_callback:
@@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
} else {
status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
copy->nf_dst->nf_file, true);
- nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
}
out:
+ release_copy_files(copy);
return status;
out_err:
if (async_copy)
--
2.39.0
next prev parent reply other threads:[~2023-01-17 21:18 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-17 19:38 [PATCH 0/2] nfsd: COPY refcounting fix and cleanup Jeff Layton
2023-01-17 19:38 ` [PATCH 1/2] nfsd: zero out pointers after putting nfsd_files on COPY setup error Jeff Layton
2023-01-17 19:38 ` Jeff Layton [this message]
2023-01-18 14:42 ` [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath Olga Kornievskaia
2023-01-18 15:27 ` Jeff Layton
2023-01-18 16:29 ` Olga Kornievskaia
2023-01-18 16:39 ` Chuck Lever III
2023-01-18 17:06 ` Jeff Layton
2023-01-18 17:11 ` Chuck Lever III
2023-01-18 17:26 ` Jeff Layton
2023-01-18 17:48 ` Olga Kornievskaia
2023-01-18 16:57 ` Jeff Layton
2023-01-18 17:07 ` Olga Kornievskaia
2023-01-18 18:16 ` Olga Kornievskaia
2023-01-18 18:34 ` Jeff Layton
2023-01-19 1:45 ` Olga Kornievskaia
2023-01-19 5:05 ` dai.ngo
2023-01-19 10:56 ` Jeff Layton
2023-01-19 18:38 ` dai.ngo
2023-01-20 11:43 ` Jeff Layton
2023-01-21 18:56 ` dai.ngo
2023-01-21 19:50 ` dai.ngo
2023-01-21 20:05 ` Jeff Layton
2023-01-21 20:12 ` Chuck Lever III
2023-01-21 21:28 ` dai.ngo
2023-01-22 16:45 ` Chuck Lever III
2023-01-22 17:10 ` Chuck Lever III
2023-01-23 12:17 ` Jeff Layton
2023-01-23 15:22 ` Olga Kornievskaia
2023-01-23 15:32 ` Jeff Layton
2023-01-23 20:32 ` dai.ngo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230117193831.75201-3-jlayton@kernel.org \
--to=jlayton@kernel.org \
--cc=aglo@umich.edu \
--cc=chuck.lever@oracle.com \
--cc=dai.ngo@oracle.com \
--cc=linux-nfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox