From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: ptxdist@pengutronix.de
Subject: [ptxdist] [PATCH] unfs3: fix attribute settings, second try
Date: Thu, 27 Feb 2020 14:21:37 +0100 [thread overview]
Message-ID: <20200227132137.21330-1-u.kleine-koenig@pengutronix.de> (raw)
The last change to unfs3 repaired some things for symlinks but in return
broke stuff for regular files.
With this change the previous attempt is replaced by something better
tested now.
Instead of letting unfs open the file to adapt, use fchmodat() et al,
which are racy if the underlaying filesystem changes, but on the nfs
side it is fine.
Fixes: 4e21b490eb28 ("unfs3: fix attribute setting for symlinks")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
...n-t-open-a-file-to-set-its-attribute.patch | 246 ++++++++++++++++++
...5-attr-use-futimens-instead-of-utime.patch | 102 --------
...-O_PATH-to-set-attributes-of-symlink.patch | 81 ------
patches/unfs3-0.9.22/series | 5 +-
4 files changed, 248 insertions(+), 186 deletions(-)
create mode 100644 patches/unfs3-0.9.22/0005-attr-Don-t-open-a-file-to-set-its-attribute.patch
delete mode 100644 patches/unfs3-0.9.22/0005-attr-use-futimens-instead-of-utime.patch
delete mode 100644 patches/unfs3-0.9.22/0006-attr-make-use-of-O_PATH-to-set-attributes-of-symlink.patch
diff --git a/patches/unfs3-0.9.22/0005-attr-Don-t-open-a-file-to-set-its-attribute.patch b/patches/unfs3-0.9.22/0005-attr-Don-t-open-a-file-to-set-its-attribute.patch
new file mode 100644
index 000000000000..e14a8a1ef4ae
--- /dev/null
+++ b/patches/unfs3-0.9.22/0005-attr-Don-t-open-a-file-to-set-its-attribute.patch
@@ -0,0 +1,246 @@
+From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
+Date: Thu, 27 Feb 2020 14:04:18 +0100
+Subject: [PATCH] attr: Don't open a file to set its attribute
+
+Instead of opening the file (which has side effects like updating atime)
+use utimensat(), fchownat() and fchmodat() to set the relevant
+attributes. The downside is that this makes the code somewhat more racy
+with local changes done to the exported filesystem. (Changes via nfs are
+serialized however, so the impact isn't serious.)
+
+Also this change breaks compilation on Windows.
+---
+ attr.c | 176 +++++++++++++++--------------------------------------------------
+ 1 file changed, 41 insertions(+), 135 deletions(-)
+
+diff --git a/attr.c b/attr.c
+index 2653ed0a16bb..78ba0feee4a7 100644
+--- a/attr.c
++++ b/attr.c
+@@ -267,38 +267,41 @@ post_op_attr get_post_cached(struct svc_req * req)
+
+ /*
+ * setting of time, races with local filesystem
+- *
+- * there is no futimes() function in POSIX or Linux
+ */
+-static nfsstat3 set_time(const char *path, backend_statstruct buf, sattr3 new)
++static nfsstat3 set_time(const char *path, sattr3 new)
+ {
+- time_t new_atime, new_mtime;
+- struct utimbuf utim;
+- int res;
+-
+ /* set atime and mtime */
+ if (new.atime.set_it != DONT_CHANGE || new.mtime.set_it != DONT_CHANGE) {
+
++ /* atime in t[0], mtime in t[1] */
++ struct timespec t[2];
++ int res;
++
+ /* compute atime to set */
+- if (new.atime.set_it == SET_TO_SERVER_TIME)
+- new_atime = time(NULL);
+- else if (new.atime.set_it == SET_TO_CLIENT_TIME)
+- new_atime = new.atime.set_atime_u.atime.seconds;
+- else /* DONT_CHANGE */
+- new_atime = buf.st_atime;
++ if (new.atime.set_it == SET_TO_SERVER_TIME) {
++ t[0].tv_sec = UTIME_NOW;
++ t[0].tv_nsec = UTIME_NOW;
++ } else if (new.atime.set_it == SET_TO_CLIENT_TIME) {
++ t[0].tv_sec = new.atime.set_atime_u.atime.seconds;
++ t[0].tv_nsec = new.atime.set_atime_u.atime.nseconds;
++ } else {
++ t[0].tv_sec = UTIME_OMIT;
++ t[0].tv_nsec = UTIME_OMIT;
++ }
+
+ /* compute mtime to set */
+- if (new.mtime.set_it == SET_TO_SERVER_TIME)
+- new_mtime = time(NULL);
+- else if (new.mtime.set_it == SET_TO_CLIENT_TIME)
+- new_mtime = new.mtime.set_mtime_u.mtime.seconds;
+- else /* DONT_CHANGE */
+- new_mtime = buf.st_mtime;
+-
+- utim.actime = new_atime;
+- utim.modtime = new_mtime;
++ if (new.mtime.set_it == SET_TO_SERVER_TIME) {
++ t[1].tv_sec = UTIME_NOW;
++ t[1].tv_nsec = UTIME_NOW;
++ } else if (new.mtime.set_it == SET_TO_CLIENT_TIME) {
++ t[1].tv_sec = new.mtime.set_mtime_u.mtime.seconds;
++ t[1].tv_nsec = new.mtime.set_mtime_u.mtime.nseconds;
++ } else {
++ t[1].tv_sec = UTIME_OMIT;
++ t[1].tv_nsec = UTIME_OMIT;
++ }
+
+- res = backend_utime(path, &utim);
++ res = utimensat(AT_FDCWD, path, t, AT_SYMLINK_NOFOLLOW);
+ if (res == -1)
+ return setattr_err();
+ }
+@@ -307,23 +310,28 @@ static nfsstat3 set_time(const char *path, backend_statstruct buf, sattr3 new)
+ }
+
+ /*
+- * race unsafe setting of attributes
++ * set attributes of an object
+ */
+-static nfsstat3 set_attr_unsafe(const char *path, nfs_fh3 nfh, sattr3 new)
++nfsstat3 set_attr(const char *path, nfs_fh3 nfh, sattr3 new)
+ {
+ unfs3_fh_t *fh = (void *) nfh.data.data_val;
++ int res;
+ uid_t new_uid;
+ gid_t new_gid;
+ backend_statstruct buf;
+- int res;
+
+ res = backend_lstat(path, &buf);
+ if (res != 0)
+- return NFS3ERR_STALE;
++ return NFS3ERR_STALE;
+
+- /* check local fs race */
+- if (buf.st_dev != fh->dev || buf.st_ino != fh->ino)
+- return NFS3ERR_STALE;
++ /*
++ * Setting the mode on symlinks isn't possible on Linux. But passing
++ * AT_SYMLINK_NOFOLLOW on non-symlinks makes the call return failure, so
++ * it must not be used here and we check for symlinks explicitly.
++ */
++ if (S_ISLNK(buf.st_mode) && new.mode.set_it == TRUE &&
++ new.mode.set_mode3_u.mode != buf.st_mode)
++ return NFS3ERR_NOTSUPP;
+
+ /* set file size */
+ if (new.size.set_it == TRUE) {
+@@ -346,122 +354,20 @@ static nfsstat3 set_attr_unsafe(const char *path, nfs_fh3 nfh, sattr3 new)
+ else
+ new_gid = -1;
+
+- res = backend_lchown(path, new_uid, new_gid);
++ res = fchownat(AT_FDCWD, path, new_uid, new_gid, AT_SYMLINK_NOFOLLOW);
+ if (res == -1)
+ return setattr_err();
+ }
+
+ /* set mode */
+ if (new.mode.set_it == TRUE) {
+- res = backend_chmod(path, new.mode.set_mode3_u.mode);
++ res = fchmodat(AT_FDCWD, path, new.mode.set_mode3_u.mode, 0);
+ if (res == -1)
+ return setattr_err();
+ }
+
+- return set_time(path, buf, new);
+-}
+-
+-/*
+- * set attributes of an object
+- */
+-nfsstat3 set_attr(const char *path, nfs_fh3 nfh, sattr3 new)
+-{
+- unfs3_fh_t *fh = (void *) nfh.data.data_val;
+- int res, fd;
+- uid_t new_uid;
+- gid_t new_gid;
+- backend_statstruct buf;
+-
+- res = backend_lstat(path, &buf);
+- if (res != 0)
+- return NFS3ERR_STALE;
+-
+- /*
+- * don't open(2) device nodes, it could trigger
+- * module loading on the server
+- */
+- if (S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode))
+- return set_attr_unsafe(path, nfh, new);
+-
+-#ifdef S_ISLNK
+- /*
+- * opening a symlink would open the underlying file,
+- * don't try to do that
+- */
+- if (S_ISLNK(buf.st_mode))
+- return set_attr_unsafe(path, nfh, new);
+-#endif
+-
+- /*
+- * open object for atomic setting of attributes
+- */
+- fd = backend_open(path, O_WRONLY | O_NONBLOCK);
+- if (fd == -1)
+- fd = backend_open(path, O_RDONLY | O_NONBLOCK);
+-
+- if (fd == -1)
+- return set_attr_unsafe(path, nfh, new);
+-
+- res = backend_fstat(fd, &buf);
+- if (res == -1) {
+- backend_close(fd);
+- return NFS3ERR_STALE;
+- }
+-
+- /* check local fs race */
+- if (fh->dev != buf.st_dev || fh->ino != buf.st_ino ||
+- fh->gen != backend_get_gen(buf, fd, path)) {
+- backend_close(fd);
+- return NFS3ERR_STALE;
+- }
+-
+- /* set file size */
+- if (new.size.set_it == TRUE) {
+- res = backend_ftruncate(fd, new.size.set_size3_u.size);
+- if (res == -1) {
+- backend_close(fd);
+- return setattr_err();
+- }
+- }
+-
+- /* set uid and gid */
+- if (new.uid.set_it == TRUE || new.gid.set_it == TRUE) {
+- if (new.uid.set_it == TRUE)
+- new_uid = new.uid.set_uid3_u.uid;
+- else
+- new_uid = -1;
+- if (new_uid == buf.st_uid)
+- new_uid = -1;
+-
+- if (new.gid.set_it == TRUE)
+- new_gid = new.gid.set_gid3_u.gid;
+- else
+- new_gid = -1;
+-
+- res = backend_fchown(fd, new_uid, new_gid);
+- if (res == -1) {
+- backend_close(fd);
+- return setattr_err();
+- }
+- }
+-
+- /* set mode */
+- if (new.mode.set_it == TRUE) {
+- res = backend_fchmod(fd, new.mode.set_mode3_u.mode);
+- if (res == -1) {
+- backend_close(fd);
+- return setattr_err();
+- }
+- }
+-
+- res = backend_close(fd);
+- if (res == -1) {
+- /* error on close probably means attributes didn't make it */
+- return NFS3ERR_IO;
+- }
+-
+ /* finally, set times */
+- return set_time(path, buf, new);
++ return set_time(path, new);
+ }
+
+ /*
diff --git a/patches/unfs3-0.9.22/0005-attr-use-futimens-instead-of-utime.patch b/patches/unfs3-0.9.22/0005-attr-use-futimens-instead-of-utime.patch
deleted file mode 100644
index 36f69e44b81a..000000000000
--- a/patches/unfs3-0.9.22/0005-attr-use-futimens-instead-of-utime.patch
+++ /dev/null
@@ -1,102 +0,0 @@
-From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
-Date: Tue, 18 Feb 2020 15:20:12 +0100
-Subject: [PATCH] attr: use futimens() instead of utime()
-
-The former has two relevant advantages: It works on file descriptors
-instead of a path name and it has nano second resolution (instead of only
-seconds).
-
-Note, I don't know about Windows which is probably broken by this commit
-as I assume it doesn't support futimens().
----
- attr.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
- 1 file changed, 48 insertions(+), 5 deletions(-)
-
-diff --git a/attr.c b/attr.c
-index 2653ed0a16bb..08c9d24cb446 100644
---- a/attr.c
-+++ b/attr.c
-@@ -265,12 +265,51 @@ post_op_attr get_post_cached(struct svc_req * req)
- return get_post_buf(st_cache, req);
- }
-
-+static nfsstat3 set_time(int fd, sattr3 new)
-+{
-+ if (new.atime.set_it != DONT_CHANGE || new.mtime.set_it != DONT_CHANGE) {
-+
-+ /* atime in t[0], mtime in t[1] */
-+ struct timespec t[2];
-+ int res;
-+
-+ /* compute atime to set */
-+ if (new.atime.set_it == SET_TO_SERVER_TIME) {
-+ t[0].tv_sec = UTIME_NOW;
-+ t[0].tv_nsec = UTIME_NOW;
-+ } else if (new.atime.set_it == SET_TO_CLIENT_TIME) {
-+ t[0].tv_sec = new.atime.set_atime_u.atime.seconds;
-+ t[0].tv_nsec = new.atime.set_atime_u.atime.nseconds;
-+ } else { /* DONT_CHANGE */
-+ t[0].tv_sec = UTIME_OMIT;
-+ t[0].tv_nsec = UTIME_OMIT;
-+ }
-+
-+ /* compute mtime to set */
-+ if (new.mtime.set_it == SET_TO_SERVER_TIME) {
-+ t[1].tv_sec = UTIME_NOW;
-+ t[1].tv_nsec = UTIME_NOW;
-+ } else if (new.mtime.set_it == SET_TO_CLIENT_TIME) {
-+ t[1].tv_sec = new.mtime.set_mtime_u.mtime.seconds;
-+ t[1].tv_nsec = new.mtime.set_mtime_u.mtime.nseconds;
-+ } else { /* DONT_CHANGE */
-+ t[1].tv_sec = UTIME_OMIT;
-+ t[1].tv_nsec = UTIME_OMIT;
-+ }
-+
-+ res = futimens(fd, &t);
-+ if (res == -1)
-+ return setattr_err();
-+ }
-+ return NFS3_OK;
-+}
-+
- /*
- * setting of time, races with local filesystem
- *
- * there is no futimes() function in POSIX or Linux
- */
--static nfsstat3 set_time(const char *path, backend_statstruct buf, sattr3 new)
-+static nfsstat3 set_time_path(const char *path, backend_statstruct buf, sattr3 new)
- {
- time_t new_atime, new_mtime;
- struct utimbuf utim;
-@@ -358,7 +397,7 @@ static nfsstat3 set_attr_unsafe(const char *path, nfs_fh3 nfh, sattr3 new)
- return setattr_err();
- }
-
-- return set_time(path, buf, new);
-+ return set_time_path(path, buf, new);
- }
-
- /*
-@@ -454,14 +493,18 @@ nfsstat3 set_attr(const char *path, nfs_fh3 nfh, sattr3 new)
- }
- }
-
-+ /* finally, set times */
-+ res = set_time(fd, new);
-+ if (res != NFS3_OK) {
-+ backend_close(fd);
-+ return res;
-+ }
-+
- res = backend_close(fd);
- if (res == -1) {
- /* error on close probably means attributes didn't make it */
- return NFS3ERR_IO;
- }
--
-- /* finally, set times */
-- return set_time(path, buf, new);
- }
-
- /*
diff --git a/patches/unfs3-0.9.22/0006-attr-make-use-of-O_PATH-to-set-attributes-of-symlink.patch b/patches/unfs3-0.9.22/0006-attr-make-use-of-O_PATH-to-set-attributes-of-symlink.patch
deleted file mode 100644
index 7e38c549dd77..000000000000
--- a/patches/unfs3-0.9.22/0006-attr-make-use-of-O_PATH-to-set-attributes-of-symlink.patch
+++ /dev/null
@@ -1,81 +0,0 @@
-From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
-Date: Tue, 18 Feb 2020 15:23:52 +0100
-Subject: [PATCH] attr: make use of O_PATH to set attributes of symlinks and
- devices
-
-This fixes setting atime/mtime for symlinks which before this patch changed
-the timestamps for the symlink's target (and resulted in an error if the
-symlink is dangling).
----
- attr.c | 24 ++++++++++++++++++++++--
- 1 file changed, 22 insertions(+), 2 deletions(-)
-
-diff --git a/attr.c b/attr.c
-index 08c9d24cb446..ae2a759e9c86 100644
---- a/attr.c
-+++ b/attr.c
-@@ -304,6 +304,7 @@ static nfsstat3 set_time(int fd, sattr3 new)
- return NFS3_OK;
- }
-
-+#ifndef O_PATH
- /*
- * setting of time, races with local filesystem
- *
-@@ -399,6 +400,7 @@ static nfsstat3 set_attr_unsafe(const char *path, nfs_fh3 nfh, sattr3 new)
-
- return set_time_path(path, buf, new);
- }
-+#endif /* #ifndef O_PATH */
-
- /*
- * set attributes of an object
-@@ -410,13 +412,29 @@ nfsstat3 set_attr(const char *path, nfs_fh3 nfh, sattr3 new)
- uid_t new_uid;
- gid_t new_gid;
- backend_statstruct buf;
-+#ifdef O_PATH
-+ int flags = O_PATH | O_NOFOLLOW;
-+#endif
-
- res = backend_lstat(path, &buf);
- if (res != 0)
- return NFS3ERR_STALE;
-
-+#ifdef O_PATH
-+
-+ if (new.size.set_it == TRUE) {
-+ if (!S_ISREG(buf.st_mode))
-+ return NFS3ERR_NOTSUPP;
-+ flags = O_WRONLY | O_NONBLOCK;
-+ }
-+
-+ fd = backend_open(path, flags);
-+ if (fd < 0)
-+ return NFS3ERR_STALE;
-+
-+#else
- /*
-- * don't open(2) device nodes, it could trigger
-+ * don't open(2) device nodes without O_PATH, it could trigger
- * module loading on the server
- */
- if (S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode))
-@@ -424,7 +442,7 @@ nfsstat3 set_attr(const char *path, nfs_fh3 nfh, sattr3 new)
-
- #ifdef S_ISLNK
- /*
-- * opening a symlink would open the underlying file,
-+ * opening a symlink without O_PATH would open the underlying file,
- * don't try to do that
- */
- if (S_ISLNK(buf.st_mode))
-@@ -441,6 +459,8 @@ nfsstat3 set_attr(const char *path, nfs_fh3 nfh, sattr3 new)
- if (fd == -1)
- return set_attr_unsafe(path, nfh, new);
-
-+#endif /* #ifdef O_PATH / else */
-+
- res = backend_fstat(fd, &buf);
- if (res == -1) {
- backend_close(fd);
diff --git a/patches/unfs3-0.9.22/series b/patches/unfs3-0.9.22/series
index deda41f14aef..95530189b4e7 100644
--- a/patches/unfs3-0.9.22/series
+++ b/patches/unfs3-0.9.22/series
@@ -4,6 +4,5 @@
0002-fix-unfs3-build-with-recent-versions-of-flex.patch
0003-64-bit-fixes-don-t-assume-that-long-is-the-same-size.patch
0004-use-libtirpc-if-found.patch
-0005-attr-use-futimens-instead-of-utime.patch
-0006-attr-make-use-of-O_PATH-to-set-attributes-of-symlink.patch
-# 0534e0228eabedcabb5e164a284ad3f5 - git-ptx-patches magic
+0005-attr-Don-t-open-a-file-to-set-its-attribute.patch
+# 1e81c166e198bc82d019b3eba0d7bb14 - git-ptx-patches magic
--
2.25.0
_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
reply other threads:[~2020-02-27 13:21 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
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=20200227132137.21330-1-u.kleine-koenig@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=ptxdist@pengutronix.de \
/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