Skip to content

Commit f16b5a1

Browse files
jtlaytonjtlayton
committed
nfsd: fix handling of cached open files in nfsd4_open codepath
Bugzilla: https://bugzilla.redhat.com/2152473 commit 0b3a551 Author: Jeff Layton <jlayton@kernel.org> Date: Thu Jan 5 14:55:56 2023 -0500 nfsd: fix handling of cached open files in nfsd4_open codepath Commit fb70bf1 ("NFSD: Instantiate a struct file when creating a regular NFSv4 file") added the ability to cache an open fd over a compound. There are a couple of problems with the way this currently works: It's racy, as a newly-created nfsd_file can end up with its PENDING bit cleared while the nf is hashed, and the nf_file pointer is still zeroed out. Other tasks can find it in this state and they expect to see a valid nf_file, and can oops if nf_file is NULL. Also, there is no guarantee that we'll end up creating a new nfsd_file if one is already in the hash. If an extant entry is in the hash with a valid nf_file, nfs4_get_vfs_file will clobber its nf_file pointer with the value of op_file and the old nf_file will leak. Fix both issues by making a new nfsd_file_acquirei_opened variant that takes an optional file pointer. If one is present when this is called, we'll take a new reference to it instead of trying to open the file. If the nfsd_file already has a valid nf_file, we'll just ignore the optional file and pass the nfsd_file back as-is. Also rework the tracepoints a bit to allow for an "opened" variant and don't try to avoid counting acquisitions in the case where we already have a cached open file. Fixes: fb70bf1 ("NFSD: Instantiate a struct file when creating a regular NFSv4 file") Cc: Trond Myklebust <trondmy@hammerspace.com> Reported-by: Stanislav Saner <ssaner@redhat.com> Reported-and-Tested-by: Ruben Vestergaard <rubenv@drcmr.dk> Reported-and-Tested-by: Torkil Svensgaard <torkil@drcmr.dk> Signed-off-by: Jeff Layton <jlayton@kernel.org> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> Signed-off-by: Jeffrey Layton <jlayton@redhat.com>
1 parent ced6654 commit f16b5a1

File tree

4 files changed

+42
-71
lines changed

4 files changed

+42
-71
lines changed

‎fs/nfsd/filecache.c‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,8 +1069,8 @@ nfsd_file_is_cached(struct inode *inode)
10691069

10701070
static __be32
10711071
nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
1072-
unsigned int may_flags, struct nfsd_file **pnf,
1073-
bool open, bool want_gc)
1072+
unsigned int may_flags, struct file *file,
1073+
struct nfsd_file **pnf, bool want_gc)
10741074
{
10751075
struct nfsd_file_lookup_key key = {
10761076
.type = NFSD_FILE_KEY_FULL,
@@ -1145,8 +1145,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
11451145
status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags));
11461146
out:
11471147
if (status == nfs_ok) {
1148-
if (open)
1149-
this_cpu_inc(nfsd_file_acquisitions);
1148+
this_cpu_inc(nfsd_file_acquisitions);
11501149
*pnf = nf;
11511150
} else {
11521151
if (refcount_dec_and_test(&nf->nf_ref))
@@ -1156,20 +1155,23 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
11561155

11571156
out_status:
11581157
put_cred(key.cred);
1159-
if (open)
1160-
trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
1158+
trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
11611159
return status;
11621160

11631161
open_file:
11641162
trace_nfsd_file_alloc(nf);
11651163
nf->nf_mark = nfsd_file_mark_find_or_create(nf, key.inode);
11661164
if (nf->nf_mark) {
1167-
if (open) {
1165+
if (file) {
1166+
get_file(file);
1167+
nf->nf_file = file;
1168+
status = nfs_ok;
1169+
trace_nfsd_file_opened(nf, status);
1170+
} else {
11681171
status = nfsd_open_verified(rqstp, fhp, may_flags,
11691172
&nf->nf_file);
11701173
trace_nfsd_file_open(nf, status);
1171-
} else
1172-
status = nfs_ok;
1174+
}
11731175
} else
11741176
status = nfserr_jukebox;
11751177
/*
@@ -1205,7 +1207,7 @@ __be32
12051207
nfsd_file_acquire_gc(struct svc_rqst *rqstp, struct svc_fh *fhp,
12061208
unsigned int may_flags, struct nfsd_file **pnf)
12071209
{
1208-
return nfsd_file_do_acquire(rqstp, fhp, may_flags, pnf, true, true);
1210+
return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, true);
12091211
}
12101212

12111213
/**
@@ -1226,28 +1228,30 @@ __be32
12261228
nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
12271229
unsigned int may_flags, struct nfsd_file **pnf)
12281230
{
1229-
return nfsd_file_do_acquire(rqstp, fhp, may_flags, pnf, true, false);
1231+
return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, false);
12301232
}
12311233

12321234
/**
1233-
* nfsd_file_create - Get a struct nfsd_file, do not open
1235+
* nfsd_file_acquire_opened - Get a struct nfsd_file using existing open file
12341236
* @rqstp: the RPC transaction being executed
12351237
* @fhp: the NFS filehandle of the file just created
12361238
* @may_flags: NFSD_MAY_ settings for the file
1239+
* @file: cached, already-open file (may be NULL)
12371240
* @pnf: OUT: new or found "struct nfsd_file" object
12381241
*
1239-
* The nfsd_file_object returned by this API is reference-counted
1240-
* but not garbage-collected. The object is released immediately
1241-
* one RCU grace period after the final nfsd_file_put().
1242+
* Acquire a nfsd_file object that is not GC'ed. If one doesn't already exist,
1243+
* and @file is non-NULL, use it to instantiate a new nfsd_file instead of
1244+
* opening a new one.
12421245
*
12431246
* Returns nfs_ok and sets @pnf on success; otherwise an nfsstat in
12441247
* network byte order is returned.
12451248
*/
12461249
__be32
1247-
nfsd_file_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
1248-
unsigned int may_flags, struct nfsd_file **pnf)
1250+
nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
1251+
unsigned int may_flags, struct file *file,
1252+
struct nfsd_file **pnf)
12491253
{
1250-
return nfsd_file_do_acquire(rqstp, fhp, may_flags, pnf, false, false);
1254+
return nfsd_file_do_acquire(rqstp, fhp, may_flags, file, pnf, false);
12511255
}
12521256

