Re: fstatat + AT_NO_AUTOMOUNT
On Mär 09 2022, Paul Eggert wrote: > I audited gnulib's uses of fstatat and found one fishy one that doesn't > use AT_NO_AUTOMOUNT, namely, in fts.c where the follow-symlink branch uses > 'stat' whereas the no-follow-symlink branch uses fstatat without > AT_NO_AUTOMOUNT. I installed the first patch to cause it be consistent in > using AT_NO_AUTOMOUNT, which is also consistent with what glibc does ??? In glibc, stat is the same as fstatat(,,,0). -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: fstatat + AT_NO_AUTOMOUNT
I audited gnulib's uses of fstatat and found one fishy one that doesn't
use AT_NO_AUTOMOUNT, namely, in fts.c where the follow-symlink branch
uses 'stat' whereas the no-follow-symlink branch uses fstatat without
AT_NO_AUTOMOUNT. I installed the first patch to cause it be consistent
in using AT_NO_AUTOMOUNT, which is also consistent with what glibc does
(though this doesn't necessarily mean it's right - perhaps fts should
have a new flag to control automounts, depending on what the user wants).
The second attached patch deprecates statat and lstatat, due to the
confusion already mentioned.
I haven't audited Gnulib's uses of 'stat' and 'lstat'.From 44f347ce4009cd0100d0e6562939a032b16d6db1 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 9 Mar 2022 11:54:13 -0800
Subject: [PATCH 1/2] fts: be consistent about AT_NO_AUTOMOUNT
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/fts.c (fts_stat): Use fstatat with AT_NO_AUTOMOUNT
consistently, instead of sometimes using stat (which implies
AT_NO_AUTOMOUNT) and sometimes using fstatat without AT_NO_AUTOMOUNT.
Remove a goto while we’re at it.
---
ChangeLog | 8
lib/fts.c | 34 +-
2 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index e3f0ed216c..58873a1762 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2022-03-09 Paul Eggert
+
+ fts: be consistent about AT_NO_AUTOMOUNT
+ * lib/fts.c (fts_stat): Use fstatat with AT_NO_AUTOMOUNT
+ consistently, instead of sometimes using stat (which implies
+ AT_NO_AUTOMOUNT) and sometimes using fstatat without AT_NO_AUTOMOUNT.
+ Remove a goto while we’re at it.
+
2022-03-07 Pádraig Brady
fcntl-h: add AT_NO_AUTOMOUNT
diff --git a/lib/fts.c b/lib/fts.c
index 706c56c597..a1a7c09fdb 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -1766,7 +1766,8 @@ fts_stat(FTS *sp, register FTSENT *p, bool follow)
{
struct stat *sbp = p->fts_statp;
-if (p->fts_level == FTS_ROOTLEVEL && ISSET(FTS_COMFOLLOW))
+if (ISSET (FTS_LOGICAL)
+|| (ISSET (FTS_COMFOLLOW) && p->fts_level == FTS_ROOTLEVEL))
follow = true;
/*
@@ -1774,22 +1775,21 @@ fts_stat(FTS *sp, register FTSENT *p, bool follow)
* a stat(2). If that fails, check for a non-existent symlink. If
* fail, set the errno from the stat call.
*/
-if (ISSET(FTS_LOGICAL) || follow) {
-if (stat(p->fts_accpath, sbp)) {
-if (errno == ENOENT
-&& lstat(p->fts_accpath, sbp) == 0) {
-__set_errno (0);
-return (FTS_SLNONE);
-}
-p->fts_errno = errno;
-goto err;
-}
-} else if (fstatat(sp->fts_cwd_fd, p->fts_accpath, sbp,
- AT_SYMLINK_NOFOLLOW)) {
-p->fts_errno = errno;
-err:memset(sbp, 0, sizeof(struct stat));
-return (FTS_NS);
-}
+int flags = (follow ? 0 : AT_SYMLINK_NOFOLLOW) | AT_NO_AUTOMOUNT;
+if (fstatat (sp->fts_cwd_fd, p->fts_accpath, sbp, flags) < 0)
+ {
+if (follow && errno == ENOENT
+&& 0 <= fstatat (sp->fts_cwd_fd, p->fts_accpath, sbp,
+ AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT))
+ {
+__set_errno (0);
+return FTS_SLNONE;
+ }
+
+p->fts_errno = errno;
+memset (sbp, 0, sizeof *sbp);
+return FTS_NS;
+ }
if (S_ISDIR(sbp->st_mode)) {
if (ISDOT(p->fts_name)) {
--
2.35.1
From eea9688d521634c58efa81130e509f647bbd9ff9 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 9 Mar 2022 13:54:53 -0800
Subject: [PATCH 2/2] statat: now obsolete
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/openat.h (statat, lstatat): Now deprecated.
All uses removed, and replaced with fstatat.
* modules/statat: Mark as obsolete, because it’s confusing:
it’s not clear whether it should use AT_NO_AUTOMOUNT,
which is implied by stat and by lstat, but not by fstatat.
* tests/test-statat.c: Disable deprecated-declarations warnings.
---
ChangeLog | 8
NEWS| 3 +++
lib/fchownat.c | 2 +-
lib/openat.h| 2 ++
lib/renameatu.c | 15 ---
lib/unlinkat.c | 5 +++--
modules/fchownat| 1 -
modules/renameatu | 1 -
modules/statat | 7 +++
modules/unlinkat| 1 -
tests/test-statat.c | 4
11 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 58873a1762..294f6286f3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
2022-03-09 Paul Eggert
+ statat: now obsolete
+ * lib/openat.h (statat,
Re: fstatat + AT_NO_AUTOMOUNT
On 3/8/22 18:44, Paul Eggert wrote:
> On 3/8/22 04:29, Bernhard Voelker wrote:
>> I'm not so sure about that: at least in container environment it seems to be
>> common practice to mount regular files somewhere into the container, e.g.:
>
> Oh, I wasn't aware of that. However, don't they mount a regular file
> atop an already-existing regular file? In that case, the optimization
> would still be valid.
Indeed, the GNU/Linux kernel does not allow to bind-mount a file atop of a
directory:
$ strace -ve mount mount --bind /etc/hosts dir
mount("/etc/hosts", "/root/dir", 0x557e7ed77570, MS_BIND, NULL) = -1 ENOTDIR
(Not a directory)
mount: /root/dir: mount(2) system call failed: Not a directory.
+++ exited with 32 +++
The kernel doesn't seem so picky about other target file types;
e.g. it allows to bind-mount atop of a character device:
$ mknod char c 1 3
$ ls -log x
crw-r--r-- 1 1, 3 Mar 9 00:51 char
$ mount --bind /etc/hosts char
$ ls -log char
-rw-r--r-- 1 1515 Jan 16 14:55 char
Have a nice day,
Berny
Re: fstatat + AT_NO_AUTOMOUNT
On 3/8/22 04:29, Bernhard Voelker wrote: I'm not so sure about that: at least in container environment it seems to be common practice to mount regular files somewhere into the container, e.g.: Oh, I wasn't aware of that. However, don't they mount a regular file atop an already-existing regular file? In that case, the optimization would still be valid.
Re: fstatat + AT_NO_AUTOMOUNT
On 3/7/22 20:09, Paul Eggert wrote: > Although it's possible to mount onto a > non-directory, this is bad practice and I don't think we need to worry > about that. I'm not so sure about that: at least in container environment it seems to be common practice to mount regular files somewhere into the container, e.g.: [root@7ce1ad192b6d /]# findmnt /etc/hosts -o target,source TARGET SOURCE /etc/hosts /dev/sda5[/docker/containers/7ce1ad192b6dc0e265e34d31c8a1ac22e878a751d9d810356eecdc77002e8013/hosts] Have a nice day, Berny
Re: fstatat + AT_NO_AUTOMOUNT
On 3/7/22 15:23, Pádraig Brady wrote: * I don't yet see any other use for AT_NO_AUTOMOUNT. Apart from the ls(1) and stat(1) cases previously mentioned I presume. I'll push those patches now. Yes, thanks.
Re: fstatat + AT_NO_AUTOMOUNT
On 07/03/2022 22:36, Paul Eggert wrote: On 3/7/22 13:56, Pádraig Brady wrote: $ git show 571f63f50 | grep -B3 fstatat Ah, I now see the problem: I suppressed some of GitHub's JavaScript code when looking at the URL you sent. I'll have to remember not to do that. Or add a .patch suffix to the URL After thinking about it a bit more, how about the following idea? * Normally, do not use AT_NO_AUTOMOUNT. * If a file is known to exist but its type is not known, it's OK to use fstatat with AT_NO_AUTOMOUNT to determine the file's type, under the theory that mounting should merely replace directories with directories and so won't change the type, so that AT_NO_AUTOMOUNT is merely an optimization here. A use case might be fts.c wanting to know whether a directory entry with type DT_UNKNOWN is itself a directory. Right. * I don't yet see any other use for AT_NO_AUTOMOUNT. Apart from the ls(1) and stat(1) cases previously mentioned I presume. I'll push those patches now. 'cp -x' is an example where we can't use AT_NO_AUTOMOUNT, since cp needs to read the source file (not just stat it) and so we want the automount so that the stat result is consistent with the other accesses. 'du -x' is a more-plausible use of AT_NO_AUTOMOUNT but I don't see how it could work correctly. 'df' likely needs to use both fstatat and statvfs and the latter lacks AT_NO_AUTOMOUNT, which means the former shouldn't use AT_NO_AUTOMOUNT. Yes I agree with the above analysis. thanks for the consideration. Pádraig
Re: fstatat + AT_NO_AUTOMOUNT
On 3/7/22 13:56, Pádraig Brady wrote: $ git show 571f63f50 | grep -B3 fstatat Ah, I now see the problem: I suppressed some of GitHub's JavaScript code when looking at the URL you sent. I'll have to remember not to do that. After thinking about it a bit more, how about the following idea? * Normally, do not use AT_NO_AUTOMOUNT. * If a file is known to exist but its type is not known, it's OK to use fstatat with AT_NO_AUTOMOUNT to determine the file's type, under the theory that mounting should merely replace directories with directories and so won't change the type, so that AT_NO_AUTOMOUNT is merely an optimization here. A use case might be fts.c wanting to know whether a directory entry with type DT_UNKNOWN is itself a directory. * I don't yet see any other use for AT_NO_AUTOMOUNT. 'cp -x' is an example where we can't use AT_NO_AUTOMOUNT, since cp needs to read the source file (not just stat it) and so we want the automount so that the stat result is consistent with the other accesses. 'du -x' is a more-plausible use of AT_NO_AUTOMOUNT but I don't see how it could work correctly. 'df' likely needs to use both fstatat and statvfs and the latter lacks AT_NO_AUTOMOUNT, which means the former shouldn't use AT_NO_AUTOMOUNT.
Re: fstatat + AT_NO_AUTOMOUNT
On 07/03/2022 19:09, Paul Eggert wrote: On 3/7/22 09:33, Pádraig Brady wrote: On 07/03/2022 17:11, Paul Eggert wrote: On 3/7/22 07:02, Pádraig Brady wrote: When looking into https://bugs.gnu.org/54286 which discussed statx + AT_NO_AUTOMOUNT, I was wondering about the fstatat changes in https://github.com/coreutils/coreutils/commit/571f63f50 and whether some of those should also specify AT_NO_AUTOMOUNT? I'm a bit lost, as I see no mention of fstatat in that commit. That commit replaced some stat() calls with fstatat(). How did it do that? Sorry, I'm still lost: the commit doesn't say "fstatat" anywhere. $ git show 571f63f50 | grep -B3 fstatat -(dereference_dest_dir_symlinks ? stat (file, ) : lstat (file, )); - int err = (stat_result == 0 ? 0 : errno); + int flags = dereference_dest_dir_symlinks ? 0 : AT_SYMLINK_NOFOLLOW; + int err = errnoize (fstatat (AT_FDCWD, file, , flags)); -- - if ((logical ? stat (source, _stats) - : lstat (source, _stats)) - != 0) + source_status = fstatat (AT_FDCWD, source, _stats, nofollow_flag); Attached is an adjustment for ln in coreutils at least. I see a problem with that change. Suppose D does not exist, but is an on-demand directory such as used for automounter indirect maps. Then "ln -f S D" will use fstatat with AT_NO_AUTOMOUNT on D, and will assume that D does not exist. I can see where AT_NO_AUTOMOUNT might be useful for commands like 'find' that do mass lstats or commands like 'df' designed to access mountpoints, but why is it useful for 'ln'? It seems like it'd be more trouble than it's worth. Agreed. As a rule of thumb I guess for stateful commands it would be best not to use AT_NO_AUTOMOUNT I see unlinkat() uses lstatat(), and in that context it would be better to specify AT_NO_AUTOMOUNT I think since we're operating on the dir not the file itself. If it weren't for on-demand directories I would agree with AT_NO_AUTOMOUNT there, but I think the reason would be different. Ordinarily mount points are directories, and so automounting replaces one directory with another. Since gnulib/lib/unlinkat.c is trying to determine whether it's unlinking a directory, mounting won't change that, so there's no point to mounting, and AT_NO_AUTOMOUNT is a performance win there. (Although it's possible to mount onto a non-directory, this is bad practice and I don't think we need to worry about that.) Unfortunately, on-demand directories mess up the calculation, since they replace nonexistent files with directories. So it appears that AT_NO_AUTOMOUNT would not be an acceptable optimization in gnulib/lib/unlinkat.c, since it would mess up in that case. agreed Also agreed that eliminating these aliases would avoid at least this particular ambiguity. Yes, that looks like a win (unfortunately, as the aliases do make things shorter; but they're too confusing now). cheers, Pádraig
Re: fstatat + AT_NO_AUTOMOUNT
On 3/7/22 09:33, Pádraig Brady wrote: On 07/03/2022 17:11, Paul Eggert wrote: On 3/7/22 07:02, Pádraig Brady wrote: When looking into https://bugs.gnu.org/54286 which discussed statx + AT_NO_AUTOMOUNT, I was wondering about the fstatat changes in https://github.com/coreutils/coreutils/commit/571f63f50 and whether some of those should also specify AT_NO_AUTOMOUNT? I'm a bit lost, as I see no mention of fstatat in that commit. That commit replaced some stat() calls with fstatat(). How did it do that? Sorry, I'm still lost: the commit doesn't say "fstatat" anywhere. Attached is an adjustment for ln in coreutils at least. I see a problem with that change. Suppose D does not exist, but is an on-demand directory such as used for automounter indirect maps. Then "ln -f S D" will use fstatat with AT_NO_AUTOMOUNT on D, and will assume that D does not exist. I can see where AT_NO_AUTOMOUNT might be useful for commands like 'find' that do mass lstats or commands like 'df' designed to access mountpoints, but why is it useful for 'ln'? It seems like it'd be more trouble than it's worth. I see unlinkat() uses lstatat(), and in that context it would be better to specify AT_NO_AUTOMOUNT I think since we're operating on the dir not the file itself. If it weren't for on-demand directories I would agree with AT_NO_AUTOMOUNT there, but I think the reason would be different. Ordinarily mount points are directories, and so automounting replaces one directory with another. Since gnulib/lib/unlinkat.c is trying to determine whether it's unlinking a directory, mounting won't change that, so there's no point to mounting, and AT_NO_AUTOMOUNT is a performance win there. (Although it's possible to mount onto a non-directory, this is bad practice and I don't think we need to worry about that.) Unfortunately, on-demand directories mess up the calculation, since they replace nonexistent files with directories. So it appears that AT_NO_AUTOMOUNT would not be an acceptable optimization in gnulib/lib/unlinkat.c, since it would mess up in that case. Also agreed that eliminating these aliases would avoid at least this particular ambiguity. Yes, that looks like a win (unfortunately, as the aliases do make things shorter; but they're too confusing now).
Re: fstatat + AT_NO_AUTOMOUNT
On 07/03/2022 17:11, Paul Eggert wrote:
On 3/7/22 07:02, Pádraig Brady wrote:
When looking into https://bugs.gnu.org/54286
which discussed statx + AT_NO_AUTOMOUNT,
I was wondering about the fstatat changes in
https://github.com/coreutils/coreutils/commit/571f63f50
and whether some of those should also specify AT_NO_AUTOMOUNT?
I'm a bit lost, as I see no mention of fstatat in that commit.
That commit replaced some stat() calls with fstatat().
I was not aware of AT_NO_AUTOMOUNT until now, and am surprised about it.
POSIX says that stat(f,s) is supposed to be equivalent to
fstatat(AT_FDCWD,f,s,0). But if I understand things correctly, this
isn't true on GNU/Linux: you must pass AT_NO_AUTOMOUNT instead of 0. I
assume that this departure from POSIX was intentional, but even so it's
pretty confusing. Could you explain why AT_NO_AUTOMOUNT wasn't made the
default for fstatat?
Yes this is surprising.
I'm only going by the fstatat man page,
but I also see correlation of the issue in for e.g.:
https://bugs.python.org/issue35677
I've yet to dig into the original kernel discussions and implementation.
I suppose we first need to audit all of Gnulib's uses of fstatat and
statx to see whether they need AT_NO_AUTOMOUNT added. Once we're done
with that we can turn to Coreutils.
Yes I expect we'll have different treatments in different places.
Attached is an adjustment for ln in coreutils at least.
For example, what about the calls to fstatat in gnulib/lib/openat.h? Are
statat and lstatat intended as convenient aliases for fstatat, or as
directory-fd extensions to stat and lstat? If the former, they should be
left alone; if the latter, they need AT_NO_AUTOMOUNT. Or perhaps it
would be better to eliminate them and replace them with calls to
fstatat, since we'll need to decide on a call-by-call basis anyway.
I see unlinkat() uses lstatat(),
and in that context it would be better to specify AT_NO_AUTOMOUNT
I think since we're operating on the dir not the file itself.
Also agreed that eliminating these aliases
would avoid at least this particular ambiguity.
Another example: what about the fstatat call in gnulib/lib/fchmodat.c? I
suppose we should not add AT_NO_AUTOMOUNT there, under the theory that
GNU/Linux faccessat does not do the equivalent of AT_NO_AUTOMOUNT. Is
that right?
Yes the flag is not mentioned wrt that call,
I've yet to audit kernel code.
cheers,
PádraigFrom 1f5aa31f4787f4c585d663b56606ac6e5b068e4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?=
Date: Mon, 7 Mar 2022 17:18:49 +
Subject: [PATCH] ln: avoid triggering automounts
* src/ln.c (do_link): Always pass AT_NO_AUTOMOUNT to fstatat().
NEWS: Mention the fix.
---
NEWS | 8
src/ln.c | 6 --
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/NEWS b/NEWS
index 8e95be755..f57ca004b 100644
--- a/NEWS
+++ b/NEWS
@@ -34,10 +34,10 @@ GNU coreutils NEWS-*- outline -*-
and the documentation has been clarified for unusual cases.
[bug introduced in coreutils-7.0]
- ls and stat no longer try to automount files, reverting to the behavior
- before the statx() call was introduced. Only `stat --cached=never`
- will continue to automount files.
- [bug introduced in coreutils-8.32]
+ ln, ls, and stat no longer try to automount files, reverting to the behavior
+ before the statx() and fstatat() calls were introduced.
+ Only `stat --cached=never` will continue to automount files.
+ [bug introduced in ln in coreutils-8.31, and ls and stat in coreutils-8.32]
On macOS, 'mv A B' no longer fails with "Operation not supported"
when A and B are in the same tmpfs file system.
diff --git a/src/ln.c b/src/ln.c
index bb4695853..6b9a1ec76 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -192,7 +192,8 @@ do_link (char const *source, int destdir_fd, char const *dest_base,
diagnostics. */
if ((link_errno || dest_set) && !symbolic_link)
{
- source_status = fstatat (AT_FDCWD, source, _stats, nofollow_flag);
+ source_status = fstatat (AT_FDCWD, source, _stats,
+ nofollow_flag | AT_NO_AUTOMOUNT);
if (source_status != 0)
{
error (0, errno, _("failed to access %s"), quoteaf (source));
@@ -217,7 +218,8 @@ do_link (char const *source, int destdir_fd, char const *dest_base,
if (force)
{
struct stat dest_stats;
- if (fstatat (destdir_fd, dest_base, _stats, AT_SYMLINK_NOFOLLOW)
+ if (fstatat (destdir_fd, dest_base, _stats,
+ AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT)
!= 0)
{
if (errno != ENOENT)
--
2.26.2
Re: fstatat + AT_NO_AUTOMOUNT
On 3/7/22 07:02, Pádraig Brady wrote: When looking into https://bugs.gnu.org/54286 which discussed statx + AT_NO_AUTOMOUNT, I was wondering about the fstatat changes in https://github.com/coreutils/coreutils/commit/571f63f50 and whether some of those should also specify AT_NO_AUTOMOUNT? I'm a bit lost, as I see no mention of fstatat in that commit. I was not aware of AT_NO_AUTOMOUNT until now, and am surprised about it. POSIX says that stat(f,s) is supposed to be equivalent to fstatat(AT_FDCWD,f,s,0). But if I understand things correctly, this isn't true on GNU/Linux: you must pass AT_NO_AUTOMOUNT instead of 0. I assume that this departure from POSIX was intentional, but even so it's pretty confusing. Could you explain why AT_NO_AUTOMOUNT wasn't made the default for fstatat? I suppose we first need to audit all of Gnulib's uses of fstatat and statx to see whether they need AT_NO_AUTOMOUNT added. Once we're done with that we can turn to Coreutils. For example, what about the calls to fstatat in gnulib/lib/openat.h? Are statat and lstatat intended as convenient aliases for fstatat, or as directory-fd extensions to stat and lstat? If the former, they should be left alone; if the latter, they need AT_NO_AUTOMOUNT. Or perhaps it would be better to eliminate them and replace them with calls to fstatat, since we'll need to decide on a call-by-call basis anyway. Another example: what about the fstatat call in gnulib/lib/fchmodat.c? I suppose we should not add AT_NO_AUTOMOUNT there, under the theory that GNU/Linux faccessat does not do the equivalent of AT_NO_AUTOMOUNT. Is that right?
