* [PATCH] nfsd: fix file access refcount leak when nfsd4_truncate fails @ 2014-06-26 10:49 Jeff Layton 2014-06-26 11:17 ` Christoph Hellwig 0 siblings, 1 reply; 3+ messages in thread From: Jeff Layton @ 2014-06-26 10:49 UTC (permalink / raw) To: bfields; +Cc: linux-nfs 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. Signed-off-by: Jeff Layton <jlayton@primarydata.com> --- fs/nfsd/nfs4state.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 2204e1fe5725..3b19008c2978 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3351,8 +3351,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf if (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); -- 1.9.3 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nfsd: fix file access refcount leak when nfsd4_truncate fails 2014-06-26 10:49 [PATCH] nfsd: fix file access refcount leak when nfsd4_truncate fails Jeff Layton @ 2014-06-26 11:17 ` Christoph Hellwig 2014-06-26 13:33 ` Jeff Layton 0 siblings, 1 reply; 3+ messages in thread From: Christoph Hellwig @ 2014-06-26 11:17 UTC (permalink / raw) To: Jeff Layton; +Cc: bfields, linux-nfs > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 2204e1fe5725..3b19008c2978 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -3351,8 +3351,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf > if (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): diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 2204e1f..f904951 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -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); ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nfsd: fix file access refcount leak when nfsd4_truncate fails 2014-06-26 11:17 ` Christoph Hellwig @ 2014-06-26 13:33 ` Jeff Layton 0 siblings, 0 replies; 3+ messages in thread From: Jeff Layton @ 2014-06-26 13:33 UTC (permalink / raw) To: Christoph Hellwig; +Cc: bfields, linux-nfs On Thu, 26 Jun 2014 04:17:52 -0700 Christoph Hellwig <hch@infradead.org> wrote: > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 2204e1fe5725..3b19008c2978 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -3351,8 +3351,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf > > if (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): > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 2204e1f..f904951 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -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); 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. -- Jeff Layton <jlayton@primarydata.com> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-06-26 13:34 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-26 10:49 [PATCH] nfsd: fix file access refcount leak when nfsd4_truncate fails Jeff Layton 2014-06-26 11:17 ` Christoph Hellwig 2014-06-26 13:33 ` Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.