12531257
/*

‎fs/nfsd/filecache.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,8 @@ __be32 nfsd_file_acquire_gc(struct svc_rqst *rqstp, struct svc_fh *fhp,
6060
unsigned int may_flags, struct nfsd_file **nfp);
6161
__be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
6262
unsigned int may_flags, struct nfsd_file **nfp);
63-
__be32 nfsd_file_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
64-
unsigned int may_flags, struct nfsd_file **nfp);
63+
__be32 nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp,
64+
unsigned int may_flags, struct file *file,
65+
struct nfsd_file **nfp);
6566
int nfsd_file_cache_stats_show(struct seq_file *m, void *v);
6667
#endif /* _FS_NFSD_FILECACHE_H */

‎fs/nfsd/nfs4state.c‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5042,18 +5042,10 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
50425042
if (!fp->fi_fds[oflag]) {
50435043
spin_unlock(&fp->fi_lock);
50445044

5045-
if (!open->op_filp) {
5046-
status = nfsd_file_acquire(rqstp, cur_fh, access, &nf);
5047-
if (status != nfs_ok)
5048-
goto out_put_access;
5049-
} else {
5050-
status = nfsd_file_create(rqstp, cur_fh, access, &nf);
5051-
if (status != nfs_ok)
5052-
goto out_put_access;
5053-
nf->nf_file = open->op_filp;
5054-
open->op_filp = NULL;
5055-
trace_nfsd_file_create(rqstp, access, nf);
5056-
}
5045+
status = nfsd_file_acquire_opened(rqstp, cur_fh, access,
5046+
open->op_filp, &nf);
5047+
if (status != nfs_ok)
5048+
goto out_put_access;
50575049

50585050
spin_lock(&fp->fi_lock);
50595051
if (!fp->fi_fds[oflag]) {

‎fs/nfsd/trace.h‎

Lines changed: 13 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -902,43 +902,6 @@ TRACE_EVENT(nfsd_file_acquire,
902902
)
903903
);
904904

905-
TRACE_EVENT(nfsd_file_create,
906-
TP_PROTO(
907-
const struct svc_rqst *rqstp,
908-
unsigned int may_flags,
909-
const struct nfsd_file *nf
910-
),
911-
912-
TP_ARGS(rqstp, may_flags, nf),
913-
914-
TP_STRUCT__entry(
915-
__field(const void *, nf_inode)
916-
__field(const void *, nf_file)
917-
__field(unsigned long, may_flags)
918-
__field(unsigned long, nf_flags)
919-
__field(unsigned long, nf_may)
920-
__field(unsigned int, nf_ref)
921-
__field(u32, xid)
922-
),
923-
924-
TP_fast_assign(
925-
__entry->nf_inode = nf->nf_inode;
926-
__entry->nf_file = nf->nf_file;
927-
__entry->may_flags = may_flags;
928-
__entry->nf_flags = nf->nf_flags;
929-
__entry->nf_may = nf->nf_may;
930-
__entry->nf_ref = refcount_read(&nf->nf_ref);
931-
__entry->xid = be32_to_cpu(rqstp->rq_xid);
932-
),
933-
934-
TP_printk("xid=0x%x inode=%p may_flags=%s ref=%u nf_flags=%s nf_may=%s nf_file=%p",
935-
__entry->xid, __entry->nf_inode,
936-
show_nfsd_may_flags(__entry->may_flags),
937-
__entry->nf_ref, show_nf_flags(__entry->nf_flags),
938-
show_nfsd_may_flags(__entry->nf_may), __entry->nf_file
939-
)
940-
);
941-
942905
TRACE_EVENT(nfsd_file_insert_err,
943906
TP_PROTO(
944907
const struct svc_rqst *rqstp,
@@ -1000,8 +963,8 @@ TRACE_EVENT(nfsd_file_cons_err,
1000963
)
1001964
);
1002965

1003-
TRACE_EVENT(nfsd_file_open,
1004-
TP_PROTO(struct nfsd_file *nf, __be32 status),
966+
DECLARE_EVENT_CLASS(nfsd_file_open_class,
967+
TP_PROTO(const struct nfsd_file *nf, __be32 status),
1005968
TP_ARGS(nf, status),
1006969
TP_STRUCT__entry(
1007970
__field(void *, nf_inode) /* cannot be dereferenced */
@@ -1025,6 +988,17 @@ TRACE_EVENT(nfsd_file_open,
1025988
__entry->nf_file)
1026989
)
1027990

991+
#define DEFINE_NFSD_FILE_OPEN_EVENT(name) \
992+
DEFINE_EVENT(nfsd_file_open_class, name, \
993+
TP_PROTO( \
994+
const struct nfsd_file *nf, \
995+
__be32 status \
996+
), \
997+
TP_ARGS(nf, status))
998+
999+
DEFINE_NFSD_FILE_OPEN_EVENT(nfsd_file_open);
1000+
DEFINE_NFSD_FILE_OPEN_EVENT(nfsd_file_opened);
1001+
10281002
TRACE_EVENT(nfsd_file_is_cached,
10291003
TP_PROTO(
10301004
const struct inode *inode,

0 commit comments

Comments
 (0)