Message-ID: <9636431303ae3a8b24c84b885cfadcb963124232.camel@sipsolutions.net>
Date: Mon, 06 Oct 2025 23:20:32 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Linus Torvalds <torvalds@...ux-foundation.org>, Chuck Lever
 <cel@...nel.org>,  Christian Brauner	 <brauner@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org, Jeff Layton
	 <jlayton@...nel.org>, linux-um@...ts.infradead.org
Subject: Re: [GIT PULL] NFSD changes for v6.18

On Mon, 2025-10-06 at 13:51 -0700, Linus Torvalds wrote:
> Danger Will Robinson: hostfs has odd duplicate copies of all these, including a
> 
>    #define HOSTFS_ATTR_ATTR_FLAG   1024
> 
> of that no-longer existing flag.
> 
> But hostfs doesn't use ATTR_FORCE (aka HOSTFS_ATTR_FORCE), so
> switching those two bits around wouldn't affect it either, even if you
> were to have a version mismatch between the client and host when doing
> UML (which I don't know
> 
> Adding Christian to the participants list, because I did *not* do that
> cleanup thing myself, because I'm slightly worried that I'm missing
> something. But it would seem to be a good thing to do just to have the
> numbering make more sense, and Christian is probably the right person.

That indeed looks messy in hostfs; I'm not really all _that_ familiar
with all the details of it, but the stated reason is that it needs to
have the defines in kernel and user code.

That doesn't mean it's between the host and guest kernels, it's just
between code built for "userspace" and code built for "kernel", both of
which go into the UML linux "guest" binary. IOW, it's just for
communication between hostfs_kern.c and hostfs_user.c, which nobody else
needs to care about.

And as long as hostfs doesn't propagate CTIME_SET from the host to the
guest, it doesn't need a HOSTFS_ATTR_CTIME_SET. No idea why
HOSTFS_ATTR_ATTR_FLAG was even defined, it's ancient anyway.

However ... it looks like hostfs_kern.c is using ATTR_* in some places,
and hostfs_user.c is using HOSTFS_ATTR_*, so it looks like right now
these *do* need to match. Given that, we should just generate them via
asm-offsets.h, like we do for other constants with the property of being
needed on both sides but defined in places that cannot be included into
user-side code, like this:


From: Johannes Berg <johannes.berg@...el.com>
Date: Mon, 6 Oct 2025 23:14:36 +0200
Subject: [PATCH] um/hostfs: define HOSTFS_ATTR_* via asm-offsets

The HOSTFS_ATTR_* values were meant to be standalone for
communication between hostfs's kernel and user code parts.
However, it's easy to forget that HOSTFS_ATTR_* should be
used even on the kernel side, and that wasn't consistently
done. As a result, the values need to match ATTR_* values,
which is not useful to maintain by hand. Instead, generate
them via asm-offsets like other constants that UML needs
in user-side code that aren't otherwise available in any
header files that can be included there.

Signed-off-by: Johannes Berg <johannes.berg@...el.com>
---
 arch/um/include/shared/common-offsets.h    | 10 +++++++
 arch/x86/um/shared/sysdep/kernel-offsets.h |  1 +
 fs/hostfs/hostfs.h                         | 34 +---------------------
 3 files changed, 12 insertions(+), 33 deletions(-)

diff --git a/arch/um/include/shared/common-offsets.h b/arch/um/include/shared/common-offsets.h
index 8ca66a1918c3..fcec75a93e0c 100644
--- a/arch/um/include/shared/common-offsets.h
+++ b/arch/um/include/shared/common-offsets.h
@@ -18,3 +18,13 @@ DEFINE(UM_NSEC_PER_USEC, NSEC_PER_USEC);
 DEFINE(UM_KERN_GDT_ENTRY_TLS_ENTRIES, GDT_ENTRY_TLS_ENTRIES);
 
 DEFINE(UM_SECCOMP_ARCH_NATIVE, SECCOMP_ARCH_NATIVE);
+
+DEFINE(HOSTFS_ATTR_MODE, ATTR_MODE);
+DEFINE(HOSTFS_ATTR_UID, ATTR_UID);
+DEFINE(HOSTFS_ATTR_GID, ATTR_GID);
+DEFINE(HOSTFS_ATTR_SIZE, ATTR_SIZE);
+DEFINE(HOSTFS_ATTR_ATIME, ATTR_ATIME);
+DEFINE(HOSTFS_ATTR_MTIME, ATTR_MTIME);
+DEFINE(HOSTFS_ATTR_CTIME, ATTR_CTIME);
+DEFINE(HOSTFS_ATTR_ATIME_SET, ATTR_ATIME_SET);
+DEFINE(HOSTFS_ATTR_MTIME_SET, ATTR_MTIME_SET);
diff --git a/arch/x86/um/shared/sysdep/kernel-offsets.h b/arch/x86/um/shared/sysdep/kernel-offsets.h
index 6fd1ed400399..ee6b44ef2217 100644
--- a/arch/x86/um/shared/sysdep/kernel-offsets.h
+++ b/arch/x86/um/shared/sysdep/kernel-offsets.h
@@ -5,6 +5,7 @@
 #include <linux/crypto.h>
 #include <linux/kbuild.h>
 #include <linux/audit.h>
+#include <linux/fs.h>
 #include <asm/mman.h>
 #include <asm/seccomp.h>
 
diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 15b2f094d36e..aa02599b770f 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -3,40 +3,8 @@
 #define __UM_FS_HOSTFS
 
 #include <os.h>
+#include <generated/asm-offsets.h>
 
-/*
- * These are exactly the same definitions as in fs.h, but the names are
- * changed so that this file can be included in both kernel and user files.
- */
-
-#define HOSTFS_ATTR_MODE	1
-#define HOSTFS_ATTR_UID 	2
-#define HOSTFS_ATTR_GID 	4
-#define HOSTFS_ATTR_SIZE	8
-#define HOSTFS_ATTR_ATIME	16
-#define HOSTFS_ATTR_MTIME	32
-#define HOSTFS_ATTR_CTIME	64
-#define HOSTFS_ATTR_ATIME_SET	128
-#define HOSTFS_ATTR_MTIME_SET	256
-
-/* This one is unused by hostfs. */
-#define HOSTFS_ATTR_FORCE	512	/* Not a change, but a change it */
-#define HOSTFS_ATTR_ATTR_FLAG	1024
-
-/*
- * If you are very careful, you'll notice that these two are missing:
- *
- * #define ATTR_KILL_SUID	2048
- * #define ATTR_KILL_SGID	4096
- *
- * and this is because they were added in 2.5 development.
- * Actually, they are not needed by most ->setattr() methods - they are set by
- * callers of notify_change() to notify that the setuid/setgid bits must be
- * dropped.
- * notify_change() will delete those flags, make sure attr->ia_valid & ATTR_MODE
- * is on, and remove the appropriate bits from attr->ia_mode (attr is a
- * "struct iattr *"). -BlaisorBlade
- */
 struct hostfs_timespec {
 	long long tv_sec;
 	long long tv_nsec;
-- 
2.51.0



(that passes my usual tests, if you want you can apply it as is, or I
can resend it as a real patch, or I can also put it into uml-next for
later...)

johannes

Powered by blists - more mailing lists