nfsd: fix file access refcount leak when nfsd4_truncate fails
Subsystems (2)
PATCH 1/1 nfsd: fix file access refcount leak when nfsd4_truncate fails
We currently will get access to the file, and then call nfsd4_truncate to (possibly) truncate it. If that operation fails though, then the access references will never be released as the nfs4_ol_stateid is never initialized.
--- fs/nfsd/nfs4state.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
fs/nfsd/nfs4state.c +4 -1
@@ -3351,8 +3351,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfif (status) goto out; status =nfsd4_truncate (rqstp, current_fh, open); -if (status) +if (status) { +nfs4_file_put_access (fp, +nfs4_access_to_omode (open->op_share_access)); goto out; + } stp = open->op_stp; open->op_stp =NULL ;init_open_stateid (stp, fp, open);
Signature · 1 line
1.9.3
Quoted 5 lines
fs/nfsd/nfs4state.c +0
-0
fs/nfsd/nfs4state.c nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf · line 3358
Quoted 9 lines
fs/nfsd/nfs4state.c +4
-1@@ -3351,8 +3351,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfif (status) goto out; status =nfsd4_truncate (rqstp, current_fh, open); -if (status) +if (status) { +nfs4_file_put_access (fp, +nfs4_access_to_omode (open->op_share_access)); goto out; + }
This looks correct, but a little awkward. Given that nfs4_get_vfs_file generally is followed by the truncate something like the (untested) patch below seems much nicer (and as a follow on I'd merge nfs4_upgrade_open into nfsd4_process_open2):
fs/nfsd/nfs4state.c +30 -32
@@ -3042,6 +3042,21 @@ static inline int nfs4_access_to_access(u32 nfs4_access)return flags; } +static inline __be32 +nfsd4_truncate (struct svc_rqst *rqstp,struct svc_fh *fh, +struct nfsd4_open *open) +{ +struct iattr iattr = { + .ia_valid =ATTR_SIZE , + .ia_size =0 , + }; +if (!open->op_truncate) +return 0 ; +if (!(open->op_share_access &NFS4_SHARE_ACCESS_WRITE )) +return nfserr_inval; +return nfsd_setattr (rqstp, fh, &iattr,0 , (time_t )0 ); +} +static __be32 nfs4_get_vfs_file (struct svc_rqst *rqstp,struct nfs4_file *fp,struct svc_fh *cur_fh,struct nfsd4_open *open) {
@@ -3053,53 +3068,39 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, status =nfsd_open (rqstp, cur_fh,S_IFREG , access, &fp->fi_fds[oflag]);if (status) -return status; + goto out; }nfs4_file_get_access (fp, oflag); + status =nfsd4_truncate (rqstp, cur_fh, open); +if (status) + goto out_put_access; +return nfs_ok; -} -static inline __be32 -nfsd4_truncate (struct svc_rqst *rqstp,struct svc_fh *fh, -struct nfsd4_open *open) -{ -struct iattr iattr = { - .ia_valid =ATTR_SIZE , - .ia_size =0 , - }; -if (!open->op_truncate) -return 0 ; -if (!(open->op_share_access &NFS4_SHARE_ACCESS_WRITE )) -return nfserr_inval; -return nfsd_setattr (rqstp, fh, &iattr,0 , (time_t )0 ); +out_put_access: +nfs4_file_put_access (fp, oflag); +out: +return status; }static __be32 nfs4_upgrade_open (struct svc_rqst *rqstp,struct nfs4_file *fp,struct svc_fh *cur_fh,struct nfs4_ol_stateid *stp,struct nfsd4_open *open) {u32 op_share_access = open->op_share_access; -bool new_access;__be32 status; - new_access = !test_access (op_share_access, stp); -if (new_access) { +if (!test_access (op_share_access, stp)) status =nfs4_get_vfs_file (rqstp, fp, cur_fh, open); -if (status) -return status; - } - status =nfsd4_truncate (rqstp, cur_fh, open); -if (status) { -if (new_access) { -int oflag =nfs4_access_to_omode (op_share_access); -nfs4_file_put_access (fp, oflag); - } +else + status =nfsd4_truncate (rqstp, cur_fh, open); + +if (status)return status; - } +/* remember the open */ set_access (op_share_access, stp);set_deny (open->op_share_deny, stp); -return nfs_ok; }
@@ -3350,9 +3351,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf status =nfs4_get_vfs_file (rqstp, fp, current_fh, open);if (status) goto out; - status =nfsd4_truncate (rqstp, current_fh, open); -if (status) - goto out; stp = open->op_stp; open->op_stp =NULL ;init_open_stateid (stp, fp, open);
Quoted 116 lines · On Thu, 26 Jun 2014 04:17:52 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 5 lines
fs/nfsd/nfs4state.c +0
-0fs/nfsd/nfs4state.c nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf · line 3358
Quoted 9 lines
fs/nfsd/nfs4state.c +4
-1@@ -3351,8 +3351,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfif (status) goto out; status =nfsd4_truncate (rqstp, current_fh, open); -if (status) +if (status) { +nfs4_file_put_access (fp, +nfs4_access_to_omode (open->op_share_access)); goto out; + }This looks correct, but a little awkward. Given that
nfs4_get_vfs_filegenerally is followed by the truncate something like the (untested) patch below seems much nicer (and as a follow on I'd mergenfs4_upgrade_openintonfsd4_process_open2):fs/nfsd/nfs4state.c +30
-29@@ -3042,6 +3042,21 @@ static inline int nfs4_access_to_access(u32 nfs4_access)return flags; } +static inline __be32 +nfsd4_truncate (struct svc_rqst *rqstp,struct svc_fh *fh, +struct nfsd4_open *open) +{ +struct iattr iattr = { + .ia_valid =ATTR_SIZE , + .ia_size =0 , + }; +if (!open->op_truncate) +return 0 ; +if (!(open->op_share_access &NFS4_SHARE_ACCESS_WRITE )) +return nfserr_inval; +return nfsd_setattr (rqstp, fh, &iattr,0 , (time_t )0 ); +} +static __be32 nfs4_get_vfs_file (struct svc_rqst *rqstp,struct nfs4_file *fp,struct svc_fh *cur_fh,struct nfsd4_open *open) {@@ -3053,53 +3068,39 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp, status =nfsd_open (rqstp, cur_fh,S_IFREG , access, &fp->fi_fds[oflag]);if (status) -return status; + goto out; }nfs4_file_get_access (fp, oflag); + status =nfsd4_truncate (rqstp, cur_fh, open); +if (status) + goto out_put_access; +return nfs_ok; -} -static inline __be32 -nfsd4_truncate (struct svc_rqst *rqstp,struct svc_fh *fh, -struct nfsd4_open *open) -{ -struct iattr iattr = { - .ia_valid =ATTR_SIZE , - .ia_size =0 , - }; -if (!open->op_truncate) -return 0 ; -if (!(open->op_share_access &NFS4_SHARE_ACCESS_WRITE )) -return nfserr_inval; -return nfsd_setattr (rqstp, fh, &iattr,0 , (time_t )0 ); +out_put_access: +nfs4_file_put_access (fp, oflag); +out: +return status; }static __be32 nfs4_upgrade_open (struct svc_rqst *rqstp,struct nfs4_file *fp,struct svc_fh *cur_fh,struct nfs4_ol_stateid *stp,struct nfsd4_open *open) {u32 op_share_access = open->op_share_access; -bool new_access;__be32 status; - new_access = !test_access (op_share_access, stp); -if (new_access) { +if (!test_access (op_share_access, stp)) status =nfs4_get_vfs_file (rqstp, fp, cur_fh, open); -if (status) -return status; - } - status =nfsd4_truncate (rqstp, cur_fh, open); -if (status) { -if (new_access) { -int oflag =nfs4_access_to_omode (op_share_access); -nfs4_file_put_access (fp, oflag); - } +else + status =nfsd4_truncate (rqstp, cur_fh, open); + +if (status)return status; - } +/* remember the open */ set_access (op_share_access, stp);set_deny (open->op_share_deny, stp); -return nfs_ok; }
fs/nfsd/nfs4state.c nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf · line 3356
Quoted 9 lines
fs/nfsd/nfs4state.c +0
-3@@ -3350,9 +3351,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf status =nfs4_get_vfs_file (rqstp, fp, current_fh, open);if (status) goto out; - status =nfsd4_truncate (rqstp, current_fh, open); -if (status) - goto out; stp = open->op_stp; open->op_stp =NULL ;init_open_stateid (stp, fp, open);
That does look nicer.
I'll plan to put that one at the head of the set and test it out today, and rebase my patches on top. FWIW, I just found this by inspection and didn't actually hit it that I know of.
Signature · 1 line
Jeff Layton <[email protected]>