handling 64bit values for st_ino]

[My apologies, forgot to Cc the first half...]

Date: Thu, 10 Nov 2005 00:27:29 +0000
From: Al Viro <[email protected]>
To: Linus Torvalds <[email protected]>
Subject: [PATCH 1/2] handling 64bit values for st_ino
User-Agent: Mutt/1.4.1i

We certainly do not want 64bit kernel ino_t, since that would
screw icache lookups for no good reason; fs with 64bit keys used to identify inodes can just use iget5().
*However*, there's an area where we have a problem: stat64() and
getdents64() _could_ return 64bit st_ino/d_ino just fine, if not for having too narrow field in kstat and argument of filldir_t. As it is, we have ->getattr() fill struct kstat and the syscall proper copy the contents of that struct kstat into user buffer. stat64 has 64bit st_ino; kstat field used to set it is only unsigned long.
Note that it's not just a theory - there are filesystems that
want 64bit values in st_ino (AFS, for one). There are consumers of these values ready to deal with 64bit values - aside of aforementioned syscalls, e.g. NFSv3 and NFSv4 are happy with those.

Fix is pretty cheap and consists of two parts:
1) widen struct kstat ->ino to u64, add a macro (check_inumber()) to be used in callers of ->getattr() that want to store ->ino in possibly narrower fields and care about overflows (stuff like sys_old_stat() with its 16bit st_ino clearly doesn't ;-)

2) widen ino argument of filldir_t to u64, update prototypes of instances.

Below is the first part (kstat), filldir_t one goes in the next patch.

----

 arch/ia64/ia32/sys_ia32.c         |    3 ++-
 arch/mips/kernel/linux32.c        |    3 +++
 arch/mips/kernel/sysirix.c        |    2 ++
 arch/parisc/hpux/fs.c             |    2 ++
 arch/parisc/kernel/sys_parisc32.c |    3 ++-
 arch/powerpc/kernel/sys_ppc32.c   |    3 ++-
 arch/s390/kernel/compat_linux.c   |    2 ++
 arch/sparc64/kernel/sys_sparc32.c |    3 ++-
 arch/sparc64/solaris/fs.c         |    3 ++-
 arch/x86_64/ia32/sys_ia32.c       |    2 ++
 fs/stat.c                         |    2 ++
 include/linux/stat.h              |   23 ++++++++++++++++++++++-
 12 files changed, 45 insertions(+), 6 deletions(-)

0a5cfa057c7f56cf55cc1611f60b680643e1636b

@@ -130,7 +130,8 @@ int cp_compat_stat(struct kstat *stat, s
 
 	if ((u64) stat->size > MAX_NON_LFS ||
 	    !old_valid_dev(stat->dev) ||
-	    !old_valid_dev(stat->rdev))
+	    !old_valid_dev(stat->rdev) ||
+	    !check_inumber(stat->ino, &ubuf->st_ino))
 		return -EOVERFLOW;
 
 	if (clear_user(ubuf, sizeof(*ubuf)))

