From: Seth Forshee seth.forshee@canonical.com Subject: tmpfs: disallow CONFIG_TMPFS_INODE64 on alpha
As with s390, alpha is a 64-bit architecture with a 32-bit ino_t. With CONFIG_TMPFS_INODE64=y tmpfs mounts will get 64-bit inode numbers and display "inode64" in the mount options, whereas passing "inode64" in the mount options will fail. This leads to erroneous behaviours such as this:
# mkdir mnt # mount -t tmpfs nodev mnt # mount -o remount,rw mnt mount: /home/ubuntu/mnt: mount point not mounted or bad option.
Prevent CONFIG_TMPFS_INODE64 from being selected on alpha.
Link: https://lkml.kernel.org/r/20210208215726.608197-1-seth.forshee@canonical.com Fixes: ea3271f7196c ("tmpfs: support 64-bit inums per-sb") Signed-off-by: Seth Forshee seth.forshee@canonical.com Acked-by: Hugh Dickins hughd@google.com Cc: Chris Down chris@chrisdown.name Cc: Amir Goldstein amir73il@gmail.com Cc: Richard Henderson rth@twiddle.net Cc: Ivan Kokshaysky ink@jurassic.park.msu.ru Cc: Matt Turner mattst88@gmail.com Cc: stable@vger.kernel.org [5.9+] Signed-off-by: Andrew Morton akpm@linux-foundation.org ---
fs/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
--- a/fs/Kconfig~tmpfs-disallow-config_tmpfs_inode64-on-alpha +++ a/fs/Kconfig @@ -203,7 +203,7 @@ config TMPFS_XATTR
config TMPFS_INODE64 bool "Use 64-bit ino_t by default in tmpfs" - depends on TMPFS && 64BIT && !S390 + depends on TMPFS && 64BIT && !(S390 || ALPHA) default n help tmpfs has historically used only inode numbers as wide as an unsigned _
On Tue, Feb 9, 2021 at 1:42 PM Andrew Morton akpm@linux-foundation.org wrote:
As with s390, alpha is a 64-bit architecture with a 32-bit ino_t. With CONFIG_TMPFS_INODE64=y tmpfs mounts will get 64-bit inode numbers and display "inode64" in the mount options, whereas passing "inode64" in the mount options will fail.
Ugh.
The two patches for s390 and alpha are obviously the right thing to do, but I do wonder if we could strive to make __kernel_ino_t go away entirely.
It's actually not used very much, because it's such a nasty type, and s390 and alpha are the only ones that override it from the default "word length" version (and honestly, even that default is not a great type).
The main use of it is for "ino_t" and for "struct ustat".
And yes, "ino_t" is widely used, but I think pretty much all uses of it are entirely internal to the kernel, and we could just make it be "unsigned long".
Does anybody see any actual user interfaces that depend on "__kernel_ino_t", aka "ino_t" (apart from that "struct ustat")?
I guess this is mostly a question for s390, which is actively maintained?
Linus
On Tue, Feb 09, 2021 at 02:03:19PM -0800, Linus Torvalds wrote:
On Tue, Feb 9, 2021 at 1:42 PM Andrew Morton akpm@linux-foundation.org wrote:
As with s390, alpha is a 64-bit architecture with a 32-bit ino_t. With CONFIG_TMPFS_INODE64=y tmpfs mounts will get 64-bit inode numbers and display "inode64" in the mount options, whereas passing "inode64" in the mount options will fail.
Ugh.
The two patches for s390 and alpha are obviously the right thing to do, but I do wonder if we could strive to make __kernel_ino_t go away entirely.
It's actually not used very much, because it's such a nasty type, and s390 and alpha are the only ones that override it from the default "word length" version (and honestly, even that default is not a great type).
The main use of it is for "ino_t" and for "struct ustat".
And yes, "ino_t" is widely used, but I think pretty much all uses of it are entirely internal to the kernel, and we could just make it be "unsigned long".
Does anybody see any actual user interfaces that depend on "__kernel_ino_t", aka "ino_t" (apart from that "struct ustat")?
I guess this is mostly a question for s390, which is actively maintained?
I couldn't spot any and also gave the patch below a try and my system still boots without any errors. So, as far as I can tell it _should_ be ok to change this.
Note that the unusual 32 bit ino_t also recently caused a bug on s390. See commit ebce3eb2f7ef ("ceph: fix inode number handling on arches with 32-bit ino_t"). So getting rid of this would be a good thing.
diff --git a/arch/Kconfig b/arch/Kconfig index 24862d15f3a3..383c98e86a70 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -327,6 +327,10 @@ config ARCH_32BIT_OFF_T still support 32-bit off_t. This option is enabled for all such architectures explicitly.
+# Selected by 64 bit architectures which have a 32 bit f_tinode in struct ustat +config ARCH_32BIT_USTAT_F_TINODE + bool + config HAVE_ASM_MODVERSIONS bool help diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig index 1f51437d5765..96ce6565890e 100644 --- a/arch/alpha/Kconfig +++ b/arch/alpha/Kconfig @@ -2,6 +2,7 @@ config ALPHA bool default y + select ARCH_32BIT_USTAT_F_TINODE select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO select ARCH_NO_PREEMPT diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index c72874f09741..434efd9ca0c5 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -58,6 +58,7 @@ config S390 # Note: keep this list sorted alphabetically # imply IMA_SECURE_AND_OR_TRUSTED_BOOT + select ARCH_32BIT_USTAT_F_TINODE select ARCH_BINFMT_ELF_STATE select ARCH_HAS_DEBUG_VM_PGTABLE select ARCH_HAS_DEBUG_WX diff --git a/fs/statfs.c b/fs/statfs.c index 68cb07788750..0ba34c135593 100644 --- a/fs/statfs.c +++ b/fs/statfs.c @@ -255,7 +255,10 @@ SYSCALL_DEFINE2(ustat, unsigned, dev, struct ustat __user *, ubuf)
memset(&tmp,0,sizeof(struct ustat)); tmp.f_tfree = sbuf.f_bfree; - tmp.f_tinode = sbuf.f_ffree; + if (IS_ENABLED(CONFIG_ARCH_32BIT_USTAT_F_TINODE)) + tmp.f_tinode = min_t(u64, sbuf.f_ffree, UINT_MAX); + else + tmp.f_tinode = sbuf.f_ffree;
return copy_to_user(ubuf, &tmp, sizeof(struct ustat)) ? -EFAULT : 0; } diff --git a/include/linux/types.h b/include/linux/types.h index a147977602b5..1e9d0a2c1dba 100644 --- a/include/linux/types.h +++ b/include/linux/types.h @@ -14,7 +14,7 @@ typedef u32 __kernel_dev_t;
typedef __kernel_fd_set fd_set; typedef __kernel_dev_t dev_t; -typedef __kernel_ino_t ino_t; +typedef __kernel_ulong_t ino_t; typedef __kernel_mode_t mode_t; typedef unsigned short umode_t; typedef u32 nlink_t; @@ -189,7 +189,11 @@ struct hlist_node {
struct ustat { __kernel_daddr_t f_tfree; - __kernel_ino_t f_tinode; +#ifdef ARCH_HAS_32BIT_F_TINODE + unsigned int f_tinode; +#else + unsigned long f_tinode; +#endif char f_fname[6]; char f_fpack[6]; };
On Wed, Feb 10, 2021 at 02:34:08PM +0100, Heiko Carstens wrote:
diff --git a/include/linux/types.h b/include/linux/types.h index a147977602b5..1e9d0a2c1dba 100644 --- a/include/linux/types.h +++ b/include/linux/types.h @@ -14,7 +14,7 @@ typedef u32 __kernel_dev_t; typedef __kernel_fd_set fd_set; typedef __kernel_dev_t dev_t; -typedef __kernel_ino_t ino_t; +typedef __kernel_ulong_t ino_t; typedef __kernel_mode_t mode_t; typedef unsigned short umode_t; typedef u32 nlink_t; @@ -189,7 +189,11 @@ struct hlist_node { struct ustat { __kernel_daddr_t f_tfree;
- __kernel_ino_t f_tinode;
+#ifdef ARCH_HAS_32BIT_F_TINODE
- unsigned int f_tinode;
+#else
- unsigned long f_tinode;
+#endif
Of course that should have been CONFIG_ARCH_32BIT_USTAT_F_TINODE in order to not break the existing ABI for alpha and s390.
On Wed, Feb 10, 2021 at 5:39 AM Heiko Carstens hca@linux.ibm.com wrote:
I couldn't spot any and also gave the patch below a try and my system still boots without any errors. So, as far as I can tell it _should_ be ok to change this.
So your patch (with the fix on top) looks sane to me.
I'm not entirely sure it is worth it, but the fact that we've had bugs wrt this before does seem to imply that we should do this.
I'd remove the __kernel_ino_t type entirely, but I wonder if user space might depend on it. I do find
#ifndef __kernel_ino_t typedef __kernel_ulong_t __kernel_ino_t; #endif
in the GNU libc headers I have, but then I don't find any actual use of that, so it looks like it may be jyst a "we copied things for other reasons".
On the whole I think this would be the right thing to do, but I'm a bit worried that it's more pain that it might be worth.
Heiko, I think I'll leave this decision entirely to you. If you think it's worth it to avoid any possible future pain wrt this odd inode number thing for s390, just add it to the s390 tree with my ack. Because honestly, I think s390 is the only architecture that really cares by now.
Linus
On Wed, Feb 10, 2021 at 8:17 PM Linus Torvalds torvalds@linux-foundation.org wrote:
On Wed, Feb 10, 2021 at 5:39 AM Heiko Carstens hca@linux.ibm.com wrote:
I couldn't spot any and also gave the patch below a try and my system still boots without any errors. So, as far as I can tell it _should_ be ok to change this.
So your patch (with the fix on top) looks sane to me.
I'm not entirely sure it is worth it, but the fact that we've had bugs wrt this before does seem to imply that we should do this.
I'd remove the __kernel_ino_t type entirely, but I wonder if user space might depend on it. I do find
#ifndef __kernel_ino_t typedef __kernel_ulong_t __kernel_ino_t; #endif
in the GNU libc headers I have, but then I don't find any actual use of that, so it looks like it may be jyst a "we copied things for other reasons".
I checked debian codesearch to see if there are any users in distro source code and found exactly one instance that will definitely break at compile time:
https://sources.debian.org/src/nfs-utils/1:1.3.4-4/support/include/nfs/nfs.h...
This is a copy of a kernel header that was removed ten years ago with commit c152292f9ee7 ("nfsd: remove include/linux/nfsd/syscall.h").
The mainline version of that package removed the contents in 2016 in the following release (2.1.1), but debian is still on the previous version (1.3.4) http://git.linux-nfs.org/?p=steved/nfs-utils.git%3Ba=commitdiff%3Bh=fc1127d7...
Someone will have to update the package for Debian, but it seems that would be a good idea anyway.
Arnd
On Wed, Feb 10, 2021 at 11:17:10AM -0800, Linus Torvalds wrote:
On Wed, Feb 10, 2021 at 5:39 AM Heiko Carstens hca@linux.ibm.com wrote:
I couldn't spot any and also gave the patch below a try and my system still boots without any errors. So, as far as I can tell it _should_ be ok to change this.
So your patch (with the fix on top) looks sane to me.
I'm not entirely sure it is worth it, but the fact that we've had bugs wrt this before does seem to imply that we should do this.
I'd remove the __kernel_ino_t type entirely, but I wonder if user space might depend on it. I do find
#ifndef __kernel_ino_t typedef __kernel_ulong_t __kernel_ino_t; #endif
in the GNU libc headers I have, but then I don't find any actual use of that, so it looks like it may be jyst a "we copied things for other reasons".
On the whole I think this would be the right thing to do, but I'm a bit worried that it's more pain that it might be worth.
Heiko, I think I'll leave this decision entirely to you. If you think it's worth it to avoid any possible future pain wrt this odd inode number thing for s390, just add it to the s390 tree with my ack. Because honestly, I think s390 is the only architecture that really cares by now.
So, yes. We will go to change this to hopefully avoid future problems. The patch is supposed to be part of the next merge window and converts both s390 and alpha, unless somebody objects.
After that has been merged I'll provide a follow-on patch which enables TMPFS_INODE64 for alpha and s390 again, and yet another one which removes __kernel_ino_t as suggested by you.
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Arnd Bergmann -
Heiko Carstens -
Linus Torvalds