Split out two helpers to make the code more readable and easier to verify
for correctness.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/nfsd/vfs.c | 144 +++++++++++++++++++++++++++++++++-------------------------
1 file changed, 84 insertions(+), 60 deletions(-)
Index: linux/fs/nfsd/vfs.c
===================================================================
--- linux.orig/fs/nfsd/vfs.c 2013-11-17 19:32:05.807321620 +0100
+++ linux/fs/nfsd/vfs.c 2013-11-18 11:45:09.971804080 +0100
@@ -298,41 +298,12 @@ commit_metadata(struct svc_fh *fhp)
}
/*
- * Set various file attributes.
- * N.B. After this call fhp needs an fh_put
+ * Go over the attributes and take care of the small differences between
+ * NFS semantics and what Linux expects.
*/
-__be32
-nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
- int check_guard, time_t guardtime)
+static void
+nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
{
- struct dentry *dentry;
- struct inode *inode;
- int accmode = NFSD_MAY_SATTR;
- umode_t ftype = 0;
- __be32 err;
- int host_err;
- int size_change = 0;
-
- if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
- accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
- if (iap->ia_valid & ATTR_SIZE)
- ftype = S_IFREG;
-
- /* Get inode */
- err = fh_verify(rqstp, fhp, ftype, accmode);
- if (err)
- goto out;
-
- dentry = fhp->fh_dentry;
- inode = dentry->d_inode;
-
- /* Ignore any mode updates on symlinks */
- if (S_ISLNK(inode->i_mode))
- iap->ia_valid &= ~ATTR_MODE;
-
- if (!iap->ia_valid)
- goto out;
-
/*
* NFSv2 does not differentiate between "set-[ac]time-to-now"
* which only requires access, and "set-[ac]time-to-X" which
@@ -342,8 +313,7 @@ nfsd_setattr(struct svc_rqst *rqstp, str
* convert to "set to now" instead of "set to explicit time"
*
* We only call inode_change_ok as the last test as technically
- * it is not an interface that we should be using. It is only
- * valid if the filesystem does not define it's own i_op->setattr.
+ * it is not an interface that we should be using.
*/
#define BOTH_TIME_SET (ATTR_ATIME_SET | ATTR_MTIME_SET)
#define MAX_TOUCH_TIME_ERROR (30*60)
@@ -369,30 +339,6 @@ nfsd_setattr(struct svc_rqst *rqstp, str
iap->ia_valid &= ~BOTH_TIME_SET;
}
}
-
- /*
- * The size case is special.
- * It changes the file as well as the attributes.
- */
- if (iap->ia_valid & ATTR_SIZE) {
- if (iap->ia_size < inode->i_size) {
- err = nfsd_permission(rqstp, fhp->fh_export, dentry,
- NFSD_MAY_TRUNC|NFSD_MAY_OWNER_OVERRIDE);
- if (err)
- goto out;
- }
-
- host_err = get_write_access(inode);
- if (host_err)
- goto out_nfserr;
-
- size_change = 1;
- host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
- if (host_err) {
- put_write_access(inode);
- goto out_nfserr;
- }
- }
/* sanitize the mode change */
if (iap->ia_valid & ATTR_MODE) {
@@ -415,8 +361,86 @@ nfsd_setattr(struct svc_rqst *rqstp, str
iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
}
}
+}
+
+static __be32
+nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
+ struct iattr *iap)
+{
+ struct inode *inode = fhp->fh_dentry->d_inode;
+ int host_err;
+
+ if (iap->ia_size < inode->i_size) {
+ __be32 err;
+
+ err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
+ NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
+ if (err)
+ return err;
+ }
+
+ host_err = get_write_access(inode);
+ if (host_err)
+ goto out_nfserrno;
+
+ host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
+ if (host_err)
+ goto out_put_write_access;
+ return 0;
+
+out_put_write_access:
+ put_write_access(inode);
+out_nfserrno:
+ return nfserrno(host_err);
+}
+
+/*
+ * Set various file attributes. After this call fhp needs an fh_put.
+ */
+__be32
+nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
+ int check_guard, time_t guardtime)
+{
+ struct dentry *dentry;
+ struct inode *inode;
+ int accmode = NFSD_MAY_SATTR;
+ umode_t ftype = 0;
+ __be32 err;
+ int host_err;
+ int size_change = 0;
+
+ if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
+ accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
+ if (iap->ia_valid & ATTR_SIZE)
+ ftype = S_IFREG;
+
+ /* Get inode */
+ err = fh_verify(rqstp, fhp, ftype, accmode);
+ if (err)
+ goto out;
+
+ dentry = fhp->fh_dentry;
+ inode = dentry->d_inode;
+
+ /* Ignore any mode updates on symlinks */
+ if (S_ISLNK(inode->i_mode))
+ iap->ia_valid &= ~ATTR_MODE;
+
+ if (!iap->ia_valid)
+ goto out;
+
+ nfsd_sanitize_attrs(inode, iap);
- /* Change the attributes. */
+ /*
+ * The size case is special, it changes the file in addition to the
+ * attributes.
+ */
+ if (iap->ia_valid & ATTR_SIZE) {
+ err = nfsd_get_write_access(rqstp, fhp, iap);
+ if (err)
+ goto out;
+ size_change = 1;
+ }
iap->ia_valid |= ATTR_CTIME;
On Mon, Nov 18, 2013 at 09:10:31AM -0500, J. Bruce Fields wrote:
> On Mon, Nov 18, 2013 at 05:07:30AM -0800, Christoph Hellwig wrote:
> > Split out two helpers to make the code more readable and easier to verify
> > for correctness.
>
> Thanks, both queued up for 2.14.
The write counter leak on a break_lease failure is quite serious,
given that nfsd_break_lease passes O_NONBLOCK and thus remote users
can arbitrarily trigger it.
Use a straight goto error label style in nfsd_setattr to make sure
we always do the put_write_access call after we got it earlier.
Signed-off-by: Christoph Hellwig <[email protected]>
---
fs/nfsd/vfs.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
Index: linux/fs/nfsd/vfs.c
===================================================================
--- linux.orig/fs/nfsd/vfs.c 2013-11-18 11:45:09.971804080 +0100
+++ linux/fs/nfsd/vfs.c 2013-11-18 14:04:55.003632006 +0100
@@ -444,27 +444,28 @@ nfsd_setattr(struct svc_rqst *rqstp, str
iap->ia_valid |= ATTR_CTIME;
- err = nfserr_notsync;
- if (!check_guard || guardtime == inode->i_ctime.tv_sec) {
- host_err = nfsd_break_lease(inode);
- if (host_err)
- goto out_nfserr;
- fh_lock(fhp);
-
- host_err = notify_change(dentry, iap, NULL);
- err = nfserrno(host_err);
- fh_unlock(fhp);
+ if (check_guard && guardtime != inode->i_ctime.tv_sec) {
+ err = nfserr_notsync;
+ goto out_put_write_access;
}
+
+ host_err = nfsd_break_lease(inode);
+ if (host_err)
+ goto out_put_write_access_nfserror;
+
+ fh_lock(fhp);
+ host_err = notify_change(dentry, iap, NULL);
+ fh_unlock(fhp);
+
+out_put_write_access_nfserror:
+ err = nfserrno(host_err);
+out_put_write_access:
if (size_change)
put_write_access(inode);
if (!err)
commit_metadata(fhp);
out:
return err;
-
-out_nfserr:
- err = nfserrno(host_err);
- goto out;
}
#if defined(CONFIG_NFSD_V2_ACL) || \
On Mon, Nov 18, 2013 at 06:16:24AM -0800, Christoph Hellwig wrote:
> On Mon, Nov 18, 2013 at 09:10:31AM -0500, J. Bruce Fields wrote:
> > On Mon, Nov 18, 2013 at 05:07:30AM -0800, Christoph Hellwig wrote:
> > > Split out two helpers to make the code more readable and easier to verify
> > > for correctness.
> >
> > Thanks, both queued up for 2.14.
>
> The write counter leak on a break_lease failure is quite serious,
> given that nfsd_break_lease passes O_NONBLOCK and thus remote users
> can arbitrarily trigger it.
Oops, I read too quickly and thought it was just cleanup.
Looks like I introduced that bug into 2.6.38 with
6a76bebefe15d9a08864f824d7f8d5beaf37c997 "nfsd4: break lease on nfsd
setattr"
OK, added a sentence saying that to the second path and I'll pass it
along with stable cc:'s after some testing.
--b.
On Mon, Nov 18, 2013 at 05:07:30AM -0800, Christoph Hellwig wrote:
> Split out two helpers to make the code more readable and easier to verify
> for correctness.
Thanks, both queued up for 2.14.
Might also be worth splitting off that time_set/inode_change_ok logic
into a separate function as its a lot of lines (comments and code)
devoted to a case that we don't care about in the normal (non-v2) case.
--b.
> Signed-off-by: Christoph Hellwig <[email protected]>
>
> ---
> fs/nfsd/vfs.c | 144 +++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 84 insertions(+), 60 deletions(-)
>
> Index: linux/fs/nfsd/vfs.c
> ===================================================================
> --- linux.orig/fs/nfsd/vfs.c 2013-11-17 19:32:05.807321620 +0100
> +++ linux/fs/nfsd/vfs.c 2013-11-18 11:45:09.971804080 +0100
> @@ -298,41 +298,12 @@ commit_metadata(struct svc_fh *fhp)
> }
>
> /*
> - * Set various file attributes.
> - * N.B. After this call fhp needs an fh_put
> + * Go over the attributes and take care of the small differences between
> + * NFS semantics and what Linux expects.
> */
> -__be32
> -nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> - int check_guard, time_t guardtime)
> +static void
> +nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
> {
> - struct dentry *dentry;
> - struct inode *inode;
> - int accmode = NFSD_MAY_SATTR;
> - umode_t ftype = 0;
> - __be32 err;
> - int host_err;
> - int size_change = 0;
> -
> - if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> - accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> - if (iap->ia_valid & ATTR_SIZE)
> - ftype = S_IFREG;
> -
> - /* Get inode */
> - err = fh_verify(rqstp, fhp, ftype, accmode);
> - if (err)
> - goto out;
> -
> - dentry = fhp->fh_dentry;
> - inode = dentry->d_inode;
> -
> - /* Ignore any mode updates on symlinks */
> - if (S_ISLNK(inode->i_mode))
> - iap->ia_valid &= ~ATTR_MODE;
> -
> - if (!iap->ia_valid)
> - goto out;
> -
> /*
> * NFSv2 does not differentiate between "set-[ac]time-to-now"
> * which only requires access, and "set-[ac]time-to-X" which
> @@ -342,8 +313,7 @@ nfsd_setattr(struct svc_rqst *rqstp, str
> * convert to "set to now" instead of "set to explicit time"
> *
> * We only call inode_change_ok as the last test as technically
> - * it is not an interface that we should be using. It is only
> - * valid if the filesystem does not define it's own i_op->setattr.
> + * it is not an interface that we should be using.
> */
> #define BOTH_TIME_SET (ATTR_ATIME_SET | ATTR_MTIME_SET)
> #define MAX_TOUCH_TIME_ERROR (30*60)
> @@ -369,30 +339,6 @@ nfsd_setattr(struct svc_rqst *rqstp, str
> iap->ia_valid &= ~BOTH_TIME_SET;
> }
> }
> -
> - /*
> - * The size case is special.
> - * It changes the file as well as the attributes.
> - */
> - if (iap->ia_valid & ATTR_SIZE) {
> - if (iap->ia_size < inode->i_size) {
> - err = nfsd_permission(rqstp, fhp->fh_export, dentry,
> - NFSD_MAY_TRUNC|NFSD_MAY_OWNER_OVERRIDE);
> - if (err)
> - goto out;
> - }
> -
> - host_err = get_write_access(inode);
> - if (host_err)
> - goto out_nfserr;
> -
> - size_change = 1;
> - host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
> - if (host_err) {
> - put_write_access(inode);
> - goto out_nfserr;
> - }
> - }
>
> /* sanitize the mode change */
> if (iap->ia_valid & ATTR_MODE) {
> @@ -415,8 +361,86 @@ nfsd_setattr(struct svc_rqst *rqstp, str
> iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
> }
> }
> +}
> +
> +static __be32
> +nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp,
> + struct iattr *iap)
> +{
> + struct inode *inode = fhp->fh_dentry->d_inode;
> + int host_err;
> +
> + if (iap->ia_size < inode->i_size) {
> + __be32 err;
> +
> + err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> + NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE);
> + if (err)
> + return err;
> + }
> +
> + host_err = get_write_access(inode);
> + if (host_err)
> + goto out_nfserrno;
> +
> + host_err = locks_verify_truncate(inode, NULL, iap->ia_size);
> + if (host_err)
> + goto out_put_write_access;
> + return 0;
> +
> +out_put_write_access:
> + put_write_access(inode);
> +out_nfserrno:
> + return nfserrno(host_err);
> +}
> +
> +/*
> + * Set various file attributes. After this call fhp needs an fh_put.
> + */
> +__be32
> +nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> + int check_guard, time_t guardtime)
> +{
> + struct dentry *dentry;
> + struct inode *inode;
> + int accmode = NFSD_MAY_SATTR;
> + umode_t ftype = 0;
> + __be32 err;
> + int host_err;
> + int size_change = 0;
> +
> + if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE))
> + accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> + if (iap->ia_valid & ATTR_SIZE)
> + ftype = S_IFREG;
> +
> + /* Get inode */
> + err = fh_verify(rqstp, fhp, ftype, accmode);
> + if (err)
> + goto out;
> +
> + dentry = fhp->fh_dentry;
> + inode = dentry->d_inode;
> +
> + /* Ignore any mode updates on symlinks */
> + if (S_ISLNK(inode->i_mode))
> + iap->ia_valid &= ~ATTR_MODE;
> +
> + if (!iap->ia_valid)
> + goto out;
> +
> + nfsd_sanitize_attrs(inode, iap);
>
> - /* Change the attributes. */
> + /*
> + * The size case is special, it changes the file in addition to the
> + * attributes.
> + */
> + if (iap->ia_valid & ATTR_SIZE) {
> + err = nfsd_get_write_access(rqstp, fhp, iap);
> + if (err)
> + goto out;
> + size_change = 1;
> + }
>
> iap->ia_valid |= ATTR_CTIME;
>