@@ -76,6 +76,9 @@ int cp_compat_stat(struct kstat *stat, s
 	if (!new_valid_dev(stat->dev) || !new_valid_dev(stat->rdev))
 		return -EOVERFLOW;
 
+	if (!check_inumber(stat->ino, &tmp.st_ino))
+		return -EOVERFLOW;
+
 	memset(&tmp, 0, sizeof(tmp));
 	tmp.st_dev = new_encode_dev(stat->dev);
 	tmp.st_ino = stat->ino;

@@ -1175,6 +1175,8 @@ static int irix_xstat32_xlate(struct kst
 
 	if (!sysv_valid_dev(stat->dev) || !sysv_valid_dev(stat->rdev))
 		return -EOVERFLOW;
+	if (!check_inumber(stat->ino, &ub.st_ino))
+		return -EOVERFLOW;
 	ub.st_dev     = sysv_encode_dev(stat->dev);
 	ub.st_ino     = stat->ino;
 	ub.st_mode    = stat->mode;

@@ -147,6 +147,8 @@ static int cp_hpux_stat(struct kstat *st
 
 	if (!new_valid_dev(stat->dev) || !new_valid_dev(stat->rdev))
 		return -EOVERFLOW;
+	if (!check_inumber(stat->ino, &tmp.st_ino))
+		return -EOVERFLOW;
 
 	memset(&tmp, 0, sizeof(tmp));
 	tmp.st_dev = new_encode_dev(stat->dev);

@@ -242,7 +242,8 @@ int cp_compat_stat(struct kstat *stat, s
 	int err;
 
 	if (stat->size > MAX_NON_LFS || !new_valid_dev(stat->dev) ||
-	    !new_valid_dev(stat->rdev))
+	    !new_valid_dev(stat->rdev) ||
+	    !check_inumber(stat->ino, &statbuf->st_ino))
 		return -EOVERFLOW;
 
 	err  = put_user(new_encode_dev(stat->dev), &statbuf->st_dev);

@@ -126,7 +126,8 @@ int cp_compat_stat(struct kstat *stat, s
 	long err;
 
 	if (stat->size > MAX_NON_LFS || !new_valid_dev(stat->dev) ||
-	    !new_valid_dev(stat->rdev))
+	    !new_valid_dev(stat->rdev) ||
+	    !check_inumber(stat->ino, &statbuf->st_ino))
 		return -EOVERFLOW;
 
 	err  = access_ok(VERIFY_WRITE, statbuf, sizeof(*statbuf)) ? 0 : -EFAULT;

@@ -362,6 +362,8 @@ int cp_compat_stat(struct kstat *stat, s
 
 	if (!old_valid_dev(stat->dev) || !old_valid_dev(stat->rdev))
 		return -EOVERFLOW;
+	if (!check_inumber(stat->ino, &statbuf->st_ino))
+		return -EOVERFLOW;
 
 	err = put_user(old_encode_dev(stat->dev), &statbuf->st_dev);
 	err |= put_user(stat->ino, &statbuf->st_ino);

@@ -340,7 +340,8 @@ int cp_compat_stat(struct kstat *stat, s
 	int err;
 
 	if (stat->size > MAX_NON_LFS || !old_valid_dev(stat->dev) ||
-	    !old_valid_dev(stat->rdev))
+	    !old_valid_dev(stat->rdev) ||
+	    !check_inumber(stat->ino, &statbuf->st_ino))
 		return -EOVERFLOW;
 
 	err  = put_user(old_encode_dev(stat->dev), &statbuf->st_dev);

@@ -83,7 +83,8 @@ static inline int putstat(struct sol_sta
 {
 	if (kbuf->size > MAX_NON_LFS ||
 	    !sysv_valid_dev(kbuf->dev) ||
-	    !sysv_valid_dev(kbuf->rdev))
+	    !sysv_valid_dev(kbuf->rdev) ||
+	    !check_inumber(kbuf->ino, &ubuf->st_ino))
 		return -EOVERFLOW;
 	if (put_user (sysv_encode_dev(kbuf->dev), &ubuf->st_dev)	||
 	    __put_user (kbuf->ino, &ubuf->st_ino)		||

@@ -85,6 +85,8 @@ int cp_compat_stat(struct kstat *kbuf, s
 		return -EOVERFLOW;
 	if (kbuf->size >= 0x7fffffff)
 		return -EOVERFLOW;
+	if (!check_inumber(kbuf->ino, &ubuf->st_ino))
+		return -EOVERFLOW;
 	if (!access_ok(VERIFY_WRITE, ubuf, sizeof(struct compat_stat)) ||
 	    __put_user (old_encode_dev(kbuf->dev), &ubuf->st_dev) ||
 	    __put_user (kbuf->ino, &ubuf->st_ino) ||

@@ -199,6 +199,8 @@ static int cp_new_stat(struct kstat *sta
 #else
 	tmp.st_dev = new_encode_dev(stat->dev);
 #endif
+	if (!check_inumber(stat->ino, &tmp.st_ino))
+		return -EOVERFLOW;
 	tmp.st_ino = stat->ino;
 	tmp.st_mode = stat->mode;
 	tmp.st_nlink = stat->nlink;

@@ -57,7 +57,7 @@
 #include <linux/time.h>
 
 struct kstat {
-	unsigned long	ino;
+	u64		ino;
 	dev_t		dev;
 	umode_t		mode;
 	unsigned int	nlink;

@@ -72,6 +72,27 @@ struct kstat {
 	unsigned long	blocks;
 };
 
+extern void __check_inumber_with_bogus_size(void);
+#define check_inumber(ino, to)					\
+({								\
+	int res = 1;						\
+	switch (sizeof(*to)) {					\
+		case 2:						\
+			if (ino > 0xffff)			\
+				res = 0;			\
+			break;					\
+		case 4:						\
+			if (ino > 0xffffffff)			\
+				res = 0;			\
+			break;					\
+		case 8:						\
+			break;					\
+		default:					\
+			__check_inumber_with_bogus_size();	\
+	}							\
+	res;							\
+})
+
 #endif
 
 #endif

----- End forwarded message -----

Has this potential degradation been measured? This is a lot of extra complexity which needs to justified by the resulting performance.

It seems to me that a type with a name which better matches the intended semantics would be a better choice than u64. Even something like ino64_t would help file systems maintainers to correctly implement the appropriate support.

Thanx...

ps

What extra complexity?

Why the hell would fs maintainers needs to touch their code at all? Have you actually read that patches?

Two different sized types to describe inode numbers, different paths, etc. Having two of something, when just one would suffice, is usually more complicated.

The simplest implementation would be to have one, 64 bit ino_t, and then only places which need to know about 32 bit ino_t would know about them. These places would be restricted to system call interfaces with defined 32 bit ino_t's and file systems which only support 32 bit ino_t's.

Yes, Al, I read the patches. I didn't say anything about current file systems needing make changes to implement this support. You've already proposed the changes to do so. I am more concerned with new file systems coming down the road and file system maintenance starting the second after your changes are integrated. You aren't going to be doing that support, so we will be depending upon people who aren't as aware of the intended semantics to do the support. Maintenance of a file system implementation is already complex enough, so anything that can be done to simplify the job is a good thing.

ps

_What_ different paths? And what "two of something"?

The only requirement for fs that want to report wider st_ino is to put the right value into kstat->ino in their ->getattr().

And there's already a plenty of filesystems using iget5() et.al. for icache lookups - this isn't adding anything new.

As far as 64bit ino_t is concerned - no, thanks. We'd need to walk through all existing fs code and audit existing uses of ino_t. Which is far more of support burden.

And then there's an issue of overhead on normal icache lookups for nearly all existing filesystems; ones that do wider lookup keys are *already* using
iget5(), BTW. So you'll simply punish all users of iget().