[RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

When a filesystem manages its own inode numbers, like NFS's fileid shown
to user space with getattr(), other part of the kernel may still expose
the private inode->ino through kernel logs and audit.

Another issue is on 32-bit architectures, on which ino_t is 32 bits,
whereas the user space's view of an inode number can still be 64 bits.

Add a new inode_get_ino() helper calling the new struct
inode_operations' get_ino() when set, to get the user space's view of an
inode number.  inode_get_ino() is called by generic_fillattr().

Implement get_ino() for NFS.

Cc: Trond Myklebust <trondmy@kernel.org>
Cc: Anna Schumaker <anna@kernel.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

I'm not sure about nfs_namespace_getattr(), please review carefully.

I guess there are other filesystems exposing inode numbers different
than inode->i_ino, and they should be patched too.
---
 fs/nfs/inode.c     | 6 ++++--
 fs/nfs/internal.h  | 1 +
 fs/nfs/namespace.c | 2 ++
 fs/stat.c          | 2 +-
 include/linux/fs.h | 9 +++++++++
 5 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 542c7d97b235..5dfc176b6d92 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
 
 /**
  * nfs_compat_user_ino64 - returns the user-visible inode number
- * @fileid: 64-bit fileid
+ * @inode: inode pointer
  *
  * This function returns a 32-bit inode number if the boot parameter
  * nfs.enable_ino64 is zero.
  */
-u64 nfs_compat_user_ino64(u64 fileid)
+u64 nfs_compat_user_ino64(const struct *inode)
 {
 #ifdef CONFIG_COMPAT
 	compat_ulong_t ino;
 #else	
 	unsigned long ino;
 #endif
+	u64 fileid = NFS_FILEID(inode);
 
 	if (enable_ino64)
 		return fileid;
@@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
 		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
 	return ino;
 }
+EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
 
 int nfs_drop_inode(struct inode *inode)
 {
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 430733e3eff2..f5555a71a733 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode *inode);
 extern void nfs_set_cache_invalid(struct inode *inode, unsigned long flags);
 extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
 extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
+extern u64 nfs_compat_user_ino64(const struct *inode);
 
 #if IS_ENABLED(CONFIG_NFS_LOCALIO)
 /* localio.c */
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index e7494cdd957e..d9b1e0606833 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 const struct inode_operations nfs_mountpoint_inode_operations = {
 	.getattr	= nfs_getattr,
 	.setattr	= nfs_setattr,
+	.get_ino	= nfs_compat_user_ino64,
 };
 
 const struct inode_operations nfs_referral_inode_operations = {
 	.getattr	= nfs_namespace_getattr,
 	.setattr	= nfs_namespace_setattr,
+	.get_ino	= nfs_compat_user_ino64,
 };
 
 static void nfs_expire_automounts(struct work_struct *work)
diff --git a/fs/stat.c b/fs/stat.c
index 41e598376d7e..05636919f94b 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
 	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
 
 	stat->dev = inode->i_sb->s_dev;
-	stat->ino = inode->i_ino;
+	stat->ino = inode_get_ino(inode);
 	stat->mode = inode->i_mode;
 	stat->nlink = inode->i_nlink;
 	stat->uid = vfsuid_into_kuid(vfsuid);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e3c603d01337..0eba09a21cf7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2165,6 +2165,7 @@ struct inode_operations {
 			    struct dentry *dentry, struct fileattr *fa);
 	int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
 	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
+	u64 (*get_ino)(const struct inode *inode);
 } ____cacheline_aligned;
 
 static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
@@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
 	return file->f_op->mmap(file, vma);
 }
 
+static inline u64 inode_get_ino(struct inode *inode)
+{
+	if (unlikely(inode->i_op->get_ino))
+		return inode->i_op->get_ino(inode);
+
+	return inode->i_ino;
+}
+
 extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
 extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
-- 
2.46.1

[RFC PATCH v1 2/7] audit: Fix inode numbers

[Mickaël Salaün]

Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Paul Moore <paul@paul-moore.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/lsm_audit.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 849e832719e2..c39a22b27cce 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -227,7 +227,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		if (inode) {
 			audit_log_format(ab, " dev=");
 			audit_log_untrustedstring(ab, inode->i_sb->s_id);
-			audit_log_format(ab, " ino=%lu", inode->i_ino);
+			audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 		}
 		break;
 	}
@@ -240,7 +240,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		if (inode) {
 			audit_log_format(ab, " dev=");
 			audit_log_untrustedstring(ab, inode->i_sb->s_id);
-			audit_log_format(ab, " ino=%lu", inode->i_ino);
+			audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 		}
 		break;
 	}
@@ -253,7 +253,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		if (inode) {
 			audit_log_format(ab, " dev=");
 			audit_log_untrustedstring(ab, inode->i_sb->s_id);
-			audit_log_format(ab, " ino=%lu", inode->i_ino);
+			audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 		}
 
 		audit_log_format(ab, " ioctlcmd=0x%hx", a->u.op->cmd);
@@ -271,7 +271,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		if (inode) {
 			audit_log_format(ab, " dev=");
 			audit_log_untrustedstring(ab, inode->i_sb->s_id);
-			audit_log_format(ab, " ino=%lu", inode->i_ino);
+			audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 		}
 		break;
 	}
@@ -290,7 +290,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		}
 		audit_log_format(ab, " dev=");
 		audit_log_untrustedstring(ab, inode->i_sb->s_id);
-		audit_log_format(ab, " ino=%lu", inode->i_ino);
+		audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 		rcu_read_unlock();
 		break;
 	}
-- 
2.46.1

Re: [PATCH RFC v1 2/7] audit: Fix inode numbers

[Paul Moore]

On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
> 
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Eric Paris <eparis@redhat.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  security/lsm_audit.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com

Re: [PATCH RFC v1 2/7] audit: Fix inode numbers

[Paul Moore]

On Thu, Oct 10, 2024 at 9:20 PM Paul Moore <paul@paul-moore.com> wrote:
> On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> >
> > Use the new inode_get_ino() helper to log the user space's view of
> > inode's numbers instead of the private kernel values.
> >
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Eric Paris <eparis@redhat.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  security/lsm_audit.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
>
> Acked-by: Paul Moore <paul@paul-moore.com>

It looks like patch 1/7 still needs some revisions, and an ACK from
the NFS/VFS folks, but once that's sorted I can send the patchset up
to Linus marked for stable.

-- 
paul-moore.com

Re: [RFC PATCH v1 2/7] audit: Fix inode numbers

[Paul Moore]

On Thu, Oct 10, 2024 at 11:26 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Eric Paris <eparis@redhat.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  security/lsm_audit.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

While answering some off-list questions regarding audit, I realized
we've got similar issues with audit_name->ino and audit_watch->ino.
It would be nice if you could also fix that in this patchset.

-- 
paul-moore.com

Re: [RFC PATCH v1 2/7] audit: Fix inode numbers

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 05:34:21PM -0400, Paul Moore wrote:
> On Thu, Oct 10, 2024 at 11:26 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Use the new inode_get_ino() helper to log the user space's view of
> > inode's numbers instead of the private kernel values.
> >
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Eric Paris <eparis@redhat.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  security/lsm_audit.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> While answering some off-list questions regarding audit, I realized
> we've got similar issues with audit_name->ino and audit_watch->ino.
> It would be nice if you could also fix that in this patchset.

I can do that with the next version, but I'm wondering how it would fit
with the UAPI's struct audit_rule_data which has only 32-bit
fields/values.  Does 64-bit inode filtering currently work?

Re: [RFC PATCH v1 2/7] audit: Fix inode numbers

[Paul Moore]

On Mon, Oct 14, 2024 at 9:30 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Fri, Oct 11, 2024 at 05:34:21PM -0400, Paul Moore wrote:
> > On Thu, Oct 10, 2024 at 11:26 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > Use the new inode_get_ino() helper to log the user space's view of
> > > inode's numbers instead of the private kernel values.
> > >
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Cc: Eric Paris <eparis@redhat.com>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > >  security/lsm_audit.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > While answering some off-list questions regarding audit, I realized
> > we've got similar issues with audit_name->ino and audit_watch->ino.
> > It would be nice if you could also fix that in this patchset.
>
> I can do that with the next version, but I'm wondering how it would fit
> with the UAPI's struct audit_rule_data which has only 32-bit
> fields/values.

Don't worry about audit_rule_data for the moment, that's obviously
going to require a userspace update as well to supply 64-bit inode
numbers.  My guess is we'll probably want to introduce a new field
type, e.g. AUDIT_INODE64 or similar, that either carries the high
32-bits and is used in conjunction with AUDIT_INODE, or we create the
new AUDIT_INODE64 field as a "special" filter field which takes up two
of the u32 value spots.  Regardless, let's not worry about that for
this patchset and focus on ensuring the underlying kernel filtering
and reporting mechanisms work as expected so that when we do sort out
the UAPI issues everything *should* work.

> Does 64-bit inode filtering currently work?

Likely not :/

-- 
paul-moore.com

[RFC PATCH v1 3/7] selinux: Fix inode numbers in error messages

[Mickaël Salaün]

Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/selinux/hooks.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fc926d3cac6e..60b31b35f475 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1384,8 +1384,8 @@ static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
 	if (rc < 0) {
 		kfree(context);
 		if (rc != -ENODATA) {
-			pr_warn("SELinux: %s:  getxattr returned %d for dev=%s ino=%ld\n",
-				__func__, -rc, inode->i_sb->s_id, inode->i_ino);
+			pr_warn("SELinux: %s:  getxattr returned %d for dev=%s ino=%llu\n",
+				__func__, -rc, inode->i_sb->s_id, inode_get_ino(inode));
 			return rc;
 		}
 		*sid = def_sid;
@@ -1396,13 +1396,13 @@ static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
 					     def_sid, GFP_NOFS);
 	if (rc) {
 		char *dev = inode->i_sb->s_id;
-		unsigned long ino = inode->i_ino;
+		u64 ino = inode_get_ino(inode);
 
 		if (rc == -EINVAL) {
-			pr_notice_ratelimited("SELinux: inode=%lu on dev=%s was found to have an invalid context=%s.  This indicates you may need to relabel the inode or the filesystem in question.\n",
+			pr_notice_ratelimited("SELinux: inode=%llu on dev=%s was found to have an invalid context=%s.  This indicates you may need to relabel the inode or the filesystem in question.\n",
 					      ino, dev, context);
 		} else {
-			pr_warn("SELinux: %s:  context_to_sid(%s) returned %d for dev=%s ino=%ld\n",
+			pr_warn("SELinux: %s:  context_to_sid(%s) returned %d for dev=%s ino=%llu\n",
 				__func__, context, -rc, dev, ino);
 		}
 	}
@@ -3324,8 +3324,8 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 					   &newsid);
 	if (rc) {
 		pr_err("SELinux:  unable to map context to SID"
-		       "for (%s, %lu), rc=%d\n",
-		       inode->i_sb->s_id, inode->i_ino, -rc);
+		       "for (%s, %llu), rc=%d\n",
+		       inode->i_sb->s_id, inode_get_ino(inode), -rc);
 		return;
 	}
 
-- 
2.46.1

Re: [PATCH RFC v1 3/7] selinux: Fix inode numbers in error messages

[Paul Moore]

On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
> 
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> Cc: Ondrej Mosnacek <omosnace@redhat.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  security/selinux/hooks.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com

[RFC PATCH v1 4/7] integrity: Fix inode numbers in audit records

[Mickaël Salaün]

Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/integrity/integrity_audit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 0ec5e4c22cb2..e344d5bcf99c 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -62,7 +62,7 @@ void integrity_audit_message(int audit_msgno, struct inode *inode,
 	if (inode) {
 		audit_log_format(ab, " dev=");
 		audit_log_untrustedstring(ab, inode->i_sb->s_id);
-		audit_log_format(ab, " ino=%lu", inode->i_ino);
+		audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 	}
 	audit_log_format(ab, " res=%d errno=%d", !result, errno);
 	audit_log_end(ab);
-- 
2.46.1

Re: [PATCH RFC v1 4/7] integrity: Fix inode numbers in audit records

[Paul Moore]

On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
> 
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Eric Snowberg <eric.snowberg@oracle.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  security/integrity/integrity_audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Should we also need to update the inode value used in hmac_add_misc()?

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 7c06ffd633d2..68ae454e187f 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
         * signatures
         */
        if (type != EVM_XATTR_PORTABLE_DIGSIG) {
-               hmac_misc.ino = inode->i_ino;
+               hmac_misc.ino = inode_get_ino(inode->i_ino);
                hmac_misc.generation = inode->i_generation;
        }
        /* The hmac uid and gid must be encoded in the initial user

--
paul-moore.com

Re: [PATCH RFC v1 4/7] integrity: Fix inode numbers in audit records

[Mickaël Salaün]

On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > 
> > Use the new inode_get_ino() helper to log the user space's view of
> > inode's numbers instead of the private kernel values.
> > 
> > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >  security/integrity/integrity_audit.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Should we also need to update the inode value used in hmac_add_misc()?

I'm not sure what the impact will be wrt backward compatibility. Mimi,
Roberto?

> 
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 7c06ffd633d2..68ae454e187f 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>          * signatures
>          */
>         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> -               hmac_misc.ino = inode->i_ino;
> +               hmac_misc.ino = inode_get_ino(inode->i_ino);
>                 hmac_misc.generation = inode->i_generation;
>         }
>         /* The hmac uid and gid must be encoded in the initial user
> 
> --
> paul-moore.com

Re: [PATCH RFC v1 4/7] integrity: Fix inode numbers in audit records

[Roberto Sassu]

On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> > On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > > 
> > > Use the new inode_get_ino() helper to log the user space's view of
> > > inode's numbers instead of the private kernel values.
> > > 
> > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > >  security/integrity/integrity_audit.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Should we also need to update the inode value used in hmac_add_misc()?
> 
> I'm not sure what the impact will be wrt backward compatibility. Mimi,
> Roberto?

Changing the inode number the HMAC was calculated with has the
potential effect of making the file inaccessible.

In order to use the new inode number, we need to define a new EVM xattr
type, and update the previous xattr version with the new one. We could
deprecate the old xattr version after a while (to be discussed with
Mimi).

Roberto

> > 
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index 7c06ffd633d2..68ae454e187f 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> >          * signatures
> >          */
> >         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> > -               hmac_misc.ino = inode->i_ino;
> > +               hmac_misc.ino = inode_get_ino(inode->i_ino);
> >                 hmac_misc.generation = inode->i_generation;
> >         }
> >         /* The hmac uid and gid must be encoded in the initial user
> > 
> > --
> > paul-moore.com

Re: [PATCH RFC v1 4/7] integrity: Fix inode numbers in audit records

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 01:34:39PM +0200, Roberto Sassu wrote:
> On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> > On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> > > On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > > > 
> > > > Use the new inode_get_ino() helper to log the user space's view of
> > > > inode's numbers instead of the private kernel values.
> > > > 
> > > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > ---
> > > >  security/integrity/integrity_audit.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Should we also need to update the inode value used in hmac_add_misc()?
> > 
> > I'm not sure what the impact will be wrt backward compatibility. Mimi,
> > Roberto?
> 
> Changing the inode number the HMAC was calculated with has the
> potential effect of making the file inaccessible.
> 
> In order to use the new inode number, we need to define a new EVM xattr
> type, and update the previous xattr version with the new one. We could
> deprecate the old xattr version after a while (to be discussed with
> Mimi).

That was my though.  I don't we should patch hmac_add_misc() because it
is already in the IMA/EVM ABI and not directly reflected to user space.
The issue might be that user space cannot recreate this hmac because
this private inode number is not known to user space, but I don't know
if there is such user space implementation of IMA/EVM.

> 
> Roberto
> 
> > > 
> > > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > > index 7c06ffd633d2..68ae454e187f 100644
> > > --- a/security/integrity/evm/evm_crypto.c
> > > +++ b/security/integrity/evm/evm_crypto.c
> > > @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> > >          * signatures
> > >          */
> > >         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> > > -               hmac_misc.ino = inode->i_ino;
> > > +               hmac_misc.ino = inode_get_ino(inode->i_ino);
> > >                 hmac_misc.generation = inode->i_generation;
> > >         }
> > >         /* The hmac uid and gid must be encoded in the initial user
> > > 
> > > --
> > > paul-moore.com
> 
> 

Re: [PATCH RFC v1 4/7] integrity: Fix inode numbers in audit records

[Roberto Sassu]

On Fri, 2024-10-11 at 14:38 +0200, Mickaël Salaün wrote:
> On Fri, Oct 11, 2024 at 01:34:39PM +0200, Roberto Sassu wrote:
> > On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> > > On Thu, Oct 10, 2024 at 09:20:52PM -0400, Paul Moore wrote:
> > > > On Oct 10, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > > > > 
> > > > > Use the new inode_get_ino() helper to log the user space's view of
> > > > > inode's numbers instead of the private kernel values.
> > > > > 
> > > > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > ---
> > > > >  security/integrity/integrity_audit.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Should we also need to update the inode value used in hmac_add_misc()?
> > > 
> > > I'm not sure what the impact will be wrt backward compatibility. Mimi,
> > > Roberto?
> > 
> > Changing the inode number the HMAC was calculated with has the
> > potential effect of making the file inaccessible.
> > 
> > In order to use the new inode number, we need to define a new EVM xattr
> > type, and update the previous xattr version with the new one. We could
> > deprecate the old xattr version after a while (to be discussed with
> > Mimi).
> 
> That was my though.  I don't we should patch hmac_add_misc() because it
> is already in the IMA/EVM ABI and not directly reflected to user space.
> The issue might be that user space cannot recreate this hmac because
> this private inode number is not known to user space, but I don't know
> if there is such user space implementation of IMA/EVM.

EVM will recalculate the HMAC of the file metadata based on the new
inode number, and will conclude that metadata was corrupted (same as if
someone modified a protected xattr during an offline attack).

Roberto

> > 
> > Roberto
> > 
> > > > 
> > > > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > > > index 7c06ffd633d2..68ae454e187f 100644
> > > > --- a/security/integrity/evm/evm_crypto.c
> > > > +++ b/security/integrity/evm/evm_crypto.c
> > > > @@ -155,7 +155,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> > > >          * signatures
> > > >          */
> > > >         if (type != EVM_XATTR_PORTABLE_DIGSIG) {
> > > > -               hmac_misc.ino = inode->i_ino;
> > > > +               hmac_misc.ino = inode_get_ino(inode->i_ino);
> > > >                 hmac_misc.generation = inode->i_generation;
> > > >         }
> > > >         /* The hmac uid and gid must be encoded in the initial user
> > > > 
> > > > --
> > > > paul-moore.com
> > 
> > 

[RFC PATCH v1 5/7] ipe: Fix inode numbers in audit records

[Mickaël Salaün]

Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Fan Wu <wufan@linux.microsoft.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/ipe/audit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/ipe/audit.c b/security/ipe/audit.c
index f05f0caa4850..72d3e02c2b5f 100644
--- a/security/ipe/audit.c
+++ b/security/ipe/audit.c
@@ -150,7 +150,7 @@ void ipe_audit_match(const struct ipe_eval_ctx *const ctx,
 		if (inode) {
 			audit_log_format(ab, " dev=");
 			audit_log_untrustedstring(ab, inode->i_sb->s_id);
-			audit_log_format(ab, " ino=%lu", inode->i_ino);
+			audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
 		} else {
 			audit_log_format(ab, " dev=? ino=?");
 		}
-- 
2.46.1

Re: [RFC PATCH v1 5/7] ipe: Fix inode numbers in audit records

[Fan Wu]

Acked-by: Fan Wu <wufan@linux.microsoft.com>

On 10/10/2024 8:26 AM, Mickaël Salaün wrote:
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
> 
> Cc: Fan Wu <wufan@linux.microsoft.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>   security/ipe/audit.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index f05f0caa4850..72d3e02c2b5f 100644
> --- a/security/ipe/audit.c
> +++ b/security/ipe/audit.c
> @@ -150,7 +150,7 @@ void ipe_audit_match(const struct ipe_eval_ctx *const ctx,
>   		if (inode) {
>   			audit_log_format(ab, " dev=");
>   			audit_log_untrustedstring(ab, inode->i_sb->s_id);
> -			audit_log_format(ab, " ino=%lu", inode->i_ino);
> +			audit_log_format(ab, " ino=%llu", inode_get_ino(inode));
>   		} else {
>   			audit_log_format(ab, " dev=? ino=?");
>   		}

[RFC PATCH v1 6/7] smack: Fix inode numbers in logs

[Mickaël Salaün]

Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/smack/smack_lsm.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 370fd594da12..0be7e442e70f 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -199,8 +199,8 @@ static int smk_bu_inode(struct inode *inode, int mode, int rc)
 	char acc[SMK_NUM_ACCESS_TYPE + 1];
 
 	if (isp->smk_flags & SMK_INODE_IMPURE)
-		pr_info("Smack Unconfined Corruption: inode=(%s %ld) %s\n",
-			inode->i_sb->s_id, inode->i_ino, current->comm);
+		pr_info("Smack Unconfined Corruption: inode=(%s %llu) %s\n",
+			inode->i_sb->s_id, inode_get_ino(inode), current->comm);
 
 	if (rc <= 0)
 		return rc;
@@ -212,9 +212,9 @@ static int smk_bu_inode(struct inode *inode, int mode, int rc)
 
 	smk_bu_mode(mode, acc);
 
-	pr_info("Smack %s: (%s %s %s) inode=(%s %ld) %s\n", smk_bu_mess[rc],
+	pr_info("Smack %s: (%s %s %s) inode=(%s %llu) %s\n", smk_bu_mess[rc],
 		tsp->smk_task->smk_known, isp->smk_inode->smk_known, acc,
-		inode->i_sb->s_id, inode->i_ino, current->comm);
+		inode->i_sb->s_id, inode_get_ino(inode), current->comm);
 	return 0;
 }
 #else
@@ -231,8 +231,8 @@ static int smk_bu_file(struct file *file, int mode, int rc)
 	char acc[SMK_NUM_ACCESS_TYPE + 1];
 
 	if (isp->smk_flags & SMK_INODE_IMPURE)
-		pr_info("Smack Unconfined Corruption: inode=(%s %ld) %s\n",
-			inode->i_sb->s_id, inode->i_ino, current->comm);
+		pr_info("Smack Unconfined Corruption: inode=(%s %llu) %s\n",
+			inode->i_sb->s_id, inode_get_ino(inode), current->comm);
 
 	if (rc <= 0)
 		return rc;
@@ -240,9 +240,9 @@ static int smk_bu_file(struct file *file, int mode, int rc)
 		rc = 0;
 
 	smk_bu_mode(mode, acc);
-	pr_info("Smack %s: (%s %s %s) file=(%s %ld %pD) %s\n", smk_bu_mess[rc],
+	pr_info("Smack %s: (%s %s %s) file=(%s %llu %pD) %s\n", smk_bu_mess[rc],
 		sskp->smk_known, smk_of_inode(inode)->smk_known, acc,
-		inode->i_sb->s_id, inode->i_ino, file,
+		inode->i_sb->s_id, inode_get_ino(inode), file,
 		current->comm);
 	return 0;
 }
@@ -261,8 +261,8 @@ static int smk_bu_credfile(const struct cred *cred, struct file *file,
 	char acc[SMK_NUM_ACCESS_TYPE + 1];
 
 	if (isp->smk_flags & SMK_INODE_IMPURE)
-		pr_info("Smack Unconfined Corruption: inode=(%s %ld) %s\n",
-			inode->i_sb->s_id, inode->i_ino, current->comm);
+		pr_info("Smack Unconfined Corruption: inode=(%s %llu) %s\n",
+			inode->i_sb->s_id, inode_get_ino(inode), current->comm);
 
 	if (rc <= 0)
 		return rc;
@@ -270,9 +270,9 @@ static int smk_bu_credfile(const struct cred *cred, struct file *file,
 		rc = 0;
 
 	smk_bu_mode(mode, acc);
-	pr_info("Smack %s: (%s %s %s) file=(%s %ld %pD) %s\n", smk_bu_mess[rc],
+	pr_info("Smack %s: (%s %s %s) file=(%s %llu %pD) %s\n", smk_bu_mess[rc],
 		sskp->smk_known, smk_of_inode(inode)->smk_known, acc,
-		inode->i_sb->s_id, inode->i_ino, file,
+		inode->i_sb->s_id, inode_get_ino(inode), file,
 		current->comm);
 	return 0;
 }
-- 
2.46.1

Re: [RFC PATCH v1 6/7] smack: Fix inode numbers in logs

[Casey Schaufler]

On 10/10/2024 8:26 AM, Mickaël Salaün wrote:
> Use the new inode_get_ino() helper to log the user space's view of
> inode's numbers instead of the private kernel values.
>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/smack/smack_lsm.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 370fd594da12..0be7e442e70f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -199,8 +199,8 @@ static int smk_bu_inode(struct inode *inode, int mode, int rc)
>  	char acc[SMK_NUM_ACCESS_TYPE + 1];
>  
>  	if (isp->smk_flags & SMK_INODE_IMPURE)
> -		pr_info("Smack Unconfined Corruption: inode=(%s %ld) %s\n",
> -			inode->i_sb->s_id, inode->i_ino, current->comm);
> +		pr_info("Smack Unconfined Corruption: inode=(%s %llu) %s\n",
> +			inode->i_sb->s_id, inode_get_ino(inode), current->comm);
>  
>  	if (rc <= 0)
>  		return rc;
> @@ -212,9 +212,9 @@ static int smk_bu_inode(struct inode *inode, int mode, int rc)
>  
>  	smk_bu_mode(mode, acc);
>  
> -	pr_info("Smack %s: (%s %s %s) inode=(%s %ld) %s\n", smk_bu_mess[rc],
> +	pr_info("Smack %s: (%s %s %s) inode=(%s %llu) %s\n", smk_bu_mess[rc],
>  		tsp->smk_task->smk_known, isp->smk_inode->smk_known, acc,
> -		inode->i_sb->s_id, inode->i_ino, current->comm);
> +		inode->i_sb->s_id, inode_get_ino(inode), current->comm);
>  	return 0;
>  }
>  #else
> @@ -231,8 +231,8 @@ static int smk_bu_file(struct file *file, int mode, int rc)
>  	char acc[SMK_NUM_ACCESS_TYPE + 1];
>  
>  	if (isp->smk_flags & SMK_INODE_IMPURE)
> -		pr_info("Smack Unconfined Corruption: inode=(%s %ld) %s\n",
> -			inode->i_sb->s_id, inode->i_ino, current->comm);
> +		pr_info("Smack Unconfined Corruption: inode=(%s %llu) %s\n",
> +			inode->i_sb->s_id, inode_get_ino(inode), current->comm);
>  
>  	if (rc <= 0)
>  		return rc;
> @@ -240,9 +240,9 @@ static int smk_bu_file(struct file *file, int mode, int rc)
>  		rc = 0;
>  
>  	smk_bu_mode(mode, acc);
> -	pr_info("Smack %s: (%s %s %s) file=(%s %ld %pD) %s\n", smk_bu_mess[rc],
> +	pr_info("Smack %s: (%s %s %s) file=(%s %llu %pD) %s\n", smk_bu_mess[rc],
>  		sskp->smk_known, smk_of_inode(inode)->smk_known, acc,
> -		inode->i_sb->s_id, inode->i_ino, file,
> +		inode->i_sb->s_id, inode_get_ino(inode), file,
>  		current->comm);
>  	return 0;
>  }
> @@ -261,8 +261,8 @@ static int smk_bu_credfile(const struct cred *cred, struct file *file,
>  	char acc[SMK_NUM_ACCESS_TYPE + 1];
>  
>  	if (isp->smk_flags & SMK_INODE_IMPURE)
> -		pr_info("Smack Unconfined Corruption: inode=(%s %ld) %s\n",
> -			inode->i_sb->s_id, inode->i_ino, current->comm);
> +		pr_info("Smack Unconfined Corruption: inode=(%s %llu) %s\n",
> +			inode->i_sb->s_id, inode_get_ino(inode), current->comm);
>  
>  	if (rc <= 0)
>  		return rc;
> @@ -270,9 +270,9 @@ static int smk_bu_credfile(const struct cred *cred, struct file *file,
>  		rc = 0;
>  
>  	smk_bu_mode(mode, acc);
> -	pr_info("Smack %s: (%s %s %s) file=(%s %ld %pD) %s\n", smk_bu_mess[rc],
> +	pr_info("Smack %s: (%s %s %s) file=(%s %llu %pD) %s\n", smk_bu_mess[rc],
>  		sskp->smk_known, smk_of_inode(inode)->smk_known, acc,
> -		inode->i_sb->s_id, inode->i_ino, file,
> +		inode->i_sb->s_id, inode_get_ino(inode), file,
>  		current->comm);
>  	return 0;
>  }

[RFC PATCH v1 7/7] tomoyo: Fix inode numbers in logs

[Mickaël Salaün]

Use the new inode_get_ino() helper to log the user space's view of
inode's numbers instead of the private kernel values.

Cc: Kentaro Takeda <takedakn@nttdata.co.jp>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

Because of the required type changes, there might be some side effects.
Please review carefully.
---
 security/tomoyo/common.h    | 4 ++--
 security/tomoyo/condition.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index 0e8e2e959aef..c670a8e3c351 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -524,7 +524,7 @@ struct tomoyo_name_union {
 
 /* Structure for holding a number. */
 struct tomoyo_number_union {
-	unsigned long values[2];
+	u64 values[2];
 	struct tomoyo_group *group; /* Maybe NULL. */
 	/* One of values in "enum tomoyo_value_type". */
 	u8 value_type[2];
@@ -567,7 +567,7 @@ struct tomoyo_address_group {
 struct tomoyo_mini_stat {
 	kuid_t uid;
 	kgid_t gid;
-	ino_t ino;
+	u64 ino;
 	umode_t mode;
 	dev_t dev;
 	dev_t rdev;
diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
index f8bcc083bb0d..a44ea574fd89 100644
--- a/security/tomoyo/condition.c
+++ b/security/tomoyo/condition.c
@@ -741,7 +741,7 @@ void tomoyo_get_attributes(struct tomoyo_obj_info *obj)
 
 			stat->uid  = inode->i_uid;
 			stat->gid  = inode->i_gid;
-			stat->ino  = inode->i_ino;
+			stat->ino  = inode_get_ino(inode);
 			stat->mode = inode->i_mode;
 			stat->dev  = inode->i_sb->s_dev;
 			stat->rdev = inode->i_rdev;
@@ -766,8 +766,8 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
 		      const struct tomoyo_condition *cond)
 {
 	u32 i;
-	unsigned long min_v[2] = { 0, 0 };
-	unsigned long max_v[2] = { 0, 0 };
+	u64 min_v[2] = { 0, 0 };
+	u64 max_v[2] = { 0, 0 };
 	const struct tomoyo_condition_element *condp;
 	const struct tomoyo_number_union *numbers_p;
 	const struct tomoyo_name_union *names_p;
@@ -834,7 +834,7 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
 		/* Check numeric or bit-op expressions. */
 		for (j = 0; j < 2; j++) {
 			const u8 index = j ? right : left;
-			unsigned long value = 0;
+			u64 value = 0;
 
 			switch (index) {
 			case TOMOYO_TASK_UID:
-- 
2.46.1

[PATCH] tomoyo: use u64 for handling numeric values

[Tetsuo Handa]

TOMOYO was using "unsigned long" for handling numeric values because all
possible value range fits in "unsigned long". Since Mickaël Salaün is
about to replace "ino_t" with "u64", possible value range no longer fits
in architecture-dependent "unsigned long". Therefore, replace "unsigned
long" and "ino_t" with "u64".

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Please include this patch before your patch.

 security/tomoyo/audit.c     | 10 ++++------
 security/tomoyo/common.c    | 14 +++++++-------
 security/tomoyo/common.h    | 17 ++++++++---------
 security/tomoyo/condition.c |  8 ++++----
 security/tomoyo/file.c      |  6 +++---
 security/tomoyo/group.c     |  3 +--
 security/tomoyo/util.c      | 28 ++++++++++++++--------------
 7 files changed, 41 insertions(+), 45 deletions(-)

diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
index 610c1536cf70..36c9e63651b5 100644
--- a/security/tomoyo/audit.c
+++ b/security/tomoyo/audit.c
@@ -195,21 +195,19 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
 		if (i & 1) {
 			pos += snprintf(buffer + pos,
 					tomoyo_buffer_len - 1 - pos,
-					" path%u.parent={ uid=%u gid=%u ino=%lu perm=0%o }",
+					" path%u.parent={ uid=%u gid=%u ino=%llu perm=0%o }",
 					(i >> 1) + 1,
 					from_kuid(&init_user_ns, stat->uid),
 					from_kgid(&init_user_ns, stat->gid),
-					(unsigned long)stat->ino,
-					stat->mode & S_IALLUGO);
+					stat->ino, stat->mode & S_IALLUGO);
 			continue;
 		}
 		pos += snprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
-				" path%u={ uid=%u gid=%u ino=%lu major=%u minor=%u perm=0%o type=%s",
+				" path%u={ uid=%u gid=%u ino=%llu major=%u minor=%u perm=0%o type=%s",
 				(i >> 1) + 1,
 				from_kuid(&init_user_ns, stat->uid),
 				from_kgid(&init_user_ns, stat->gid),
-				(unsigned long)stat->ino,
-				MAJOR(dev), MINOR(dev),
+				stat->ino, MAJOR(dev), MINOR(dev),
 				mode & S_IALLUGO, tomoyo_filetype(mode));
 		if (S_ISCHR(mode) || S_ISBLK(mode)) {
 			dev = stat->rdev;
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 5c7b059a332a..528b96c917e5 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -424,8 +424,8 @@ static void tomoyo_print_number_union_nospace
 		tomoyo_set_string(head, ptr->group->group_name->name);
 	} else {
 		int i;
-		unsigned long min = ptr->values[0];
-		const unsigned long max = ptr->values[1];
+		u64 min = ptr->values[0];
+		const u64 max = ptr->values[1];
 		u8 min_type = ptr->value_type[0];
 		const u8 max_type = ptr->value_type[1];
 		char buffer[128];
@@ -435,15 +435,15 @@ static void tomoyo_print_number_union_nospace
 			switch (min_type) {
 			case TOMOYO_VALUE_TYPE_HEXADECIMAL:
 				tomoyo_addprintf(buffer, sizeof(buffer),
-						 "0x%lX", min);
+						 "0x%llX", min);
 				break;
 			case TOMOYO_VALUE_TYPE_OCTAL:
 				tomoyo_addprintf(buffer, sizeof(buffer),
-						 "0%lo", min);
+						 "0%llo", min);
 				break;
 			default:
-				tomoyo_addprintf(buffer, sizeof(buffer), "%lu",
-						 min);
+				tomoyo_addprintf(buffer, sizeof(buffer),
+						 "%llu", min);
 				break;
 			}
 			if (min == max && min_type == max_type)
@@ -1287,7 +1287,7 @@ static bool tomoyo_print_condition(struct tomoyo_io_buffer *head,
 				switch (left) {
 				case TOMOYO_ARGV_ENTRY:
 					tomoyo_io_printf(head,
-							 "exec.argv[%lu]%s=\"",
+							 "exec.argv[%llu]%s=\"",
 							 argv->index, argv->is_not ? "!" : "");
 					tomoyo_set_string(head,
 							  argv->value->name);
diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index 0e8e2e959aef..bdbb4f0ae751 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -524,7 +524,7 @@ struct tomoyo_name_union {
 
 /* Structure for holding a number. */
 struct tomoyo_number_union {
-	unsigned long values[2];
+	u64 values[2];
 	struct tomoyo_group *group; /* Maybe NULL. */
 	/* One of values in "enum tomoyo_value_type". */
 	u8 value_type[2];
@@ -567,7 +567,7 @@ struct tomoyo_address_group {
 struct tomoyo_mini_stat {
 	kuid_t uid;
 	kgid_t gid;
-	ino_t ino;
+	u64 ino;
 	umode_t mode;
 	dev_t dev;
 	dev_t rdev;
@@ -605,7 +605,7 @@ struct tomoyo_obj_info {
 
 /* Structure for argv[]. */
 struct tomoyo_argv {
-	unsigned long index;
+	u64 index;
 	const struct tomoyo_path_info *value;
 	bool is_not;
 };
@@ -926,7 +926,7 @@ struct tomoyo_task {
 
 bool tomoyo_address_matches_group(const bool is_ipv6, const __be32 *address,
 				  const struct tomoyo_group *group);
-bool tomoyo_compare_number_union(const unsigned long value,
+bool tomoyo_compare_number_union(const u64 value,
 				 const struct tomoyo_number_union *ptr);
 bool tomoyo_condition(struct tomoyo_request_info *r,
 		      const struct tomoyo_condition *cond);
@@ -938,8 +938,7 @@ bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r);
 bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
 		      struct tomoyo_page_dump *dump);
 bool tomoyo_memory_ok(void *ptr);
-bool tomoyo_number_matches_group(const unsigned long min,
-				 const unsigned long max,
+bool tomoyo_number_matches_group(const u64 min, const u64 max,
 				 const struct tomoyo_group *group);
 bool tomoyo_parse_ipaddr_union(struct tomoyo_acl_param *param,
 			       struct tomoyo_ipaddr_union *ptr);
@@ -1037,7 +1036,7 @@ struct tomoyo_policy_namespace *tomoyo_assign_namespace
 (const char *domainname);
 struct tomoyo_profile *tomoyo_profile(const struct tomoyo_policy_namespace *ns,
 				      const u8 profile);
-u8 tomoyo_parse_ulong(unsigned long *result, char **str);
+u8 tomoyo_parse_u64(u64 *result, char **str);
 void *tomoyo_commit_ok(void *data, const unsigned int size);
 void __init tomoyo_load_builtin_policy(void);
 void __init tomoyo_mm_init(void);
@@ -1055,8 +1054,8 @@ void tomoyo_normalize_line(unsigned char *buffer);
 void tomoyo_notify_gc(struct tomoyo_io_buffer *head, const bool is_register);
 void tomoyo_print_ip(char *buf, const unsigned int size,
 		     const struct tomoyo_ipaddr_union *ptr);
-void tomoyo_print_ulong(char *buffer, const int buffer_len,
-			const unsigned long value, const u8 type);
+void tomoyo_print_u64(char *buffer, const int buffer_len,
+		      const u64 value, const u8 type);
 void tomoyo_put_name_union(struct tomoyo_name_union *ptr);
 void tomoyo_put_number_union(struct tomoyo_number_union *ptr);
 void tomoyo_read_log(struct tomoyo_io_buffer *head);
diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
index f8bcc083bb0d..4a27fbf4588b 100644
--- a/security/tomoyo/condition.c
+++ b/security/tomoyo/condition.c
@@ -299,7 +299,7 @@ static bool tomoyo_parse_name_union_quoted(struct tomoyo_acl_param *param,
 static bool tomoyo_parse_argv(char *left, char *right,
 			      struct tomoyo_argv *argv)
 {
-	if (tomoyo_parse_ulong(&argv->index, &left) !=
+	if (tomoyo_parse_u64(&argv->index, &left) !=
 	    TOMOYO_VALUE_TYPE_DECIMAL || *left++ != ']' || *left)
 		return false;
 	argv->value = tomoyo_get_dqword(right);
@@ -766,8 +766,8 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
 		      const struct tomoyo_condition *cond)
 {
 	u32 i;
-	unsigned long min_v[2] = { 0, 0 };
-	unsigned long max_v[2] = { 0, 0 };
+	u64 min_v[2] = { 0, 0 };
+	u64 max_v[2] = { 0, 0 };
 	const struct tomoyo_condition_element *condp;
 	const struct tomoyo_number_union *numbers_p;
 	const struct tomoyo_name_union *names_p;
@@ -834,7 +834,7 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
 		/* Check numeric or bit-op expressions. */
 		for (j = 0; j < 2; j++) {
 			const u8 index = j ? right : left;
-			unsigned long value = 0;
+			u64 value = 0;
 
 			switch (index) {
 			case TOMOYO_TASK_UID:
diff --git a/security/tomoyo/file.c b/security/tomoyo/file.c
index 8f3b90b6e03d..4fa58abf5975 100644
--- a/security/tomoyo/file.c
+++ b/security/tomoyo/file.c
@@ -109,7 +109,7 @@ void tomoyo_put_number_union(struct tomoyo_number_union *ptr)
  *
  * Returns true if @value matches @ptr, false otherwise.
  */
-bool tomoyo_compare_number_union(const unsigned long value,
+bool tomoyo_compare_number_union(const u64 value,
 				 const struct tomoyo_number_union *ptr)
 {
 	if (ptr->group)
@@ -230,8 +230,8 @@ static int tomoyo_audit_path_number_log(struct tomoyo_request_info *r)
 		radix = TOMOYO_VALUE_TYPE_DECIMAL;
 		break;
 	}
-	tomoyo_print_ulong(buffer, sizeof(buffer), r->param.path_number.number,
-			   radix);
+	tomoyo_print_u64(buffer, sizeof(buffer), r->param.path_number.number,
+			 radix);
 	return tomoyo_supervisor(r, "file %s %s %s\n", tomoyo_mac_keywords
 				 [tomoyo_pn2mac[type]],
 				 r->param.path_number.filename->name, buffer);
diff --git a/security/tomoyo/group.c b/security/tomoyo/group.c
index 1cecdd797597..dc650eaedba3 100644
--- a/security/tomoyo/group.c
+++ b/security/tomoyo/group.c
@@ -155,8 +155,7 @@ tomoyo_path_matches_group(const struct tomoyo_path_info *pathname,
  *
  * Caller holds tomoyo_read_lock().
  */
-bool tomoyo_number_matches_group(const unsigned long min,
-				 const unsigned long max,
+bool tomoyo_number_matches_group(const u64 min, const u64 max,
 				 const struct tomoyo_group *group)
 {
 	struct tomoyo_number_group *member;
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 6799b1122c9d..ac9535b4bdcd 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -172,9 +172,9 @@ const struct tomoyo_path_info *tomoyo_get_domainname
 }
 
 /**
- * tomoyo_parse_ulong - Parse an "unsigned long" value.
+ * tomoyo_parse_u64 - Parse a u64 value.
  *
- * @result: Pointer to "unsigned long".
+ * @result: Pointer to u64.
  * @str:    Pointer to string to parse.
  *
  * Returns one of values in "enum tomoyo_value_type".
@@ -182,7 +182,7 @@ const struct tomoyo_path_info *tomoyo_get_domainname
  * The @src is updated to point the first character after the value
  * on success.
  */
-u8 tomoyo_parse_ulong(unsigned long *result, char **str)
+u8 tomoyo_parse_u64(u64 *result, char **str)
 {
 	const char *cp = *str;
 	char *ep;
@@ -199,7 +199,7 @@ u8 tomoyo_parse_ulong(unsigned long *result, char **str)
 			cp++;
 		}
 	}
-	*result = simple_strtoul(cp, &ep, base);
+	*result = (u64) simple_strtoull(cp, &ep, base);
 	if (cp == ep)
 		return TOMOYO_VALUE_TYPE_INVALID;
 	*str = ep;
@@ -214,24 +214,24 @@ u8 tomoyo_parse_ulong(unsigned long *result, char **str)
 }
 
 /**
- * tomoyo_print_ulong - Print an "unsigned long" value.
+ * tomoyo_print_u64 - Print a u64 value.
  *
  * @buffer:     Pointer to buffer.
  * @buffer_len: Size of @buffer.
- * @value:      An "unsigned long" value.
+ * @value:      A u64 value.
  * @type:       Type of @value.
  *
  * Returns nothing.
  */
-void tomoyo_print_ulong(char *buffer, const int buffer_len,
-			const unsigned long value, const u8 type)
+void tomoyo_print_u64(char *buffer, const int buffer_len,
+		      const u64 value, const u8 type)
 {
 	if (type == TOMOYO_VALUE_TYPE_DECIMAL)
-		snprintf(buffer, buffer_len, "%lu", value);
+		snprintf(buffer, buffer_len, "%llu", value);
 	else if (type == TOMOYO_VALUE_TYPE_OCTAL)
-		snprintf(buffer, buffer_len, "0%lo", value);
+		snprintf(buffer, buffer_len, "0%llo", value);
 	else if (type == TOMOYO_VALUE_TYPE_HEXADECIMAL)
-		snprintf(buffer, buffer_len, "0x%lX", value);
+		snprintf(buffer, buffer_len, "0x%llX", value);
 	else
 		snprintf(buffer, buffer_len, "type(%u)", type);
 }
@@ -274,7 +274,7 @@ bool tomoyo_parse_number_union(struct tomoyo_acl_param *param,
 {
 	char *data;
 	u8 type;
-	unsigned long v;
+	u64 v;
 
 	memset(ptr, 0, sizeof(*ptr));
 	if (param->data[0] == '@') {
@@ -283,7 +283,7 @@ bool tomoyo_parse_number_union(struct tomoyo_acl_param *param,
 		return ptr->group != NULL;
 	}
 	data = tomoyo_read_token(param);
-	type = tomoyo_parse_ulong(&v, &data);
+	type = tomoyo_parse_u64(&v, &data);
 	if (type == TOMOYO_VALUE_TYPE_INVALID)
 		return false;
 	ptr->values[0] = v;
@@ -295,7 +295,7 @@ bool tomoyo_parse_number_union(struct tomoyo_acl_param *param,
 	}
 	if (*data++ != '-')
 		return false;
-	type = tomoyo_parse_ulong(&v, &data);
+	type = tomoyo_parse_u64(&v, &data);
 	if (type == TOMOYO_VALUE_TYPE_INVALID || *data || ptr->values[0] > v)
 		return false;
 	ptr->values[1] = v;
-- 
2.43.5

Re: [PATCH] tomoyo: use u64 for handling numeric values

[Mickaël Salaün]

On Sat, Oct 12, 2024 at 04:35:54PM +0900, Tetsuo Handa wrote:
> TOMOYO was using "unsigned long" for handling numeric values because all
> possible value range fits in "unsigned long". Since Mickaël Salaün is
> about to replace "ino_t" with "u64", possible value range no longer fits
> in architecture-dependent "unsigned long". Therefore, replace "unsigned
> long" and "ino_t" with "u64".
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Please include this patch before your patch.

Thanks, I'll merge the two patches to get a more consistent one in the
next series.

> 
>  security/tomoyo/audit.c     | 10 ++++------
>  security/tomoyo/common.c    | 14 +++++++-------
>  security/tomoyo/common.h    | 17 ++++++++---------
>  security/tomoyo/condition.c |  8 ++++----
>  security/tomoyo/file.c      |  6 +++---
>  security/tomoyo/group.c     |  3 +--
>  security/tomoyo/util.c      | 28 ++++++++++++++--------------
>  7 files changed, 41 insertions(+), 45 deletions(-)
> 
> diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
> index 610c1536cf70..36c9e63651b5 100644
> --- a/security/tomoyo/audit.c
> +++ b/security/tomoyo/audit.c
> @@ -195,21 +195,19 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
>  		if (i & 1) {
>  			pos += snprintf(buffer + pos,
>  					tomoyo_buffer_len - 1 - pos,
> -					" path%u.parent={ uid=%u gid=%u ino=%lu perm=0%o }",
> +					" path%u.parent={ uid=%u gid=%u ino=%llu perm=0%o }",
>  					(i >> 1) + 1,
>  					from_kuid(&init_user_ns, stat->uid),
>  					from_kgid(&init_user_ns, stat->gid),
> -					(unsigned long)stat->ino,
> -					stat->mode & S_IALLUGO);
> +					stat->ino, stat->mode & S_IALLUGO);
>  			continue;
>  		}
>  		pos += snprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
> -				" path%u={ uid=%u gid=%u ino=%lu major=%u minor=%u perm=0%o type=%s",
> +				" path%u={ uid=%u gid=%u ino=%llu major=%u minor=%u perm=0%o type=%s",
>  				(i >> 1) + 1,
>  				from_kuid(&init_user_ns, stat->uid),
>  				from_kgid(&init_user_ns, stat->gid),
> -				(unsigned long)stat->ino,
> -				MAJOR(dev), MINOR(dev),
> +				stat->ino, MAJOR(dev), MINOR(dev),
>  				mode & S_IALLUGO, tomoyo_filetype(mode));
>  		if (S_ISCHR(mode) || S_ISBLK(mode)) {
>  			dev = stat->rdev;
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index 5c7b059a332a..528b96c917e5 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -424,8 +424,8 @@ static void tomoyo_print_number_union_nospace
>  		tomoyo_set_string(head, ptr->group->group_name->name);
>  	} else {
>  		int i;
> -		unsigned long min = ptr->values[0];
> -		const unsigned long max = ptr->values[1];
> +		u64 min = ptr->values[0];
> +		const u64 max = ptr->values[1];
>  		u8 min_type = ptr->value_type[0];
>  		const u8 max_type = ptr->value_type[1];
>  		char buffer[128];
> @@ -435,15 +435,15 @@ static void tomoyo_print_number_union_nospace
>  			switch (min_type) {
>  			case TOMOYO_VALUE_TYPE_HEXADECIMAL:
>  				tomoyo_addprintf(buffer, sizeof(buffer),
> -						 "0x%lX", min);
> +						 "0x%llX", min);
>  				break;
>  			case TOMOYO_VALUE_TYPE_OCTAL:
>  				tomoyo_addprintf(buffer, sizeof(buffer),
> -						 "0%lo", min);
> +						 "0%llo", min);
>  				break;
>  			default:
> -				tomoyo_addprintf(buffer, sizeof(buffer), "%lu",
> -						 min);
> +				tomoyo_addprintf(buffer, sizeof(buffer),
> +						 "%llu", min);
>  				break;
>  			}
>  			if (min == max && min_type == max_type)
> @@ -1287,7 +1287,7 @@ static bool tomoyo_print_condition(struct tomoyo_io_buffer *head,
>  				switch (left) {
>  				case TOMOYO_ARGV_ENTRY:
>  					tomoyo_io_printf(head,
> -							 "exec.argv[%lu]%s=\"",
> +							 "exec.argv[%llu]%s=\"",
>  							 argv->index, argv->is_not ? "!" : "");
>  					tomoyo_set_string(head,
>  							  argv->value->name);
> diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
> index 0e8e2e959aef..bdbb4f0ae751 100644
> --- a/security/tomoyo/common.h
> +++ b/security/tomoyo/common.h
> @@ -524,7 +524,7 @@ struct tomoyo_name_union {
>  
>  /* Structure for holding a number. */
>  struct tomoyo_number_union {
> -	unsigned long values[2];
> +	u64 values[2];
>  	struct tomoyo_group *group; /* Maybe NULL. */
>  	/* One of values in "enum tomoyo_value_type". */
>  	u8 value_type[2];
> @@ -567,7 +567,7 @@ struct tomoyo_address_group {
>  struct tomoyo_mini_stat {
>  	kuid_t uid;
>  	kgid_t gid;
> -	ino_t ino;
> +	u64 ino;
>  	umode_t mode;
>  	dev_t dev;
>  	dev_t rdev;
> @@ -605,7 +605,7 @@ struct tomoyo_obj_info {
>  
>  /* Structure for argv[]. */
>  struct tomoyo_argv {
> -	unsigned long index;
> +	u64 index;
>  	const struct tomoyo_path_info *value;
>  	bool is_not;
>  };
> @@ -926,7 +926,7 @@ struct tomoyo_task {
>  
>  bool tomoyo_address_matches_group(const bool is_ipv6, const __be32 *address,
>  				  const struct tomoyo_group *group);
> -bool tomoyo_compare_number_union(const unsigned long value,
> +bool tomoyo_compare_number_union(const u64 value,
>  				 const struct tomoyo_number_union *ptr);
>  bool tomoyo_condition(struct tomoyo_request_info *r,
>  		      const struct tomoyo_condition *cond);
> @@ -938,8 +938,7 @@ bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r);
>  bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
>  		      struct tomoyo_page_dump *dump);
>  bool tomoyo_memory_ok(void *ptr);
> -bool tomoyo_number_matches_group(const unsigned long min,
> -				 const unsigned long max,
> +bool tomoyo_number_matches_group(const u64 min, const u64 max,
>  				 const struct tomoyo_group *group);
>  bool tomoyo_parse_ipaddr_union(struct tomoyo_acl_param *param,
>  			       struct tomoyo_ipaddr_union *ptr);
> @@ -1037,7 +1036,7 @@ struct tomoyo_policy_namespace *tomoyo_assign_namespace
>  (const char *domainname);
>  struct tomoyo_profile *tomoyo_profile(const struct tomoyo_policy_namespace *ns,
>  				      const u8 profile);
> -u8 tomoyo_parse_ulong(unsigned long *result, char **str);
> +u8 tomoyo_parse_u64(u64 *result, char **str);
>  void *tomoyo_commit_ok(void *data, const unsigned int size);
>  void __init tomoyo_load_builtin_policy(void);
>  void __init tomoyo_mm_init(void);
> @@ -1055,8 +1054,8 @@ void tomoyo_normalize_line(unsigned char *buffer);
>  void tomoyo_notify_gc(struct tomoyo_io_buffer *head, const bool is_register);
>  void tomoyo_print_ip(char *buf, const unsigned int size,
>  		     const struct tomoyo_ipaddr_union *ptr);
> -void tomoyo_print_ulong(char *buffer, const int buffer_len,
> -			const unsigned long value, const u8 type);
> +void tomoyo_print_u64(char *buffer, const int buffer_len,
> +		      const u64 value, const u8 type);
>  void tomoyo_put_name_union(struct tomoyo_name_union *ptr);
>  void tomoyo_put_number_union(struct tomoyo_number_union *ptr);
>  void tomoyo_read_log(struct tomoyo_io_buffer *head);
> diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
> index f8bcc083bb0d..4a27fbf4588b 100644
> --- a/security/tomoyo/condition.c
> +++ b/security/tomoyo/condition.c
> @@ -299,7 +299,7 @@ static bool tomoyo_parse_name_union_quoted(struct tomoyo_acl_param *param,
>  static bool tomoyo_parse_argv(char *left, char *right,
>  			      struct tomoyo_argv *argv)
>  {
> -	if (tomoyo_parse_ulong(&argv->index, &left) !=
> +	if (tomoyo_parse_u64(&argv->index, &left) !=
>  	    TOMOYO_VALUE_TYPE_DECIMAL || *left++ != ']' || *left)
>  		return false;
>  	argv->value = tomoyo_get_dqword(right);
> @@ -766,8 +766,8 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
>  		      const struct tomoyo_condition *cond)
>  {
>  	u32 i;
> -	unsigned long min_v[2] = { 0, 0 };
> -	unsigned long max_v[2] = { 0, 0 };
> +	u64 min_v[2] = { 0, 0 };
> +	u64 max_v[2] = { 0, 0 };
>  	const struct tomoyo_condition_element *condp;
>  	const struct tomoyo_number_union *numbers_p;
>  	const struct tomoyo_name_union *names_p;
> @@ -834,7 +834,7 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
>  		/* Check numeric or bit-op expressions. */
>  		for (j = 0; j < 2; j++) {
>  			const u8 index = j ? right : left;
> -			unsigned long value = 0;
> +			u64 value = 0;
>  
>  			switch (index) {
>  			case TOMOYO_TASK_UID:
> diff --git a/security/tomoyo/file.c b/security/tomoyo/file.c
> index 8f3b90b6e03d..4fa58abf5975 100644
> --- a/security/tomoyo/file.c
> +++ b/security/tomoyo/file.c
> @@ -109,7 +109,7 @@ void tomoyo_put_number_union(struct tomoyo_number_union *ptr)
>   *
>   * Returns true if @value matches @ptr, false otherwise.
>   */
> -bool tomoyo_compare_number_union(const unsigned long value,
> +bool tomoyo_compare_number_union(const u64 value,
>  				 const struct tomoyo_number_union *ptr)
>  {
>  	if (ptr->group)
> @@ -230,8 +230,8 @@ static int tomoyo_audit_path_number_log(struct tomoyo_request_info *r)
>  		radix = TOMOYO_VALUE_TYPE_DECIMAL;
>  		break;
>  	}
> -	tomoyo_print_ulong(buffer, sizeof(buffer), r->param.path_number.number,
> -			   radix);
> +	tomoyo_print_u64(buffer, sizeof(buffer), r->param.path_number.number,
> +			 radix);
>  	return tomoyo_supervisor(r, "file %s %s %s\n", tomoyo_mac_keywords
>  				 [tomoyo_pn2mac[type]],
>  				 r->param.path_number.filename->name, buffer);
> diff --git a/security/tomoyo/group.c b/security/tomoyo/group.c
> index 1cecdd797597..dc650eaedba3 100644
> --- a/security/tomoyo/group.c
> +++ b/security/tomoyo/group.c
> @@ -155,8 +155,7 @@ tomoyo_path_matches_group(const struct tomoyo_path_info *pathname,
>   *
>   * Caller holds tomoyo_read_lock().
>   */
> -bool tomoyo_number_matches_group(const unsigned long min,
> -				 const unsigned long max,
> +bool tomoyo_number_matches_group(const u64 min, const u64 max,
>  				 const struct tomoyo_group *group)
>  {
>  	struct tomoyo_number_group *member;
> diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> index 6799b1122c9d..ac9535b4bdcd 100644
> --- a/security/tomoyo/util.c
> +++ b/security/tomoyo/util.c
> @@ -172,9 +172,9 @@ const struct tomoyo_path_info *tomoyo_get_domainname
>  }
>  
>  /**
> - * tomoyo_parse_ulong - Parse an "unsigned long" value.
> + * tomoyo_parse_u64 - Parse a u64 value.
>   *
> - * @result: Pointer to "unsigned long".
> + * @result: Pointer to u64.
>   * @str:    Pointer to string to parse.
>   *
>   * Returns one of values in "enum tomoyo_value_type".
> @@ -182,7 +182,7 @@ const struct tomoyo_path_info *tomoyo_get_domainname
>   * The @src is updated to point the first character after the value
>   * on success.
>   */
> -u8 tomoyo_parse_ulong(unsigned long *result, char **str)
> +u8 tomoyo_parse_u64(u64 *result, char **str)
>  {
>  	const char *cp = *str;
>  	char *ep;
> @@ -199,7 +199,7 @@ u8 tomoyo_parse_ulong(unsigned long *result, char **str)
>  			cp++;
>  		}
>  	}
> -	*result = simple_strtoul(cp, &ep, base);
> +	*result = (u64) simple_strtoull(cp, &ep, base);
>  	if (cp == ep)
>  		return TOMOYO_VALUE_TYPE_INVALID;
>  	*str = ep;
> @@ -214,24 +214,24 @@ u8 tomoyo_parse_ulong(unsigned long *result, char **str)
>  }
>  
>  /**
> - * tomoyo_print_ulong - Print an "unsigned long" value.
> + * tomoyo_print_u64 - Print a u64 value.
>   *
>   * @buffer:     Pointer to buffer.
>   * @buffer_len: Size of @buffer.
> - * @value:      An "unsigned long" value.
> + * @value:      A u64 value.
>   * @type:       Type of @value.
>   *
>   * Returns nothing.
>   */
> -void tomoyo_print_ulong(char *buffer, const int buffer_len,
> -			const unsigned long value, const u8 type)
> +void tomoyo_print_u64(char *buffer, const int buffer_len,
> +		      const u64 value, const u8 type)
>  {
>  	if (type == TOMOYO_VALUE_TYPE_DECIMAL)
> -		snprintf(buffer, buffer_len, "%lu", value);
> +		snprintf(buffer, buffer_len, "%llu", value);
>  	else if (type == TOMOYO_VALUE_TYPE_OCTAL)
> -		snprintf(buffer, buffer_len, "0%lo", value);
> +		snprintf(buffer, buffer_len, "0%llo", value);
>  	else if (type == TOMOYO_VALUE_TYPE_HEXADECIMAL)
> -		snprintf(buffer, buffer_len, "0x%lX", value);
> +		snprintf(buffer, buffer_len, "0x%llX", value);
>  	else
>  		snprintf(buffer, buffer_len, "type(%u)", type);
>  }
> @@ -274,7 +274,7 @@ bool tomoyo_parse_number_union(struct tomoyo_acl_param *param,
>  {
>  	char *data;
>  	u8 type;
> -	unsigned long v;
> +	u64 v;
>  
>  	memset(ptr, 0, sizeof(*ptr));
>  	if (param->data[0] == '@') {
> @@ -283,7 +283,7 @@ bool tomoyo_parse_number_union(struct tomoyo_acl_param *param,
>  		return ptr->group != NULL;
>  	}
>  	data = tomoyo_read_token(param);
> -	type = tomoyo_parse_ulong(&v, &data);
> +	type = tomoyo_parse_u64(&v, &data);
>  	if (type == TOMOYO_VALUE_TYPE_INVALID)
>  		return false;
>  	ptr->values[0] = v;
> @@ -295,7 +295,7 @@ bool tomoyo_parse_number_union(struct tomoyo_acl_param *param,
>  	}
>  	if (*data++ != '-')
>  		return false;
> -	type = tomoyo_parse_ulong(&v, &data);
> +	type = tomoyo_parse_u64(&v, &data);
>  	if (type == TOMOYO_VALUE_TYPE_INVALID || *data || ptr->values[0] > v)
>  		return false;
>  	ptr->values[1] = v;
> -- 
> 2.43.5
> 
> 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Anna Schumaker]

Hi Mickaël,

On 10/10/24 11:26 AM, Mickaël Salaün wrote:
> When a filesystem manages its own inode numbers, like NFS's fileid shown
> to user space with getattr(), other part of the kernel may still expose
> the private inode->ino through kernel logs and audit.
> 
> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> whereas the user space's view of an inode number can still be 64 bits.
> 
> Add a new inode_get_ino() helper calling the new struct
> inode_operations' get_ino() when set, to get the user space's view of an
> inode number.  inode_get_ino() is called by generic_fillattr().
> 
> Implement get_ino() for NFS.
> 
> Cc: Trond Myklebust <trondmy@kernel.org>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> I'm not sure about nfs_namespace_getattr(), please review carefully.
> 
> I guess there are other filesystems exposing inode numbers different
> than inode->i_ino, and they should be patched too.
> ---
>  fs/nfs/inode.c     | 6 ++++--
>  fs/nfs/internal.h  | 1 +
>  fs/nfs/namespace.c | 2 ++
>  fs/stat.c          | 2 +-
>  include/linux/fs.h | 9 +++++++++
>  5 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 542c7d97b235..5dfc176b6d92 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
>  
>  /**
>   * nfs_compat_user_ino64 - returns the user-visible inode number
> - * @fileid: 64-bit fileid
> + * @inode: inode pointer
>   *
>   * This function returns a 32-bit inode number if the boot parameter
>   * nfs.enable_ino64 is zero.
>   */
> -u64 nfs_compat_user_ino64(u64 fileid)
> +u64 nfs_compat_user_ino64(const struct *inode)
                             ^^^^^^^^^^^^^^^^^^^
This should be "const struct inode *inode"

>  {
>  #ifdef CONFIG_COMPAT
>  	compat_ulong_t ino;
>  #else	
>  	unsigned long ino;
>  #endif
> +	u64 fileid = NFS_FILEID(inode);
>  
>  	if (enable_ino64)
>  		return fileid;
> @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
>  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
>  	return ino;
>  }
> +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
>  
>  int nfs_drop_inode(struct inode *inode)
>  {
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 430733e3eff2..f5555a71a733 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode *inode);
>  extern void nfs_set_cache_invalid(struct inode *inode, unsigned long flags);
>  extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
>  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
> +extern u64 nfs_compat_user_ino64(const struct *inode);

Why add this here when it's already in include/linux/nfs_fs.h? Can you update that declaration instead?

Also, there is a caller for nfs_compat_user_ino64() in fs/nfs/dir.c that needs to be updated. Can you double check that you have CONFIG_NFS_FS=m (or 'y') in your kernel .config? These are all issues my compiler caught when I applied your patch.

Thanks,
Anna

>  
>  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
>  /* localio.c */
> diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> index e7494cdd957e..d9b1e0606833 100644
> --- a/fs/nfs/namespace.c
> +++ b/fs/nfs/namespace.c
> @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  const struct inode_operations nfs_mountpoint_inode_operations = {
>  	.getattr	= nfs_getattr,
>  	.setattr	= nfs_setattr,
> +	.get_ino	= nfs_compat_user_ino64,
>  };
>  
>  const struct inode_operations nfs_referral_inode_operations = {
>  	.getattr	= nfs_namespace_getattr,
>  	.setattr	= nfs_namespace_setattr,
> +	.get_ino	= nfs_compat_user_ino64,
>  };
>  
>  static void nfs_expire_automounts(struct work_struct *work)
> diff --git a/fs/stat.c b/fs/stat.c
> index 41e598376d7e..05636919f94b 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
>  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
>  
>  	stat->dev = inode->i_sb->s_dev;
> -	stat->ino = inode->i_ino;
> +	stat->ino = inode_get_ino(inode);
>  	stat->mode = inode->i_mode;
>  	stat->nlink = inode->i_nlink;
>  	stat->uid = vfsuid_into_kuid(vfsuid);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e3c603d01337..0eba09a21cf7 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2165,6 +2165,7 @@ struct inode_operations {
>  			    struct dentry *dentry, struct fileattr *fa);
>  	int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
>  	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> +	u64 (*get_ino)(const struct inode *inode);
>  } ____cacheline_aligned;
>  
>  static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
>  	return file->f_op->mmap(file, vma);
>  }
>  
> +static inline u64 inode_get_ino(struct inode *inode)
> +{
> +	if (unlikely(inode->i_op->get_ino))
> +		return inode->i_op->get_ino(inode);
> +
> +	return inode->i_ino;
> +}
> +
>  extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
>  extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
>  extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Thu, Oct 10, 2024 at 02:07:52PM -0400, Anna Schumaker wrote:
> Hi Mickaël,
> 
> On 10/10/24 11:26 AM, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid shown
> > to user space with getattr(), other part of the kernel may still expose
> > the private inode->ino through kernel logs and audit.
> > 
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64 bits.
> > 
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of an
> > inode number.  inode_get_ino() is called by generic_fillattr().
> > 
> > Implement get_ino() for NFS.
> > 
> > Cc: Trond Myklebust <trondmy@kernel.org>
> > Cc: Anna Schumaker <anna@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> > 
> > I'm not sure about nfs_namespace_getattr(), please review carefully.
> > 
> > I guess there are other filesystems exposing inode numbers different
> > than inode->i_ino, and they should be patched too.
> > ---
> >  fs/nfs/inode.c     | 6 ++++--
> >  fs/nfs/internal.h  | 1 +
> >  fs/nfs/namespace.c | 2 ++
> >  fs/stat.c          | 2 +-
> >  include/linux/fs.h | 9 +++++++++
> >  5 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 542c7d97b235..5dfc176b6d92 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
> >  
> >  /**
> >   * nfs_compat_user_ino64 - returns the user-visible inode number
> > - * @fileid: 64-bit fileid
> > + * @inode: inode pointer
> >   *
> >   * This function returns a 32-bit inode number if the boot parameter
> >   * nfs.enable_ino64 is zero.
> >   */
> > -u64 nfs_compat_user_ino64(u64 fileid)
> > +u64 nfs_compat_user_ino64(const struct *inode)
>                              ^^^^^^^^^^^^^^^^^^^
> This should be "const struct inode *inode"

Indeed...

> 
> >  {
> >  #ifdef CONFIG_COMPAT
> >  	compat_ulong_t ino;
> >  #else	
> >  	unsigned long ino;
> >  #endif
> > +	u64 fileid = NFS_FILEID(inode);
> >  
> >  	if (enable_ino64)
> >  		return fileid;
> > @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
> >  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
> >  	return ino;
> >  }
> > +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
> >  
> >  int nfs_drop_inode(struct inode *inode)
> >  {
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index 430733e3eff2..f5555a71a733 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode *inode);
> >  extern void nfs_set_cache_invalid(struct inode *inode, unsigned long flags);
> >  extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
> >  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
> > +extern u64 nfs_compat_user_ino64(const struct *inode);
> 
> Why add this here when it's already in include/linux/nfs_fs.h? Can you update that declaration instead?
> 
> Also, there is a caller for nfs_compat_user_ino64() in fs/nfs/dir.c that needs to be updated. Can you double check that you have CONFIG_NFS_FS=m (or 'y') in your kernel .config? These are all issues my compiler caught when I applied your patch.

Oh, I enabled CONFIG_NFSD_V4 and CONFIG_NFS_COMMON but not
CONFIG_NFS_FS, that explains these uncaught errors... Thanks!

About the nfs_do_filldir(), the nfs_compat_user_ino64() call is indeed
not correct with this patch, but I'm wondering why not use ent->ino
instead?

> 
> Thanks,
> Anna
> 
> >  
> >  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> >  /* localio.c */
> > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > index e7494cdd957e..d9b1e0606833 100644
> > --- a/fs/nfs/namespace.c
> > +++ b/fs/nfs/namespace.c
> > @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> >  const struct inode_operations nfs_mountpoint_inode_operations = {
> >  	.getattr	= nfs_getattr,
> >  	.setattr	= nfs_setattr,
> > +	.get_ino	= nfs_compat_user_ino64,
> >  };
> >  
> >  const struct inode_operations nfs_referral_inode_operations = {
> >  	.getattr	= nfs_namespace_getattr,
> >  	.setattr	= nfs_namespace_setattr,
> > +	.get_ino	= nfs_compat_user_ino64,

Is it correct to use nfs_compat_user_ino64() for both of these inode
operation structs?  With this patch, generic_fileattr() initialize the
stat struct with get_ino(), which might not what we want according to
these nfs_mountpoint and nfs_referral structs.


> >  };
> >  
> >  static void nfs_expire_automounts(struct work_struct *work)
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 41e598376d7e..05636919f94b 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
> >  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> >  
> >  	stat->dev = inode->i_sb->s_dev;
> > -	stat->ino = inode->i_ino;
> > +	stat->ino = inode_get_ino(inode);
> >  	stat->mode = inode->i_mode;
> >  	stat->nlink = inode->i_nlink;
> >  	stat->uid = vfsuid_into_kuid(vfsuid);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index e3c603d01337..0eba09a21cf7 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2165,6 +2165,7 @@ struct inode_operations {
> >  			    struct dentry *dentry, struct fileattr *fa);
> >  	int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
> >  	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> > +	u64 (*get_ino)(const struct inode *inode);
> >  } ____cacheline_aligned;
> >  
> >  static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> > @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> >  	return file->f_op->mmap(file, vma);
> >  }
> >  
> > +static inline u64 inode_get_ino(struct inode *inode)
> > +{
> > +	if (unlikely(inode->i_op->get_ino))
> > +		return inode->i_op->get_ino(inode);
> > +
> > +	return inode->i_ino;
> > +}
> > +
> >  extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
> >  extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
> >  extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
> 
> 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Trond Myklebust]

On Thu, 2024-10-10 at 17:26 +0200, Mickaël Salaün wrote:
> When a filesystem manages its own inode numbers, like NFS's fileid
> shown
> to user space with getattr(), other part of the kernel may still
> expose
> the private inode->ino through kernel logs and audit.
> 
> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> whereas the user space's view of an inode number can still be 64
> bits.
> 
> Add a new inode_get_ino() helper calling the new struct
> inode_operations' get_ino() when set, to get the user space's view of
> an
> inode number.  inode_get_ino() is called by generic_fillattr().
> 
> Implement get_ino() for NFS.
> 
> Cc: Trond Myklebust <trondmy@kernel.org>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> I'm not sure about nfs_namespace_getattr(), please review carefully.
> 
> I guess there are other filesystems exposing inode numbers different
> than inode->i_ino, and they should be patched too.
> ---
>  fs/nfs/inode.c     | 6 ++++--
>  fs/nfs/internal.h  | 1 +
>  fs/nfs/namespace.c | 2 ++
>  fs/stat.c          | 2 +-
>  include/linux/fs.h | 9 +++++++++
>  5 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 542c7d97b235..5dfc176b6d92 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
>  
>  /**
>   * nfs_compat_user_ino64 - returns the user-visible inode number
> - * @fileid: 64-bit fileid
> + * @inode: inode pointer
>   *
>   * This function returns a 32-bit inode number if the boot parameter
>   * nfs.enable_ino64 is zero.
>   */
> -u64 nfs_compat_user_ino64(u64 fileid)
> +u64 nfs_compat_user_ino64(const struct *inode)
>  {
>  #ifdef CONFIG_COMPAT
>  	compat_ulong_t ino;
>  #else	
>  	unsigned long ino;
>  #endif
> +	u64 fileid = NFS_FILEID(inode);
>  
>  	if (enable_ino64)
>  		return fileid;
> @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
>  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
>  	return ino;
>  }
> +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
>  
>  int nfs_drop_inode(struct inode *inode)
>  {
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 430733e3eff2..f5555a71a733 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode
> *inode);
>  extern void nfs_set_cache_invalid(struct inode *inode, unsigned long
> flags);
>  extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
>  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int
> mode);
> +extern u64 nfs_compat_user_ino64(const struct *inode);
>  
>  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
>  /* localio.c */
> diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> index e7494cdd957e..d9b1e0606833 100644
> --- a/fs/nfs/namespace.c
> +++ b/fs/nfs/namespace.c
> @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap,
> struct dentry *dentry,
>  const struct inode_operations nfs_mountpoint_inode_operations = {
>  	.getattr	= nfs_getattr,
>  	.setattr	= nfs_setattr,
> +	.get_ino	= nfs_compat_user_ino64,
>  };
>  
>  const struct inode_operations nfs_referral_inode_operations = {
>  	.getattr	= nfs_namespace_getattr,
>  	.setattr	= nfs_namespace_setattr,
> +	.get_ino	= nfs_compat_user_ino64,
>  };
>  
>  static void nfs_expire_automounts(struct work_struct *work)
> diff --git a/fs/stat.c b/fs/stat.c
> index 41e598376d7e..05636919f94b 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32
> request_mask,
>  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
>  
>  	stat->dev = inode->i_sb->s_dev;
> -	stat->ino = inode->i_ino;
> +	stat->ino = inode_get_ino(inode);
>  	stat->mode = inode->i_mode;
>  	stat->nlink = inode->i_nlink;
>  	stat->uid = vfsuid_into_kuid(vfsuid);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e3c603d01337..0eba09a21cf7 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2165,6 +2165,7 @@ struct inode_operations {
>  			    struct dentry *dentry, struct fileattr
> *fa);
>  	int (*fileattr_get)(struct dentry *dentry, struct fileattr
> *fa);
>  	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> +	u64 (*get_ino)(const struct inode *inode);
>  } ____cacheline_aligned;
>  
>  static inline int call_mmap(struct file *file, struct vm_area_struct
> *vma)
> @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file,
> struct vm_area_struct *vma)
>  	return file->f_op->mmap(file, vma);
>  }
>  
> +static inline u64 inode_get_ino(struct inode *inode)
> +{
> +	if (unlikely(inode->i_op->get_ino))
> +		return inode->i_op->get_ino(inode);
> +
> +	return inode->i_ino;
> +}
> +
>  extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t
> *);
>  extern ssize_t vfs_write(struct file *, const char __user *, size_t,
> loff_t *);
>  extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct
> file *,

There should be no need to add this callback to generic_fillattr().

generic_fillattr() is a helper function for use by the filesystems
themselves. It should never be called from any outside functions, as
the inode number would be far from the only attribute that will be
incorrect.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Thu, Oct 10, 2024 at 03:28:12PM -0400, Trond Myklebust wrote:
> On Thu, 2024-10-10 at 17:26 +0200, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid
> > shown
> > to user space with getattr(), other part of the kernel may still
> > expose
> > the private inode->ino through kernel logs and audit.
> > 
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64
> > bits.
> > 
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of
> > an
> > inode number.  inode_get_ino() is called by generic_fillattr().
> > 
> > Implement get_ino() for NFS.
> > 
> > Cc: Trond Myklebust <trondmy@kernel.org>
> > Cc: Anna Schumaker <anna@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> > 
> > I'm not sure about nfs_namespace_getattr(), please review carefully.
> > 
> > I guess there are other filesystems exposing inode numbers different
> > than inode->i_ino, and they should be patched too.
> > ---
> >  fs/nfs/inode.c     | 6 ++++--
> >  fs/nfs/internal.h  | 1 +
> >  fs/nfs/namespace.c | 2 ++
> >  fs/stat.c          | 2 +-
> >  include/linux/fs.h | 9 +++++++++
> >  5 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 542c7d97b235..5dfc176b6d92 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
> >  
> >  /**
> >   * nfs_compat_user_ino64 - returns the user-visible inode number
> > - * @fileid: 64-bit fileid
> > + * @inode: inode pointer
> >   *
> >   * This function returns a 32-bit inode number if the boot parameter
> >   * nfs.enable_ino64 is zero.
> >   */
> > -u64 nfs_compat_user_ino64(u64 fileid)
> > +u64 nfs_compat_user_ino64(const struct *inode)
> >  {
> >  #ifdef CONFIG_COMPAT
> >  	compat_ulong_t ino;
> >  #else	
> >  	unsigned long ino;
> >  #endif
> > +	u64 fileid = NFS_FILEID(inode);
> >  
> >  	if (enable_ino64)
> >  		return fileid;
> > @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
> >  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
> >  	return ino;
> >  }
> > +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
> >  
> >  int nfs_drop_inode(struct inode *inode)
> >  {
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index 430733e3eff2..f5555a71a733 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode
> > *inode);
> >  extern void nfs_set_cache_invalid(struct inode *inode, unsigned long
> > flags);
> >  extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
> >  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int
> > mode);
> > +extern u64 nfs_compat_user_ino64(const struct *inode);
> >  
> >  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> >  /* localio.c */
> > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > index e7494cdd957e..d9b1e0606833 100644
> > --- a/fs/nfs/namespace.c
> > +++ b/fs/nfs/namespace.c
> > @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap,
> > struct dentry *dentry,
> >  const struct inode_operations nfs_mountpoint_inode_operations = {
> >  	.getattr	= nfs_getattr,
> >  	.setattr	= nfs_setattr,
> > +	.get_ino	= nfs_compat_user_ino64,
> >  };
> >  
> >  const struct inode_operations nfs_referral_inode_operations = {
> >  	.getattr	= nfs_namespace_getattr,
> >  	.setattr	= nfs_namespace_setattr,
> > +	.get_ino	= nfs_compat_user_ino64,
> >  };
> >  
> >  static void nfs_expire_automounts(struct work_struct *work)
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 41e598376d7e..05636919f94b 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32
> > request_mask,
> >  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> >  
> >  	stat->dev = inode->i_sb->s_dev;
> > -	stat->ino = inode->i_ino;
> > +	stat->ino = inode_get_ino(inode);
> >  	stat->mode = inode->i_mode;
> >  	stat->nlink = inode->i_nlink;
> >  	stat->uid = vfsuid_into_kuid(vfsuid);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index e3c603d01337..0eba09a21cf7 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2165,6 +2165,7 @@ struct inode_operations {
> >  			    struct dentry *dentry, struct fileattr
> > *fa);
> >  	int (*fileattr_get)(struct dentry *dentry, struct fileattr
> > *fa);
> >  	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> > +	u64 (*get_ino)(const struct inode *inode);
> >  } ____cacheline_aligned;
> >  
> >  static inline int call_mmap(struct file *file, struct vm_area_struct
> > *vma)
> > @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file,
> > struct vm_area_struct *vma)
> >  	return file->f_op->mmap(file, vma);
> >  }
> >  
> > +static inline u64 inode_get_ino(struct inode *inode)
> > +{
> > +	if (unlikely(inode->i_op->get_ino))
> > +		return inode->i_op->get_ino(inode);
> > +
> > +	return inode->i_ino;
> > +}
> > +
> >  extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t
> > *);
> >  extern ssize_t vfs_write(struct file *, const char __user *, size_t,
> > loff_t *);
> >  extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct
> > file *,
> 
> There should be no need to add this callback to generic_fillattr().
> 
> generic_fillattr() is a helper function for use by the filesystems
> themselves. It should never be called from any outside functions, as
> the inode number would be far from the only attribute that will be
> incorrect.

This change will not impact filesystems except the ones that implement the new
get_ino() operation, and I suspect NFS is not or will not be the only one.  We
need to investigate on other filesystems but I wanted to get a first feedback
before.  Using get_ino() in generic_fillattr() should guarantee a consistent
getattr() wrt inode numbers.  I forgot to remove the now-useless call to
nfs_compat_user_ino64() in nfs_getattr() for this to make sense:

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 542c7d97b235..78a9e907c905 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -981,7 +983,6 @@ int nfs_getattr(struct mnt_idmap *idmap, const struct path *path,
        stat->result_mask = nfs_get_valid_attrmask(inode) | request_mask;

        generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
-       stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
        stat->change_cookie = inode_peek_iversion_raw(inode);
        stat->attributes_mask |= STATX_ATTR_CHANGE_MONOTONIC;
        if (server->change_attr_type != NFS4_CHANGE_TYPE_IS_UNDEFINED)

Al, Christian, what do you think about this generic_fillattr() change?

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Trond Myklebust]

On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> On Thu, Oct 10, 2024 at 03:28:12PM -0400, Trond Myklebust wrote:
> > On Thu, 2024-10-10 at 17:26 +0200, Mickaël Salaün wrote:
> > > When a filesystem manages its own inode numbers, like NFS's
> > > fileid
> > > shown
> > > to user space with getattr(), other part of the kernel may still
> > > expose
> > > the private inode->ino through kernel logs and audit.
> > > 
> > > Another issue is on 32-bit architectures, on which ino_t is 32
> > > bits,
> > > whereas the user space's view of an inode number can still be 64
> > > bits.
> > > 
> > > Add a new inode_get_ino() helper calling the new struct
> > > inode_operations' get_ino() when set, to get the user space's
> > > view of
> > > an
> > > inode number.  inode_get_ino() is called by generic_fillattr().
> > > 
> > > Implement get_ino() for NFS.
> > > 
> > > Cc: Trond Myklebust <trondmy@kernel.org>
> > > Cc: Anna Schumaker <anna@kernel.org>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > > 
> > > I'm not sure about nfs_namespace_getattr(), please review
> > > carefully.
> > > 
> > > I guess there are other filesystems exposing inode numbers
> > > different
> > > than inode->i_ino, and they should be patched too.
> > > ---
> > >  fs/nfs/inode.c     | 6 ++++--
> > >  fs/nfs/internal.h  | 1 +
> > >  fs/nfs/namespace.c | 2 ++
> > >  fs/stat.c          | 2 +-
> > >  include/linux/fs.h | 9 +++++++++
> > >  5 files changed, 17 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index 542c7d97b235..5dfc176b6d92 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
> > >  
> > >  /**
> > >   * nfs_compat_user_ino64 - returns the user-visible inode number
> > > - * @fileid: 64-bit fileid
> > > + * @inode: inode pointer
> > >   *
> > >   * This function returns a 32-bit inode number if the boot
> > > parameter
> > >   * nfs.enable_ino64 is zero.
> > >   */
> > > -u64 nfs_compat_user_ino64(u64 fileid)
> > > +u64 nfs_compat_user_ino64(const struct *inode)
> > >  {
> > >  #ifdef CONFIG_COMPAT
> > >  	compat_ulong_t ino;
> > >  #else	
> > >  	unsigned long ino;
> > >  #endif
> > > +	u64 fileid = NFS_FILEID(inode);
> > >  
> > >  	if (enable_ino64)
> > >  		return fileid;
> > > @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
> > >  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) *
> > > 8;
> > >  	return ino;
> > >  }
> > > +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
> > >  
> > >  int nfs_drop_inode(struct inode *inode)
> > >  {
> > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > > index 430733e3eff2..f5555a71a733 100644
> > > --- a/fs/nfs/internal.h
> > > +++ b/fs/nfs/internal.h
> > > @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode
> > > *inode);
> > >  extern void nfs_set_cache_invalid(struct inode *inode, unsigned
> > > long
> > > flags);
> > >  extern bool nfs_check_cache_invalid(struct inode *, unsigned
> > > long);
> > >  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int
> > > mode);
> > > +extern u64 nfs_compat_user_ino64(const struct *inode);
> > >  
> > >  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > >  /* localio.c */
> > > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > > index e7494cdd957e..d9b1e0606833 100644
> > > --- a/fs/nfs/namespace.c
> > > +++ b/fs/nfs/namespace.c
> > > @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap
> > > *idmap,
> > > struct dentry *dentry,
> > >  const struct inode_operations nfs_mountpoint_inode_operations =
> > > {
> > >  	.getattr	= nfs_getattr,
> > >  	.setattr	= nfs_setattr,
> > > +	.get_ino	= nfs_compat_user_ino64,
> > >  };
> > >  
> > >  const struct inode_operations nfs_referral_inode_operations = {
> > >  	.getattr	= nfs_namespace_getattr,
> > >  	.setattr	= nfs_namespace_setattr,
> > > +	.get_ino	= nfs_compat_user_ino64,
> > >  };
> > >  
> > >  static void nfs_expire_automounts(struct work_struct *work)
> > > diff --git a/fs/stat.c b/fs/stat.c
> > > index 41e598376d7e..05636919f94b 100644
> > > --- a/fs/stat.c
> > > +++ b/fs/stat.c
> > > @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap,
> > > u32
> > > request_mask,
> > >  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> > >  
> > >  	stat->dev = inode->i_sb->s_dev;
> > > -	stat->ino = inode->i_ino;
> > > +	stat->ino = inode_get_ino(inode);
> > >  	stat->mode = inode->i_mode;
> > >  	stat->nlink = inode->i_nlink;
> > >  	stat->uid = vfsuid_into_kuid(vfsuid);
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index e3c603d01337..0eba09a21cf7 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2165,6 +2165,7 @@ struct inode_operations {
> > >  			    struct dentry *dentry, struct
> > > fileattr
> > > *fa);
> > >  	int (*fileattr_get)(struct dentry *dentry, struct
> > > fileattr
> > > *fa);
> > >  	struct offset_ctx *(*get_offset_ctx)(struct inode
> > > *inode);
> > > +	u64 (*get_ino)(const struct inode *inode);
> > >  } ____cacheline_aligned;
> > >  
> > >  static inline int call_mmap(struct file *file, struct
> > > vm_area_struct
> > > *vma)
> > > @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file
> > > *file,
> > > struct vm_area_struct *vma)
> > >  	return file->f_op->mmap(file, vma);
> > >  }
> > >  
> > > +static inline u64 inode_get_ino(struct inode *inode)
> > > +{
> > > +	if (unlikely(inode->i_op->get_ino))
> > > +		return inode->i_op->get_ino(inode);
> > > +
> > > +	return inode->i_ino;
> > > +}
> > > +
> > >  extern ssize_t vfs_read(struct file *, char __user *, size_t,
> > > loff_t
> > > *);
> > >  extern ssize_t vfs_write(struct file *, const char __user *,
> > > size_t,
> > > loff_t *);
> > >  extern ssize_t vfs_copy_file_range(struct file *, loff_t ,
> > > struct
> > > file *,
> > 
> > There should be no need to add this callback to generic_fillattr().
> > 
> > generic_fillattr() is a helper function for use by the filesystems
> > themselves. It should never be called from any outside functions,
> > as
> > the inode number would be far from the only attribute that will be
> > incorrect.
> 
> This change will not impact filesystems except the ones that
> implement the new
> get_ino() operation, and I suspect NFS is not or will not be the only
> one.  We
> need to investigate on other filesystems but I wanted to get a first
> feedback
> before.  Using get_ino() in generic_fillattr() should guarantee a
> consistent
> getattr() wrt inode numbers.  I forgot to remove the now-useless call
> to
> nfs_compat_user_ino64() in nfs_getattr() for this to make sense:

You're missing my point. From the point of view of NFS, all you're
doing is to replace a relatively fast direct call to
nfs_compat_user_ino64() with a much slower callback. There is no
benefit at all to anyone in doing so.

Yes, other filesystems may also want to replace this and/or other
fields in the "struct kstat" that they return, but none of them should
have a problem with doing that after the actual call to
generic_fillattr().


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 08:22:51AM -0400, Trond Myklebust wrote:
> On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> > On Thu, Oct 10, 2024 at 03:28:12PM -0400, Trond Myklebust wrote:
> > > On Thu, 2024-10-10 at 17:26 +0200, Mickaël Salaün wrote:
> > > > When a filesystem manages its own inode numbers, like NFS's
> > > > fileid
> > > > shown
> > > > to user space with getattr(), other part of the kernel may still
> > > > expose
> > > > the private inode->ino through kernel logs and audit.
> > > > 
> > > > Another issue is on 32-bit architectures, on which ino_t is 32
> > > > bits,
> > > > whereas the user space's view of an inode number can still be 64
> > > > bits.
> > > > 
> > > > Add a new inode_get_ino() helper calling the new struct
> > > > inode_operations' get_ino() when set, to get the user space's
> > > > view of
> > > > an
> > > > inode number.  inode_get_ino() is called by generic_fillattr().
> > > > 
> > > > Implement get_ino() for NFS.
> > > > 
> > > > Cc: Trond Myklebust <trondmy@kernel.org>
> > > > Cc: Anna Schumaker <anna@kernel.org>
> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > Cc: Jan Kara <jack@suse.cz>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > ---
> > > > 
> > > > I'm not sure about nfs_namespace_getattr(), please review
> > > > carefully.
> > > > 
> > > > I guess there are other filesystems exposing inode numbers
> > > > different
> > > > than inode->i_ino, and they should be patched too.
> > > > ---
> > > >  fs/nfs/inode.c     | 6 ++++--
> > > >  fs/nfs/internal.h  | 1 +
> > > >  fs/nfs/namespace.c | 2 ++
> > > >  fs/stat.c          | 2 +-
> > > >  include/linux/fs.h | 9 +++++++++
> > > >  5 files changed, 17 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > > index 542c7d97b235..5dfc176b6d92 100644
> > > > --- a/fs/nfs/inode.c
> > > > +++ b/fs/nfs/inode.c
> > > > @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
> > > >  
> > > >  /**
> > > >   * nfs_compat_user_ino64 - returns the user-visible inode number
> > > > - * @fileid: 64-bit fileid
> > > > + * @inode: inode pointer
> > > >   *
> > > >   * This function returns a 32-bit inode number if the boot
> > > > parameter
> > > >   * nfs.enable_ino64 is zero.
> > > >   */
> > > > -u64 nfs_compat_user_ino64(u64 fileid)
> > > > +u64 nfs_compat_user_ino64(const struct *inode)
> > > >  {
> > > >  #ifdef CONFIG_COMPAT
> > > >  	compat_ulong_t ino;
> > > >  #else	
> > > >  	unsigned long ino;
> > > >  #endif
> > > > +	u64 fileid = NFS_FILEID(inode);
> > > >  
> > > >  	if (enable_ino64)
> > > >  		return fileid;
> > > > @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
> > > >  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) *
> > > > 8;
> > > >  	return ino;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
> > > >  
> > > >  int nfs_drop_inode(struct inode *inode)
> > > >  {
> > > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > > > index 430733e3eff2..f5555a71a733 100644
> > > > --- a/fs/nfs/internal.h
> > > > +++ b/fs/nfs/internal.h
> > > > @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode
> > > > *inode);
> > > >  extern void nfs_set_cache_invalid(struct inode *inode, unsigned
> > > > long
> > > > flags);
> > > >  extern bool nfs_check_cache_invalid(struct inode *, unsigned
> > > > long);
> > > >  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int
> > > > mode);
> > > > +extern u64 nfs_compat_user_ino64(const struct *inode);
> > > >  
> > > >  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > > >  /* localio.c */
> > > > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > > > index e7494cdd957e..d9b1e0606833 100644
> > > > --- a/fs/nfs/namespace.c
> > > > +++ b/fs/nfs/namespace.c
> > > > @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap
> > > > *idmap,
> > > > struct dentry *dentry,
> > > >  const struct inode_operations nfs_mountpoint_inode_operations =
> > > > {
> > > >  	.getattr	= nfs_getattr,
> > > >  	.setattr	= nfs_setattr,
> > > > +	.get_ino	= nfs_compat_user_ino64,
> > > >  };
> > > >  
> > > >  const struct inode_operations nfs_referral_inode_operations = {
> > > >  	.getattr	= nfs_namespace_getattr,
> > > >  	.setattr	= nfs_namespace_setattr,
> > > > +	.get_ino	= nfs_compat_user_ino64,
> > > >  };
> > > >  
> > > >  static void nfs_expire_automounts(struct work_struct *work)
> > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > index 41e598376d7e..05636919f94b 100644
> > > > --- a/fs/stat.c
> > > > +++ b/fs/stat.c
> > > > @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap,
> > > > u32
> > > > request_mask,
> > > >  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> > > >  
> > > >  	stat->dev = inode->i_sb->s_dev;
> > > > -	stat->ino = inode->i_ino;
> > > > +	stat->ino = inode_get_ino(inode);
> > > >  	stat->mode = inode->i_mode;
> > > >  	stat->nlink = inode->i_nlink;
> > > >  	stat->uid = vfsuid_into_kuid(vfsuid);
> > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > index e3c603d01337..0eba09a21cf7 100644
> > > > --- a/include/linux/fs.h
> > > > +++ b/include/linux/fs.h
> > > > @@ -2165,6 +2165,7 @@ struct inode_operations {
> > > >  			    struct dentry *dentry, struct
> > > > fileattr
> > > > *fa);
> > > >  	int (*fileattr_get)(struct dentry *dentry, struct
> > > > fileattr
> > > > *fa);
> > > >  	struct offset_ctx *(*get_offset_ctx)(struct inode
> > > > *inode);
> > > > +	u64 (*get_ino)(const struct inode *inode);
> > > >  } ____cacheline_aligned;
> > > >  
> > > >  static inline int call_mmap(struct file *file, struct
> > > > vm_area_struct
> > > > *vma)
> > > > @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file
> > > > *file,
> > > > struct vm_area_struct *vma)
> > > >  	return file->f_op->mmap(file, vma);
> > > >  }
> > > >  
> > > > +static inline u64 inode_get_ino(struct inode *inode)
> > > > +{
> > > > +	if (unlikely(inode->i_op->get_ino))
> > > > +		return inode->i_op->get_ino(inode);
> > > > +
> > > > +	return inode->i_ino;
> > > > +}
> > > > +
> > > >  extern ssize_t vfs_read(struct file *, char __user *, size_t,
> > > > loff_t
> > > > *);
> > > >  extern ssize_t vfs_write(struct file *, const char __user *,
> > > > size_t,
> > > > loff_t *);
> > > >  extern ssize_t vfs_copy_file_range(struct file *, loff_t ,
> > > > struct
> > > > file *,
> > > 
> > > There should be no need to add this callback to generic_fillattr().
> > > 
> > > generic_fillattr() is a helper function for use by the filesystems
> > > themselves. It should never be called from any outside functions,
> > > as
> > > the inode number would be far from the only attribute that will be
> > > incorrect.
> > 
> > This change will not impact filesystems except the ones that
> > implement the new
> > get_ino() operation, and I suspect NFS is not or will not be the only
> > one.  We
> > need to investigate on other filesystems but I wanted to get a first
> > feedback
> > before.  Using get_ino() in generic_fillattr() should guarantee a
> > consistent
> > getattr() wrt inode numbers.  I forgot to remove the now-useless call
> > to
> > nfs_compat_user_ino64() in nfs_getattr() for this to make sense:
> 
> You're missing my point. From the point of view of NFS, all you're
> doing is to replace a relatively fast direct call to
> nfs_compat_user_ino64() with a much slower callback. There is no
> benefit at all to anyone in doing so.
> 
> Yes, other filesystems may also want to replace this and/or other
> fields in the "struct kstat" that they return, but none of them should
> have a problem with doing that after the actual call to
> generic_fillattr().

OK, I'll remove this part then.

> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 02:38:29PM +0200, Mickaël Salaün wrote:
> On Fri, Oct 11, 2024 at 08:22:51AM -0400, Trond Myklebust wrote:
> > On Fri, 2024-10-11 at 12:15 +0200, Mickaël Salaün wrote:
> > > On Thu, Oct 10, 2024 at 03:28:12PM -0400, Trond Myklebust wrote:
> > > > On Thu, 2024-10-10 at 17:26 +0200, Mickaël Salaün wrote:
> > > > > When a filesystem manages its own inode numbers, like NFS's
> > > > > fileid
> > > > > shown
> > > > > to user space with getattr(), other part of the kernel may still
> > > > > expose
> > > > > the private inode->ino through kernel logs and audit.
> > > > > 
> > > > > Another issue is on 32-bit architectures, on which ino_t is 32
> > > > > bits,
> > > > > whereas the user space's view of an inode number can still be 64
> > > > > bits.
> > > > > 
> > > > > Add a new inode_get_ino() helper calling the new struct
> > > > > inode_operations' get_ino() when set, to get the user space's
> > > > > view of
> > > > > an
> > > > > inode number.  inode_get_ino() is called by generic_fillattr().
> > > > > 
> > > > > Implement get_ino() for NFS.
> > > > > 
> > > > > Cc: Trond Myklebust <trondmy@kernel.org>
> > > > > Cc: Anna Schumaker <anna@kernel.org>
> > > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > Cc: Jan Kara <jack@suse.cz>
> > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > ---
> > > > > 
> > > > > I'm not sure about nfs_namespace_getattr(), please review
> > > > > carefully.
> > > > > 
> > > > > I guess there are other filesystems exposing inode numbers
> > > > > different
> > > > > than inode->i_ino, and they should be patched too.
> > > > > ---
> > > > >  fs/nfs/inode.c     | 6 ++++--
> > > > >  fs/nfs/internal.h  | 1 +
> > > > >  fs/nfs/namespace.c | 2 ++
> > > > >  fs/stat.c          | 2 +-
> > > > >  include/linux/fs.h | 9 +++++++++
> > > > >  5 files changed, 17 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > > > index 542c7d97b235..5dfc176b6d92 100644
> > > > > --- a/fs/nfs/inode.c
> > > > > +++ b/fs/nfs/inode.c
> > > > > @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
> > > > >  
> > > > >  /**
> > > > >   * nfs_compat_user_ino64 - returns the user-visible inode number
> > > > > - * @fileid: 64-bit fileid
> > > > > + * @inode: inode pointer
> > > > >   *
> > > > >   * This function returns a 32-bit inode number if the boot
> > > > > parameter
> > > > >   * nfs.enable_ino64 is zero.
> > > > >   */
> > > > > -u64 nfs_compat_user_ino64(u64 fileid)
> > > > > +u64 nfs_compat_user_ino64(const struct *inode)
> > > > >  {
> > > > >  #ifdef CONFIG_COMPAT
> > > > >  	compat_ulong_t ino;
> > > > >  #else	
> > > > >  	unsigned long ino;
> > > > >  #endif
> > > > > +	u64 fileid = NFS_FILEID(inode);
> > > > >  
> > > > >  	if (enable_ino64)
> > > > >  		return fileid;
> > > > > @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
> > > > >  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) *
> > > > > 8;
> > > > >  	return ino;
> > > > >  }
> > > > > +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
> > > > >  
> > > > >  int nfs_drop_inode(struct inode *inode)
> > > > >  {
> > > > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > > > > index 430733e3eff2..f5555a71a733 100644
> > > > > --- a/fs/nfs/internal.h
> > > > > +++ b/fs/nfs/internal.h
> > > > > @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode
> > > > > *inode);
> > > > >  extern void nfs_set_cache_invalid(struct inode *inode, unsigned
> > > > > long
> > > > > flags);
> > > > >  extern bool nfs_check_cache_invalid(struct inode *, unsigned
> > > > > long);
> > > > >  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int
> > > > > mode);
> > > > > +extern u64 nfs_compat_user_ino64(const struct *inode);
> > > > >  
> > > > >  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > > > >  /* localio.c */
> > > > > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > > > > index e7494cdd957e..d9b1e0606833 100644
> > > > > --- a/fs/nfs/namespace.c
> > > > > +++ b/fs/nfs/namespace.c
> > > > > @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap
> > > > > *idmap,
> > > > > struct dentry *dentry,
> > > > >  const struct inode_operations nfs_mountpoint_inode_operations =
> > > > > {
> > > > >  	.getattr	= nfs_getattr,
> > > > >  	.setattr	= nfs_setattr,
> > > > > +	.get_ino	= nfs_compat_user_ino64,
> > > > >  };
> > > > >  
> > > > >  const struct inode_operations nfs_referral_inode_operations = {
> > > > >  	.getattr	= nfs_namespace_getattr,
> > > > >  	.setattr	= nfs_namespace_setattr,
> > > > > +	.get_ino	= nfs_compat_user_ino64,
> > > > >  };
> > > > >  
> > > > >  static void nfs_expire_automounts(struct work_struct *work)
> > > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > > index 41e598376d7e..05636919f94b 100644
> > > > > --- a/fs/stat.c
> > > > > +++ b/fs/stat.c
> > > > > @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap,
> > > > > u32
> > > > > request_mask,
> > > > >  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> > > > >  
> > > > >  	stat->dev = inode->i_sb->s_dev;
> > > > > -	stat->ino = inode->i_ino;
> > > > > +	stat->ino = inode_get_ino(inode);
> > > > >  	stat->mode = inode->i_mode;
> > > > >  	stat->nlink = inode->i_nlink;
> > > > >  	stat->uid = vfsuid_into_kuid(vfsuid);
> > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > > index e3c603d01337..0eba09a21cf7 100644
> > > > > --- a/include/linux/fs.h
> > > > > +++ b/include/linux/fs.h
> > > > > @@ -2165,6 +2165,7 @@ struct inode_operations {
> > > > >  			    struct dentry *dentry, struct
> > > > > fileattr
> > > > > *fa);
> > > > >  	int (*fileattr_get)(struct dentry *dentry, struct
> > > > > fileattr
> > > > > *fa);
> > > > >  	struct offset_ctx *(*get_offset_ctx)(struct inode
> > > > > *inode);
> > > > > +	u64 (*get_ino)(const struct inode *inode);
> > > > >  } ____cacheline_aligned;
> > > > >  
> > > > >  static inline int call_mmap(struct file *file, struct
> > > > > vm_area_struct
> > > > > *vma)
> > > > > @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file
> > > > > *file,
> > > > > struct vm_area_struct *vma)
> > > > >  	return file->f_op->mmap(file, vma);
> > > > >  }
> > > > >  
> > > > > +static inline u64 inode_get_ino(struct inode *inode)
> > > > > +{
> > > > > +	if (unlikely(inode->i_op->get_ino))
> > > > > +		return inode->i_op->get_ino(inode);
> > > > > +
> > > > > +	return inode->i_ino;
> > > > > +}
> > > > > +
> > > > >  extern ssize_t vfs_read(struct file *, char __user *, size_t,
> > > > > loff_t
> > > > > *);
> > > > >  extern ssize_t vfs_write(struct file *, const char __user *,
> > > > > size_t,
> > > > > loff_t *);
> > > > >  extern ssize_t vfs_copy_file_range(struct file *, loff_t ,
> > > > > struct
> > > > > file *,
> > > > 
> > > > There should be no need to add this callback to generic_fillattr().
> > > > 
> > > > generic_fillattr() is a helper function for use by the filesystems
> > > > themselves. It should never be called from any outside functions,
> > > > as
> > > > the inode number would be far from the only attribute that will be
> > > > incorrect.
> > > 
> > > This change will not impact filesystems except the ones that
> > > implement the new
> > > get_ino() operation, and I suspect NFS is not or will not be the only
> > > one.  We
> > > need to investigate on other filesystems but I wanted to get a first
> > > feedback
> > > before.  Using get_ino() in generic_fillattr() should guarantee a
> > > consistent
> > > getattr() wrt inode numbers.  I forgot to remove the now-useless call
> > > to
> > > nfs_compat_user_ino64() in nfs_getattr() for this to make sense:
> > 
> > You're missing my point. From the point of view of NFS, all you're
> > doing is to replace a relatively fast direct call to
> > nfs_compat_user_ino64() with a much slower callback. There is no
> > benefit at all to anyone in doing so.
> > 
> > Yes, other filesystems may also want to replace this and/or other
> > fields in the "struct kstat" that they return, but none of them should
> > have a problem with doing that after the actual call to
> > generic_fillattr().
> 
> OK, I'll remove this part then.

My concern is about maintaining consistency.  If this get_ino()
operation is not visible to filesystem developers, I think there is a
good chance for desynchronization between the getattr() implementation
and get get_ino().  Anyway, I guess the performance argument wins.

> 
> > 
> > 
> > -- 
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> > 
> > 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Tetsuo Handa]

On 2024/10/11 0:26, Mickaël Salaün wrote:
> When a filesystem manages its own inode numbers, like NFS's fileid shown
> to user space with getattr(), other part of the kernel may still expose
> the private inode->ino through kernel logs and audit.

I can't catch what you are trying to do. What is wrong with that?

> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> whereas the user space's view of an inode number can still be 64 bits.

Currently, ino_t is 32bits on 32-bit architectures, isn't it?
Why do you need to use 64bits on 32-bit architectures?

> Add a new inode_get_ino() helper calling the new struct
> inode_operations' get_ino() when set, to get the user space's view of an
> inode number.  inode_get_ino() is called by generic_fillattr().

What does the user space's view of an inode number mean?
What does the kernel space's view of an inode number mean?

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Tetsuo Handa]

On 2024/10/11 19:12, Tetsuo Handa wrote:
> On 2024/10/11 0:26, Mickaël Salaün wrote:
>> When a filesystem manages its own inode numbers, like NFS's fileid shown
>> to user space with getattr(), other part of the kernel may still expose
>> the private inode->ino through kernel logs and audit.
> 
> I can't catch what you are trying to do. What is wrong with that?
> 
>> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
>> whereas the user space's view of an inode number can still be 64 bits.
> 
> Currently, ino_t is 32bits on 32-bit architectures, isn't it?
> Why do you need to use 64bits on 32-bit architectures?

Changing from 32bits to 64bits for communicating with userspace programs
breaks userspace programs using "ino_t" (or "unsigned long") for handling
inode numbers, doesn't it? Attempt to change from %lu to %llu will not be
acceptable unless the upper 32bits are guaranteed to be 0 on 32-bit
architectures.

Since syslogd/auditd are not the only programs that parse kernel logs and
audit logs, updating only syslogd/auditd is not sufficient. We must not break
existing userspace programs, and thus we can't change the format string.

> 
>> Add a new inode_get_ino() helper calling the new struct
>> inode_operations' get_ino() when set, to get the user space's view of an
>> inode number.  inode_get_ino() is called by generic_fillattr().
> 
> What does the user space's view of an inode number mean?
> What does the kernel space's view of an inode number mean?
> 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 07:54:58PM +0900, Tetsuo Handa wrote:
> On 2024/10/11 19:12, Tetsuo Handa wrote:
> > On 2024/10/11 0:26, Mickaël Salaün wrote:

> >> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> >> whereas the user space's view of an inode number can still be 64 bits.
> > 
> > Currently, ino_t is 32bits on 32-bit architectures, isn't it?
> > Why do you need to use 64bits on 32-bit architectures?
> 
> Changing from 32bits to 64bits for communicating with userspace programs
> breaks userspace programs using "ino_t" (or "unsigned long") for handling
> inode numbers, doesn't it? Attempt to change from %lu to %llu will not be
> acceptable unless the upper 32bits are guaranteed to be 0 on 32-bit
> architectures.
> 
> Since syslogd/auditd are not the only programs that parse kernel logs and
> audit logs, updating only syslogd/auditd is not sufficient. We must not break
> existing userspace programs, and thus we can't change the format string.

This might only break user space that wrongfully uses 32-bit variables
to store 64-bit values.  According to the UAPI, inode numbers should be
64 bits.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 07:12:17PM +0900, Tetsuo Handa wrote:
> On 2024/10/11 0:26, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid shown
> > to user space with getattr(), other part of the kernel may still expose
> > the private inode->ino through kernel logs and audit.
> 
> I can't catch what you are trying to do. What is wrong with that?

My understanding is that tomoyo_get_attributes() is used to log or
expose access requests to user space, including inode numbers.  Is that
correct?  If yes, then the inode numbers might not reflect what user
space sees with stat(2).

> 
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64 bits.
> 
> Currently, ino_t is 32bits on 32-bit architectures, isn't it?
> Why do you need to use 64bits on 32-bit architectures?

ino_t is indeed 32 bits on 32-bit architectures, but user space can
still get a 64-bit stat->ino value.

> 
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of an
> > inode number.  inode_get_ino() is called by generic_fillattr().
> 
> What does the user space's view of an inode number mean?

It's the value user space gets with stat(2).

> What does the kernel space's view of an inode number mean?

It's struct inode->i_ino and ino_t variables.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Tetsuo Handa]

On 2024/10/11 20:04, Mickaël Salaün wrote:
> On Fri, Oct 11, 2024 at 07:12:17PM +0900, Tetsuo Handa wrote:
>> On 2024/10/11 0:26, Mickaël Salaün wrote:
>>> When a filesystem manages its own inode numbers, like NFS's fileid shown
>>> to user space with getattr(), other part of the kernel may still expose
>>> the private inode->ino through kernel logs and audit.
>>
>> I can't catch what you are trying to do. What is wrong with that?
> 
> My understanding is that tomoyo_get_attributes() is used to log or
> expose access requests to user space, including inode numbers.  Is that
> correct?  If yes, then the inode numbers might not reflect what user
> space sees with stat(2).

Several questions because I've never seen inode number beyond UINT_MAX...

Since "struct inode"->i_ino is "unsigned long" (which is 32bits on 32-bit
architectures), despite stat(2) is ready to receive inode number as 64bits,
filesystems (except NFS) did not use inode numbers beyond UINT_MAX until now
so that fs/stat.c will not hit -EOVERFLOW condition, and that resulted in
misuse of %lu for e.g. audit logs?

But NFS was already using inode numbers beyond UINT_MAX, and e.g. audit logs
had been recording incorrect values when NFS is used?

Or, some filesystems are already using inode numbers beyond UINT_MAX but the
capacity limitation on 32-bit architectures practically prevented users from
creating/mounting filesystems with so many inodes enough to require inode
numbers going beyond UINT_MAX?



You are trying to fix out-of-sync between stat(2) and e.g. audit logs
rather than introducing new feature, aren't you?

Then, what you are trying to do is OK, but TOMOYO side needs more changes.
Since TOMOYO is currently handling any numeric values (e.g. uid, gid, device
major/minor number, inode number, ioctl's cmd number) as "unsigned long",
most of "unsigned long" usage in TOMOYO needs to be updated to use "u64"
because you are about to change inode number values to always-64bits.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Fri, Oct 11, 2024 at 11:27:45PM +0900, Tetsuo Handa wrote:
> Or, some filesystems are already using inode numbers beyond UINT_MAX but the
> capacity limitation on 32-bit architectures practically prevented users from
> creating/mounting filesystems with so many inodes enough to require inode
> numbers going beyond UINT_MAX?

Plenty of file systems use 64-bit inode numbers.  XFS and btrfs for
example if you care about commonly used local file systems.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 11:27:45PM +0900, Tetsuo Handa wrote:
> On 2024/10/11 20:04, Mickaël Salaün wrote:
> > On Fri, Oct 11, 2024 at 07:12:17PM +0900, Tetsuo Handa wrote:
> >> On 2024/10/11 0:26, Mickaël Salaün wrote:
> >>> When a filesystem manages its own inode numbers, like NFS's fileid shown
> >>> to user space with getattr(), other part of the kernel may still expose
> >>> the private inode->ino through kernel logs and audit.
> >>
> >> I can't catch what you are trying to do. What is wrong with that?
> > 
> > My understanding is that tomoyo_get_attributes() is used to log or
> > expose access requests to user space, including inode numbers.  Is that
> > correct?  If yes, then the inode numbers might not reflect what user
> > space sees with stat(2).
> 
> Several questions because I've never seen inode number beyond UINT_MAX...
> 
> Since "struct inode"->i_ino is "unsigned long" (which is 32bits on 32-bit
> architectures), despite stat(2) is ready to receive inode number as 64bits,
> filesystems (except NFS) did not use inode numbers beyond UINT_MAX until now
> so that fs/stat.c will not hit -EOVERFLOW condition, and that resulted in
> misuse of %lu for e.g. audit logs?

Yes, I think other filesystems (e.g. tmpfs) only enable 64-bit inodes on
64-bit architectures.

> 
> But NFS was already using inode numbers beyond UINT_MAX, and e.g. audit logs
> had been recording incorrect values when NFS is used?

Correct, all the logs with NFS inodes are wrong.

> 
> Or, some filesystems are already using inode numbers beyond UINT_MAX but the
> capacity limitation on 32-bit architectures practically prevented users from
> creating/mounting filesystems with so many inodes enough to require inode
> numbers going beyond UINT_MAX?

I think so but I didn't take a look at all other filesystems.

> 
> 
> 
> You are trying to fix out-of-sync between stat(2) and e.g. audit logs
> rather than introducing new feature, aren't you?

Yes

> 
> Then, what you are trying to do is OK, but TOMOYO side needs more changes.
> Since TOMOYO is currently handling any numeric values (e.g. uid, gid, device
> major/minor number, inode number, ioctl's cmd number) as "unsigned long",
> most of "unsigned long" usage in TOMOYO needs to be updated to use "u64"
> because you are about to change inode number values to always-64bits.
> 

OK, could you please send a full patch in reply to this email?  I'll
include it in the next patch series.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> When a filesystem manages its own inode numbers, like NFS's fileid shown
> to user space with getattr(), other part of the kernel may still expose
> the private inode->ino through kernel logs and audit.
> 
> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> whereas the user space's view of an inode number can still be 64 bits.
> 
> Add a new inode_get_ino() helper calling the new struct
> inode_operations' get_ino() when set, to get the user space's view of an
> inode number.  inode_get_ino() is called by generic_fillattr().
> 
> Implement get_ino() for NFS.

The proper interface for that is ->getattr, and you should use that for
all file systems (through the proper vfs wrappers).

And yes, it's really time to move to a 64-bit i_ino, but that's a
separate discussion.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 05:30:12AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid shown
> > to user space with getattr(), other part of the kernel may still expose
> > the private inode->ino through kernel logs and audit.
> > 
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64 bits.
> > 
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of an
> > inode number.  inode_get_ino() is called by generic_fillattr().
> > 
> > Implement get_ino() for NFS.
> 
> The proper interface for that is ->getattr, and you should use that for
> all file systems (through the proper vfs wrappers).

How to get the inode number with ->getattr and only a struct inode?

> 
> And yes, it's really time to move to a 64-bit i_ino, but that's a
> separate discussion.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Fri, Oct 11, 2024 at 02:47:14PM +0200, Mickaël Salaün wrote:
> How to get the inode number with ->getattr and only a struct inode?

You get a struct kstat and extract it from that.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 05:54:37AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 11, 2024 at 02:47:14PM +0200, Mickaël Salaün wrote:
> > How to get the inode number with ->getattr and only a struct inode?
> 
> You get a struct kstat and extract it from that.

Yes, but how do you call getattr() without a path?

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Fri, Oct 11, 2024 at 03:20:30PM +0200, Mickaël Salaün wrote:
> On Fri, Oct 11, 2024 at 05:54:37AM -0700, Christoph Hellwig wrote:
> > On Fri, Oct 11, 2024 at 02:47:14PM +0200, Mickaël Salaün wrote:
> > > How to get the inode number with ->getattr and only a struct inode?
> > 
> > You get a struct kstat and extract it from that.
> 
> Yes, but how do you call getattr() without a path?

You don't because inode numbers are irrelevant without the path.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 06:23:48AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 11, 2024 at 03:20:30PM +0200, Mickaël Salaün wrote:
> > On Fri, Oct 11, 2024 at 05:54:37AM -0700, Christoph Hellwig wrote:
> > > On Fri, Oct 11, 2024 at 02:47:14PM +0200, Mickaël Salaün wrote:
> > > > How to get the inode number with ->getattr and only a struct inode?
> > > 
> > > You get a struct kstat and extract it from that.
> > 
> > Yes, but how do you call getattr() without a path?
> 
> You don't because inode numbers are irrelevant without the path.

They are for kernel messages and audit logs.  Please take a look at the
use cases with the other patches.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > Yes, but how do you call getattr() without a path?
> > 
> > You don't because inode numbers are irrelevant without the path.
> 
> They are for kernel messages and audit logs.  Please take a look at the
> use cases with the other patches.

It still is useless.  E.g. btrfs has duplicate inode numbers due to
subvolumes.

If you want a better pretty but not useful value just work on making
i_ino 64-bits wide, which is long overdue.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > > Yes, but how do you call getattr() without a path?
> > > 
> > > You don't because inode numbers are irrelevant without the path.
> > 
> > They are for kernel messages and audit logs.  Please take a look at the
> > use cases with the other patches.
> 
> It still is useless.  E.g. btrfs has duplicate inode numbers due to
> subvolumes.

At least it reflects what users see.

> 
> If you want a better pretty but not useful value just work on making
> i_ino 64-bits wide, which is long overdue.

That would require too much work for me, and this would be a pain to
backport to all stable kernels.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Fri, Oct 11, 2024 at 05:30:59PM +0200, Mickaël Salaün wrote:
> > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > subvolumes.
> 
> At least it reflects what users see.

Users generally don't see inode numbers.

> > If you want a better pretty but not useful value just work on making
> > i_ino 64-bits wide, which is long overdue.
> 
> That would require too much work for me, and this would be a pain to
> backport to all stable kernels.

Well, if doing the right thing is too hard we can easily do nothing.

In case it wan't clear, this thread has been a very explicit:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Fri, Oct 11, 2024 at 08:34:35AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 11, 2024 at 05:30:59PM +0200, Mickaël Salaün wrote:
> > > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > > subvolumes.
> > 
> > At least it reflects what users see.
> 
> Users generally don't see inode numbers.
> 
> > > If you want a better pretty but not useful value just work on making
> > > i_ino 64-bits wide, which is long overdue.
> > 
> > That would require too much work for me, and this would be a pain to
> > backport to all stable kernels.
> 
> Well, if doing the right thing is too hard we can easily do nothing.
> 
> In case it wan't clear, this thread has been a very explicit:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

This must be typo and you want a NAK here, right?

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Mon, Oct 14, 2024 at 04:35:24PM +0200, Christian Brauner wrote:
> On Fri, Oct 11, 2024 at 08:34:35AM -0700, Christoph Hellwig wrote:
> > On Fri, Oct 11, 2024 at 05:30:59PM +0200, Mickaël Salaün wrote:
> > > > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > > > subvolumes.
> > > 
> > > At least it reflects what users see.
> > 
> > Users generally don't see inode numbers.
> > 
> > > > If you want a better pretty but not useful value just work on making
> > > > i_ino 64-bits wide, which is long overdue.
> > > 
> > > That would require too much work for me, and this would be a pain to
> > > backport to all stable kernels.
> > 
> > Well, if doing the right thing is too hard we can easily do nothing.
> > 
> > In case it wan't clear, this thread has been a very explicit:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> This must be typo and you want a NAK here, right?

Yes :)

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jeff Layton]

On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
> On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> > On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > > > Yes, but how do you call getattr() without a path?
> > > > 
> > > > You don't because inode numbers are irrelevant without the path.
> > > 
> > > They are for kernel messages and audit logs.  Please take a look at the
> > > use cases with the other patches.
> > 
> > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > subvolumes.
> 
> At least it reflects what users see.
> 
> > 
> > If you want a better pretty but not useful value just work on making
> > i_ino 64-bits wide, which is long overdue.
> 
> That would require too much work for me, and this would be a pain to
> backport to all stable kernels.
> 

Would it though? Adding this new inode operation seems sub-optimal.

Inode numbers are static information. Once an inode number is set on an
inode it basically never changes.  This patchset will turn all of those
direct inode->i_ino fetches into a pointer chase for the new inode
operation, which will then almost always just result in a direct fetch.

A better solution here would be to make inode->i_ino a u64, and just
fix up all of the places that touch it to expect that. Then, just
ensure that all of the filesystems set it properly when instantiating
new inodes. Even on 32-bit arch, you likely wouldn't need a seqcount
loop or anything to fetch this since the chance of a torn read there is
basically zero.

If there are places where we need to convert i_ino down to 32-bits,
then we can just use some scheme like nfs_fattr_to_ino_t(), or just
cast i_ino to a u32.

Yes, it'd be a larger patchset, but that seems like it would be a
better solution.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Burn Alting]

On 13/10/24 21:17, Jeff Layton wrote:
> On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
>> On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
>>> On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
>>>>>> Yes, but how do you call getattr() without a path?
>>>>>
>>>>> You don't because inode numbers are irrelevant without the path.
>>>>
>>>> They are for kernel messages and audit logs.  Please take a look at the
>>>> use cases with the other patches.
>>>
>>> It still is useless.  E.g. btrfs has duplicate inode numbers due to
>>> subvolumes.
>>
>> At least it reflects what users see.
>>
>>>
>>> If you want a better pretty but not useful value just work on making
>>> i_ino 64-bits wide, which is long overdue.
>>
>> That would require too much work for me, and this would be a pain to
>> backport to all stable kernels.
>>
> 
> Would it though? Adding this new inode operation seems sub-optimal.
> 
> Inode numbers are static information. Once an inode number is set on an
> inode it basically never changes.  This patchset will turn all of those
> direct inode->i_ino fetches into a pointer chase for the new inode
> operation, which will then almost always just result in a direct fetch.
> 
> A better solution here would be to make inode->i_ino a u64, and just
> fix up all of the places that touch it to expect that. Then, just
> ensure that all of the filesystems set it properly when instantiating
> new inodes. Even on 32-bit arch, you likely wouldn't need a seqcount
> loop or anything to fetch this since the chance of a torn read there is
> basically zero.
> 
> If there are places where we need to convert i_ino down to 32-bits,
> then we can just use some scheme like nfs_fattr_to_ino_t(), or just
> cast i_ino to a u32.
> 
> Yes, it'd be a larger patchset, but that seems like it would be a
> better solution.
As someone who lives in the analytical user space of Linux audit,  I 
take it that for large (say >200TB) file systems, the inode value 
reported in audit PATH records is no longer forensically defensible and 
it's use as a correlation item is of questionable value now?

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Mon, Oct 14, 2024 at 07:40:37PM +1100, Burn Alting wrote:
> As someone who lives in the analytical user space of Linux audit,  I take it
> that for large (say >200TB) file systems, the inode value reported in audit
> PATH records is no longer forensically defensible and it's use as a
> correlation item is of questionable value now?

What do you mean with forensically defensible?

A 64-bit inode number is supposed to be unique.  Some file systems
(most notably btrfs, but probably also various non-native file system)
break and this, and get away with lots of userspace hacks papering
over it.  If you are on a 32-bit system and not using the LFS APIs
stat will fail with -EOVERFLOW.  Some file systems have options to
never generate > 32bit inode numbers.  None of that is directly
related to file system size, although at least for XFS file system
size is one relevant variable, but 200TB is in no way relevant.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Burn Alting]

On 14/10/24 20:02, Christoph Hellwig wrote:
> On Mon, Oct 14, 2024 at 07:40:37PM +1100, Burn Alting wrote:
>> As someone who lives in the analytical user space of Linux audit,  I take it
>> that for large (say >200TB) file systems, the inode value reported in audit
>> PATH records is no longer forensically defensible and it's use as a
>> correlation item is of questionable value now?
> 
> What do you mean with forensically defensible?

If the auditd system only maintains a 32 bit variable for an inode 
value, when it emits an inode number, then how does one categorically 
state/defend that the inode value in the audit event is the actual one 
on the file system. The PATH record will offer one value (32 bits) but 
the returned inode value from a stat will return another (the actual 64 
bit value). Basically auditd would not be recording the correct value.

> 
> A 64-bit inode number is supposed to be unique.  Some file systems
> (most notably btrfs, but probably also various non-native file system)
> break and this, and get away with lots of userspace hacks papering
> over it.  If you are on a 32-bit system and not using the LFS APIs
> stat will fail with -EOVERFLOW.  Some file systems have options to
> never generate > 32bit inode numbers.  None of that is directly
> related to file system size, although at least for XFS file system
> size is one relevant variable, but 200TB is in no way relevant.

My reference to the filesystem size was a quick and dirty estimate of 
when one may see more than 2^32 inodes on a single filesystem. What I 
failed to state (my apologies) is that this presumed an xfs filesystem 
with default values when creating the file system. (A quick check on an 
single 240TB xfs filesystem advised more than 5000000000 inodes were 
available).

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Mon, Oct 14, 2024 at 11:12:25PM +1100, Burn Alting wrote:
> > > PATH records is no longer forensically defensible and it's use as a
> > > correlation item is of questionable value now?
> > 
> > What do you mean with forensically defensible?
> 
> If the auditd system only maintains a 32 bit variable for an inode value,
> when it emits an inode number, then how does one categorically state/defend
> that the inode value in the audit event is the actual one on the file
> system. The PATH record will offer one value (32 bits) but the returned
> inode value from a stat will return another (the actual 64 bit value).
> Basically auditd would not be recording the correct value.

Does auditd only track 32-bit inodes?  If yes, it is fundamentally
broken.

> My reference to the filesystem size was a quick and dirty estimate of when
> one may see more than 2^32 inodes on a single filesystem. What I failed to
> state (my apologies) is that this presumed an xfs filesystem with default
> values when creating the file system. (A quick check on an single 240TB xfs
> filesystem advised more than 5000000000 inodes were available).

For XFS inode number encoding is sparse, with part of it encoding the
allocation group it resides in.  For btrfs for example the inode number
is simply a 64-bit monotonically increasing counter per subvolume
where freed inode numbers never get reused.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Mon, Oct 14, 2024 at 05:17:53AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 14, 2024 at 11:12:25PM +1100, Burn Alting wrote:
> > > > PATH records is no longer forensically defensible and it's use as a
> > > > correlation item is of questionable value now?
> > > 
> > > What do you mean with forensically defensible?
> > 
> > If the auditd system only maintains a 32 bit variable for an inode value,
> > when it emits an inode number, then how does one categorically state/defend
> > that the inode value in the audit event is the actual one on the file
> > system. The PATH record will offer one value (32 bits) but the returned
> > inode value from a stat will return another (the actual 64 bit value).
> > Basically auditd would not be recording the correct value.
> 
> Does auditd only track 32-bit inodes?  If yes, it is fundamentally
> broken.

auditd logs 32-bit inodes on 32-bit architecture, whereas it should
always log 64-bit inodes.  The goal of this patch series is to fix this
this issue for auditd and other kernel logs (and to backport these
fixes).

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jeff Layton]

On Mon, 2024-10-14 at 17:44 +1100, Burn Alting wrote:
>  
> 
> 
> 
> 
>  
> 
> 
> 
>  
> 
> 
> 
> On 13/10/24 21:17, Jeff Layton wrote:
>  
> 
> 
> 
>  
> 
> 
> 
> >  
> > 
> > 
> > 
> > On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
> >  
> > 
> > 
> > 
> > >  
> > > 
> > > 
> > > 
> > > On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> > >  
> > > 
> > > 
> > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > >  
> > > > 
> > > > 
> > > > 
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Yes, but how do you call getattr() without a path?
> > > > > > >  
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > You don't because inode numbers are irrelevant without the path.
> > > > > >  
> > > > > > 
> > > > > > 
> > > > > > 
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > > They are for kernel messages and audit logs.  Please take a look at the
> > > > > use cases with the other patches.
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > > > subvolumes.
> > > >  
> > > > 
> > > > 
> > > > 
> > >  
> > > 
> > > 
> > > 
> > > At least it reflects what users see.
> > > 
> > >  
> > > 
> > > 
> > > 
> > > >  
> > > > 
> > > > 
> > > > 
> > > > If you want a better pretty but not useful value just work on making
> > > > i_ino 64-bits wide, which is long overdue.
> > > >  
> > > > 
> > > > 
> > > > 
> > >  
> > > 
> > > 
> > > 
> > > That would require too much work for me, and this would be a pain to
> > > backport to all stable kernels.
> > > 
> > >  
> > > 
> > > 
> > > 
> >  
> > 
> > 
> > 
> > Would it though? Adding this new inode operation seems sub-optimal.
> > 
> > Inode numbers are static information. Once an inode number is set on an
> > inode it basically never changes.  This patchset will turn all of those
> > direct inode->i_ino fetches into a pointer chase for the new inode
> > operation, which will then almost always just result in a direct fetch.
> > 
> > A better solution here would be to make inode->i_ino a u64, and just
> > fix up all of the places that touch it to expect that. Then, just
> > ensure that all of the filesystems set it properly when instantiating
> > new inodes. Even on 32-bit arch, you likely wouldn't need a seqcount
> > loop or anything to fetch this since the chance of a torn read there is
> > basically zero.
> > 
> > If there are places where we need to convert i_ino down to 32-bits,
> > then we can just use some scheme like nfs_fattr_to_ino_t(), or just
> > cast i_ino to a u32.
> > 
> > Yes, it'd be a larger patchset, but that seems like it would be a
> > better solution.
> >  
> > 
> > 
> > 
>  
> 
> 
> 
> As someone who lives in the analytical user space of Linux audit,  I take it that for large (say >200TB) file systems, the inode reported in audit PATH records is no longer forensically defensible and it's use as a correlation item is of questionable value now?
>  
> 
> 
> 

It's been a while since I worked in audit, but if audit is only
reporting 32-bit inode numbers in its records, then that could be
ambiguous. It's easily possible on larger filesystems to generate more
than 2^32 inodes.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Sun, Oct 13, 2024 at 06:17:43AM -0400, Jeff Layton wrote:
> On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
> > On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> > > On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > > > > Yes, but how do you call getattr() without a path?
> > > > > 
> > > > > You don't because inode numbers are irrelevant without the path.
> > > > 
> > > > They are for kernel messages and audit logs.  Please take a look at the
> > > > use cases with the other patches.
> > > 
> > > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > > subvolumes.
> > 
> > At least it reflects what users see.
> > 
> > > 
> > > If you want a better pretty but not useful value just work on making
> > > i_ino 64-bits wide, which is long overdue.
> > 
> > That would require too much work for me, and this would be a pain to
> > backport to all stable kernels.
> > 
> 
> Would it though? Adding this new inode operation seems sub-optimal.

I agree.

> Inode numbers are static information. Once an inode number is set on an
> inode it basically never changes.  This patchset will turn all of those
> direct inode->i_ino fetches into a pointer chase for the new inode
> operation, which will then almost always just result in a direct fetch.

Yup.

> A better solution here would be to make inode->i_ino a u64, and just
> fix up all of the places that touch it to expect that. Then, just

I would like us to try and see to make this happen. I really dislike
that struct inode is full of non-explicity types.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Mon, Oct 14, 2024 at 04:45:00PM +0200, Christian Brauner wrote:
> On Sun, Oct 13, 2024 at 06:17:43AM -0400, Jeff Layton wrote:
> > On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
> > > On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> > > > On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:
> > > > > > > Yes, but how do you call getattr() without a path?
> > > > > > 
> > > > > > You don't because inode numbers are irrelevant without the path.
> > > > > 
> > > > > They are for kernel messages and audit logs.  Please take a look at the
> > > > > use cases with the other patches.
> > > > 
> > > > It still is useless.  E.g. btrfs has duplicate inode numbers due to
> > > > subvolumes.
> > > 
> > > At least it reflects what users see.
> > > 
> > > > 
> > > > If you want a better pretty but not useful value just work on making
> > > > i_ino 64-bits wide, which is long overdue.
> > > 
> > > That would require too much work for me, and this would be a pain to
> > > backport to all stable kernels.
> > > 
> > 
> > Would it though? Adding this new inode operation seems sub-optimal.
> 
> I agree.

Of course it would be better to fix the root of the issue.  Who is up
for the challenge?

> 
> > Inode numbers are static information. Once an inode number is set on an
> > inode it basically never changes.  This patchset will turn all of those
> > direct inode->i_ino fetches into a pointer chase for the new inode
> > operation, which will then almost always just result in a direct fetch.
> 
> Yup.
> 
> > A better solution here would be to make inode->i_ino a u64, and just
> > fix up all of the places that touch it to expect that. Then, just
> 
> I would like us to try and see to make this happen. I really dislike
> that struct inode is full of non-explicity types.

Also, it would be OK to backport it, right?

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Paul Moore]

On Mon, Oct 14, 2024 at 10:45 AM Christian Brauner <brauner@kernel.org> wrote:
> On Sun, Oct 13, 2024 at 06:17:43AM -0400, Jeff Layton wrote:
> > On Fri, 2024-10-11 at 17:30 +0200, Mickaël Salaün wrote:
> > > On Fri, Oct 11, 2024 at 07:39:33AM -0700, Christoph Hellwig wrote:
> > > > On Fri, Oct 11, 2024 at 03:52:42PM +0200, Mickaël Salaün wrote:

...

> > A better solution here would be to make inode->i_ino a u64, and just
> > fix up all of the places that touch it to expect that. Then, just
>
> I would like us to try and see to make this happen. I really dislike
> that struct inode is full of non-explicity types.

Presumably this would include moving all of the filesystems to use
inode->i_ino instead of their own private file ID number, e.g. the NFS
issue pointed out in the original patch?  If not, I think most of us
in the LSM/audit space still have a need for a filesystem agnostic way
to determine the inode number from an inode struct.

-- 
paul-moore.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> When a filesystem manages its own inode numbers, like NFS's fileid shown
> to user space with getattr(), other part of the kernel may still expose
> the private inode->ino through kernel logs and audit.
> 
> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> whereas the user space's view of an inode number can still be 64 bits.
> 
> Add a new inode_get_ino() helper calling the new struct
> inode_operations' get_ino() when set, to get the user space's view of an
> inode number.  inode_get_ino() is called by generic_fillattr().

I mean, you have to admit that this is a pretty blatant hack and that's
not worthy of a separate inode method, let alone the potential
performance implication that multiple people already brought up.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Mickaël Salaün]

On Mon, Oct 14, 2024 at 04:47:17PM +0200, Christian Brauner wrote:
> On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid shown
> > to user space with getattr(), other part of the kernel may still expose
> > the private inode->ino through kernel logs and audit.
> > 
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64 bits.
> > 
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of an
> > inode number.  inode_get_ino() is called by generic_fillattr().
> 
> I mean, you have to admit that this is a pretty blatant hack and that's
> not worthy of a separate inode method, let alone the potential
> performance implication that multiple people already brought up.

My initial approach was to use u64 for struct inode's i_ino, but I
realized it had too much implications on potentially all filesystems and
other parts of the kernel.  I'm sure they are much more legitimate folks
to handle this transition.  I'd be curious to know how such i_ino type
change could be backported though.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> When a filesystem manages its own inode numbers, like NFS's fileid shown
> to user space with getattr(), other part of the kernel may still expose
> the private inode->ino through kernel logs and audit.
> 
> Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> whereas the user space's view of an inode number can still be 64 bits.
> 
> Add a new inode_get_ino() helper calling the new struct
> inode_operations' get_ino() when set, to get the user space's view of an
> inode number.  inode_get_ino() is called by generic_fillattr().
> 
> Implement get_ino() for NFS.
> 
> Cc: Trond Myklebust <trondmy@kernel.org>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> I'm not sure about nfs_namespace_getattr(), please review carefully.
> 
> I guess there are other filesystems exposing inode numbers different
> than inode->i_ino, and they should be patched too.

What are the other filesystems that are presumably affected by this that
would need an inode accessor?

If this is just about NFS then just add a helper function that audit and
whatever can call if they need to know the real inode number without
forcing a new get_inode() method onto struct inode_operations.

I'm against adding a new inode_operations method unless we really have
to because it means that we grow a nasty interface that filesystem can
start using and we absolutely don't want them to.

And I don't buy that is suddenly rush hour for this. Seemingly no one
noticed this in the past idk how many years.

> ---
>  fs/nfs/inode.c     | 6 ++++--
>  fs/nfs/internal.h  | 1 +
>  fs/nfs/namespace.c | 2 ++
>  fs/stat.c          | 2 +-
>  include/linux/fs.h | 9 +++++++++
>  5 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 542c7d97b235..5dfc176b6d92 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
>  
>  /**
>   * nfs_compat_user_ino64 - returns the user-visible inode number
> - * @fileid: 64-bit fileid
> + * @inode: inode pointer
>   *
>   * This function returns a 32-bit inode number if the boot parameter
>   * nfs.enable_ino64 is zero.
>   */
> -u64 nfs_compat_user_ino64(u64 fileid)
> +u64 nfs_compat_user_ino64(const struct *inode)
>  {
>  #ifdef CONFIG_COMPAT
>  	compat_ulong_t ino;
>  #else	
>  	unsigned long ino;
>  #endif
> +	u64 fileid = NFS_FILEID(inode);
>  
>  	if (enable_ino64)
>  		return fileid;
> @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
>  		ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
>  	return ino;
>  }
> +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
>  
>  int nfs_drop_inode(struct inode *inode)
>  {
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 430733e3eff2..f5555a71a733 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode *inode);
>  extern void nfs_set_cache_invalid(struct inode *inode, unsigned long flags);
>  extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
>  extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
> +extern u64 nfs_compat_user_ino64(const struct *inode);
>  
>  #if IS_ENABLED(CONFIG_NFS_LOCALIO)
>  /* localio.c */
> diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> index e7494cdd957e..d9b1e0606833 100644
> --- a/fs/nfs/namespace.c
> +++ b/fs/nfs/namespace.c
> @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  const struct inode_operations nfs_mountpoint_inode_operations = {
>  	.getattr	= nfs_getattr,
>  	.setattr	= nfs_setattr,
> +	.get_ino	= nfs_compat_user_ino64,
>  };
>  
>  const struct inode_operations nfs_referral_inode_operations = {
>  	.getattr	= nfs_namespace_getattr,
>  	.setattr	= nfs_namespace_setattr,
> +	.get_ino	= nfs_compat_user_ino64,
>  };
>  
>  static void nfs_expire_automounts(struct work_struct *work)
> diff --git a/fs/stat.c b/fs/stat.c
> index 41e598376d7e..05636919f94b 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
>  	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
>  
>  	stat->dev = inode->i_sb->s_dev;
> -	stat->ino = inode->i_ino;
> +	stat->ino = inode_get_ino(inode);
>  	stat->mode = inode->i_mode;
>  	stat->nlink = inode->i_nlink;
>  	stat->uid = vfsuid_into_kuid(vfsuid);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e3c603d01337..0eba09a21cf7 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2165,6 +2165,7 @@ struct inode_operations {
>  			    struct dentry *dentry, struct fileattr *fa);
>  	int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
>  	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> +	u64 (*get_ino)(const struct inode *inode);
>  } ____cacheline_aligned;
>  
>  static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
>  	return file->f_op->mmap(file, vma);
>  }
>  
> +static inline u64 inode_get_ino(struct inode *inode)
> +{
> +	if (unlikely(inode->i_op->get_ino))
> +		return inode->i_op->get_ino(inode);
> +
> +	return inode->i_ino;
> +}
> +
>  extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
>  extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
>  extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
> -- 
> 2.46.1
> 

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Paul Moore]

On Wed, Oct 16, 2024 at 10:23 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid shown
> > to user space with getattr(), other part of the kernel may still expose
> > the private inode->ino through kernel logs and audit.
> >
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64 bits.
> >
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of an
> > inode number.  inode_get_ino() is called by generic_fillattr().
> >
> > Implement get_ino() for NFS.
> >
> > Cc: Trond Myklebust <trondmy@kernel.org>
> > Cc: Anna Schumaker <anna@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ---
> >
> > I'm not sure about nfs_namespace_getattr(), please review carefully.
> >
> > I guess there are other filesystems exposing inode numbers different
> > than inode->i_ino, and they should be patched too.
>
> What are the other filesystems that are presumably affected by this that
> would need an inode accessor?

I don't want to speak for Mickaël, but my reading of the patchset was
that he was suspecting that other filesystems had the same issue
(privately maintained inode numbers) and was posting this as a RFC
partly for clarity on this from the VFS developers such as yourself.

> If this is just about NFS then just add a helper function that audit and
> whatever can call if they need to know the real inode number without
> forcing a new get_inode() method onto struct inode_operations.

If this really is just limited to NFS, or perhaps NFS and a small
number of filesystems, then a a helper function is a reasonable
solution.  I think Mickaël was worried that private inode numbers
would be more common, in which case a get_ino() method makes a bit
more sense.

> And I don't buy that is suddenly rush hour for this.

I don't think Mickaël ever characterized this as a "rush hour" issue
and I know I didn't.  It definitely caught us by surprise to learn
that inode->i_no wasn't always maintained, and we want to find a
solution, but I'm not hearing anyone screaming for a solution
"yesterday".

> Seemingly no one noticed this in the past idk how many years.

Yet the issue has been noticed and we would like to find a solution,
one that is acceptable both to the VFS and LSM folks.

Can we start with compiling a list of filesystems that maintain their
inode numbers outside of inode->i_no?  NFS is obviously first on that
list, are there others that the VFS devs can add?

-- 
paul-moore.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Trond Myklebust]

On Wed, 2024-10-16 at 19:05 -0400, Paul Moore wrote:
> On Wed, Oct 16, 2024 at 10:23 AM Christian Brauner
> <brauner@kernel.org> wrote:
> > 
> > On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> > > When a filesystem manages its own inode numbers, like NFS's
> > > fileid shown
> > > to user space with getattr(), other part of the kernel may still
> > > expose
> > > the private inode->ino through kernel logs and audit.
> > > 
> > > Another issue is on 32-bit architectures, on which ino_t is 32
> > > bits,
> > > whereas the user space's view of an inode number can still be 64
> > > bits.
> > > 
> > > Add a new inode_get_ino() helper calling the new struct
> > > inode_operations' get_ino() when set, to get the user space's
> > > view of an
> > > inode number.  inode_get_ino() is called by generic_fillattr().
> > > 
> > > Implement get_ino() for NFS.
> > > 
> > > Cc: Trond Myklebust <trondmy@kernel.org>
> > > Cc: Anna Schumaker <anna@kernel.org>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > > 
> > > I'm not sure about nfs_namespace_getattr(), please review
> > > carefully.
> > > 
> > > I guess there are other filesystems exposing inode numbers
> > > different
> > > than inode->i_ino, and they should be patched too.
> > 
> > What are the other filesystems that are presumably affected by this
> > that
> > would need an inode accessor?
> 
> I don't want to speak for Mickaël, but my reading of the patchset was
> that he was suspecting that other filesystems had the same issue
> (privately maintained inode numbers) and was posting this as a RFC
> partly for clarity on this from the VFS developers such as yourself.
> 
> > If this is just about NFS then just add a helper function that
> > audit and
> > whatever can call if they need to know the real inode number
> > without
> > forcing a new get_inode() method onto struct inode_operations.
> 
> If this really is just limited to NFS, or perhaps NFS and a small
> number of filesystems, then a a helper function is a reasonable
> solution.  I think Mickaël was worried that private inode numbers
> would be more common, in which case a get_ino() method makes a bit
> more sense.
> 
> > And I don't buy that is suddenly rush hour for this.
> 
> I don't think Mickaël ever characterized this as a "rush hour" issue
> and I know I didn't.  It definitely caught us by surprise to learn
> that inode->i_no wasn't always maintained, and we want to find a
> solution, but I'm not hearing anyone screaming for a solution
> "yesterday".
> 
> > Seemingly no one noticed this in the past idk how many years.
> 
> Yet the issue has been noticed and we would like to find a solution,
> one that is acceptable both to the VFS and LSM folks.
> 
> Can we start with compiling a list of filesystems that maintain their
> inode numbers outside of inode->i_no?  NFS is obviously first on that
> list, are there others that the VFS devs can add?
> 

Pretty much any filesystem that uses 64-bit inode numbers has the same
problem: if the application calls stat(), 32-bit glibc will happily
convert that into a call to fstatat64() and then cry foul if the kernel
dares return an inode number that doesn't fit in 32 bits.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Paul Moore]

On Thu, Oct 17, 2024 at 10:30 AM Trond Myklebust
<trondmy@hammerspace.com> wrote:
> On Wed, 2024-10-16 at 19:05 -0400, Paul Moore wrote:
> > On Wed, Oct 16, 2024 at 10:23 AM Christian Brauner
> > <brauner@kernel.org> wrote:
> > >
> > > On Thu, Oct 10, 2024 at 05:26:41PM +0200, Mickaël Salaün wrote:
> > > > When a filesystem manages its own inode numbers, like NFS's
> > > > fileid shown
> > > > to user space with getattr(), other part of the kernel may still
> > > > expose
> > > > the private inode->ino through kernel logs and audit.
> > > >
> > > > Another issue is on 32-bit architectures, on which ino_t is 32
> > > > bits,
> > > > whereas the user space's view of an inode number can still be 64
> > > > bits.
> > > >
> > > > Add a new inode_get_ino() helper calling the new struct
> > > > inode_operations' get_ino() when set, to get the user space's
> > > > view of an
> > > > inode number.  inode_get_ino() is called by generic_fillattr().
> > > >
> > > > Implement get_ino() for NFS.
> > > >
> > > > Cc: Trond Myklebust <trondmy@kernel.org>
> > > > Cc: Anna Schumaker <anna@kernel.org>
> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > Cc: Jan Kara <jack@suse.cz>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > ---
> > > >
> > > > I'm not sure about nfs_namespace_getattr(), please review
> > > > carefully.
> > > >
> > > > I guess there are other filesystems exposing inode numbers
> > > > different
> > > > than inode->i_ino, and they should be patched too.
> > >
> > > What are the other filesystems that are presumably affected by this
> > > that
> > > would need an inode accessor?
> >
> > I don't want to speak for Mickaël, but my reading of the patchset was
> > that he was suspecting that other filesystems had the same issue
> > (privately maintained inode numbers) and was posting this as a RFC
> > partly for clarity on this from the VFS developers such as yourself.
> >
> > > If this is just about NFS then just add a helper function that
> > > audit and
> > > whatever can call if they need to know the real inode number
> > > without
> > > forcing a new get_inode() method onto struct inode_operations.
> >
> > If this really is just limited to NFS, or perhaps NFS and a small
> > number of filesystems, then a a helper function is a reasonable
> > solution.  I think Mickaël was worried that private inode numbers
> > would be more common, in which case a get_ino() method makes a bit
> > more sense.
> >
> > > And I don't buy that is suddenly rush hour for this.
> >
> > I don't think Mickaël ever characterized this as a "rush hour" issue
> > and I know I didn't.  It definitely caught us by surprise to learn
> > that inode->i_no wasn't always maintained, and we want to find a
> > solution, but I'm not hearing anyone screaming for a solution
> > "yesterday".
> >
> > > Seemingly no one noticed this in the past idk how many years.
> >
> > Yet the issue has been noticed and we would like to find a solution,
> > one that is acceptable both to the VFS and LSM folks.
> >
> > Can we start with compiling a list of filesystems that maintain their
> > inode numbers outside of inode->i_no?  NFS is obviously first on that
> > list, are there others that the VFS devs can add?
> >
>
> Pretty much any filesystem that uses 64-bit inode numbers has the same
> problem: if the application calls stat(), 32-bit glibc will happily
> convert that into a call to fstatat64() and then cry foul if the kernel
> dares return an inode number that doesn't fit in 32 bits.

Okay, good to know, but I was hoping that there we could come up with
an explicit list of filesystems that maintain their own private inode
numbers outside of inode-i_ino.

-- 
paul-moore.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> Okay, good to know, but I was hoping that there we could come up with
> an explicit list of filesystems that maintain their own private inode
> numbers outside of inode-i_ino.

Anything using iget5_locked is a good start.  Add to that file systems
implementing their own inode cache (at least xfs and bcachefs).

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Paul Moore]

On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > Okay, good to know, but I was hoping that there we could come up with
> > an explicit list of filesystems that maintain their own private inode
> > numbers outside of inode-i_ino.
>
> Anything using iget5_locked is a good start.  Add to that file systems
> implementing their own inode cache (at least xfs and bcachefs).

Also good to know, thanks.  However, at this point the lack of a clear
answer is making me wonder a bit more about inode numbers in the view
of VFS developers; do you folks care about inode numbers?  I'm not
asking to start an argument, it's a genuine question so I can get a
better understanding about the durability and sustainability of
inode->i_no.  If all of you (the VFS folks) aren't concerned about
inode numbers, I suspect we are going to have similar issues in the
future and we (the LSM folks) likely need to move away from reporting
inode numbers as they aren't reliably maintained by the VFS layer.

-- 
paul-moore.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Thu, Oct 17, 2024 at 11:15:49AM -0400, Paul Moore wrote:
> Also good to know, thanks.  However, at this point the lack of a clear
> answer is making me wonder a bit more about inode numbers in the view
> of VFS developers; do you folks care about inode numbers?

The VFS itself does not care much about inode numbers.  The Posix API
does, although btrfs ignores that and seems to get away with that
(mostly because applications put in btrfs-specific hacks).  Various
other non-native file systems that don't support real inodes numbers
also get away with that, but usually the applications used on those
file systems are very limited.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jan Kara]

On Thu 17-10-24 08:25:19, Christoph Hellwig wrote:
> On Thu, Oct 17, 2024 at 11:15:49AM -0400, Paul Moore wrote:
> > Also good to know, thanks.  However, at this point the lack of a clear
> > answer is making me wonder a bit more about inode numbers in the view
> > of VFS developers; do you folks care about inode numbers?
> 
> The VFS itself does not care much about inode numbers.  The Posix API
> does, although btrfs ignores that and seems to get away with that
> (mostly because applications put in btrfs-specific hacks).

Well, btrfs plays tricks with *device* numbers, right? Exactly so that
st_ino + st_dev actually stay unique for each file. Whether it matters for
audit I don't dare to say :). Bcachefs does not care and returns non-unique
inode numbers.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Thu, Oct 17, 2024 at 06:43:38PM +0200, Jan Kara wrote:
> On Thu 17-10-24 08:25:19, Christoph Hellwig wrote:
> > On Thu, Oct 17, 2024 at 11:15:49AM -0400, Paul Moore wrote:
> > > Also good to know, thanks.  However, at this point the lack of a clear
> > > answer is making me wonder a bit more about inode numbers in the view
> > > of VFS developers; do you folks care about inode numbers?
> > 
> > The VFS itself does not care much about inode numbers.  The Posix API
> > does, although btrfs ignores that and seems to get away with that
> > (mostly because applications put in btrfs-specific hacks).
> 
> Well, btrfs plays tricks with *device* numbers, right? Exactly so that
> st_ino + st_dev actually stay unique for each file. Whether it matters for
> audit I don't dare to say :). Bcachefs does not care and returns non-unique
> inode numbers.

But st_ino + st_dev is the only thing Posix and thus historically Linux
has guaranteed to applications.  So if st_dev is unique, but you need
an unknown scope in which it is unique it might as well not be for
that purpose.  And I think for any kind of audit report that is true
as well.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Thu, Oct 17, 2024 at 06:43:38PM +0200, Jan Kara wrote:
> On Thu 17-10-24 08:25:19, Christoph Hellwig wrote:
> > On Thu, Oct 17, 2024 at 11:15:49AM -0400, Paul Moore wrote:
> > > Also good to know, thanks.  However, at this point the lack of a clear
> > > answer is making me wonder a bit more about inode numbers in the view
> > > of VFS developers; do you folks care about inode numbers?
> > 
> > The VFS itself does not care much about inode numbers.  The Posix API
> > does, although btrfs ignores that and seems to get away with that
> > (mostly because applications put in btrfs-specific hacks).
> 
> Well, btrfs plays tricks with *device* numbers, right? Exactly so that
> st_ino + st_dev actually stay unique for each file. Whether it matters for

Yes.

> audit I don't dare to say :). Bcachefs does not care and returns non-unique
> inode numbers.

Userspace can now easily disambiguate them via STATX_SUBVOL.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jeff Layton]

On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig <hch@infradead.org> wrote:
> > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > Okay, good to know, but I was hoping that there we could come up with
> > > an explicit list of filesystems that maintain their own private inode
> > > numbers outside of inode-i_ino.
> > 
> > Anything using iget5_locked is a good start.  Add to that file systems
> > implementing their own inode cache (at least xfs and bcachefs).
> 
> Also good to know, thanks.  However, at this point the lack of a clear
> answer is making me wonder a bit more about inode numbers in the view
> of VFS developers; do you folks care about inode numbers?  I'm not
> asking to start an argument, it's a genuine question so I can get a
> better understanding about the durability and sustainability of
> inode->i_no.  If all of you (the VFS folks) aren't concerned about
> inode numbers, I suspect we are going to have similar issues in the
> future and we (the LSM folks) likely need to move away from reporting
> inode numbers as they aren't reliably maintained by the VFS layer.
> 

Like Christoph said, the kernel doesn't care much about inode numbers.

People care about them though, and sometimes we have things in the
kernel that report them in some fashion (tracepoints, procfiles, audit
events, etc.). Having those match what the userland stat() st_ino field
tells you is ideal, and for the most part that's the way it works.

The main exception is when people use 32-bit interfaces (somewhat rare
these days), or they have a 32-bit kernel with a filesystem that has a
64-bit inode number space (NFS being one of those). The NFS client has
basically hacked around this for years by tracking its own fileid field
in its inode. That's really a waste though. That could be converted
over to use i_ino instead if it were always wide enough.

It'd be better to stop with these sort of hacks and just fix this the
right way once and for all, by making i_ino 64 bits everywhere.

A lot of the changes can probably be automated via coccinelle. I'd
probably start by turning all of the direct i_ino accesses into static
inline wrapper function calls. The hard part will be parceling out that
work into digestable chunks. If you can avoid "flag day" changes, then
that's ideal.  You'd want a patch per subsystem so you can collect
ACKs. 

The hardest part will probably be the format string changes. I'm not
sure you can easily use coccinelle for that, so that may need to be
done by hand or scripted with python or something.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Trond Myklebust]

On Thu, 2024-10-17 at 13:05 -0400, Jeff Layton wrote:
> On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig
> > <hch@infradead.org> wrote:
> > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > Okay, good to know, but I was hoping that there we could come
> > > > up with
> > > > an explicit list of filesystems that maintain their own private
> > > > inode
> > > > numbers outside of inode-i_ino.
> > > 
> > > Anything using iget5_locked is a good start.  Add to that file
> > > systems
> > > implementing their own inode cache (at least xfs and bcachefs).
> > 
> > Also good to know, thanks.  However, at this point the lack of a
> > clear
> > answer is making me wonder a bit more about inode numbers in the
> > view
> > of VFS developers; do you folks care about inode numbers?  I'm not
> > asking to start an argument, it's a genuine question so I can get a
> > better understanding about the durability and sustainability of
> > inode->i_no.  If all of you (the VFS folks) aren't concerned about
> > inode numbers, I suspect we are going to have similar issues in the
> > future and we (the LSM folks) likely need to move away from
> > reporting
> > inode numbers as they aren't reliably maintained by the VFS layer.
> > 
> 
> Like Christoph said, the kernel doesn't care much about inode
> numbers.
> 
> People care about them though, and sometimes we have things in the
> kernel that report them in some fashion (tracepoints, procfiles,
> audit
> events, etc.). Having those match what the userland stat() st_ino
> field
> tells you is ideal, and for the most part that's the way it works.
> 
> The main exception is when people use 32-bit interfaces (somewhat
> rare
> these days), or they have a 32-bit kernel with a filesystem that has
> a
> 64-bit inode number space (NFS being one of those). The NFS client
> has
> basically hacked around this for years by tracking its own fileid
> field
> in its inode. That's really a waste though. That could be converted
> over to use i_ino instead if it were always wide enough.
> 
> It'd be better to stop with these sort of hacks and just fix this the
> right way once and for all, by making i_ino 64 bits everywhere.

Nope.

That won't fix glibc, which is the main problem NFS has to work around.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jeff Layton]

On Thu, 2024-10-17 at 17:09 +0000, Trond Myklebust wrote:
> On Thu, 2024-10-17 at 13:05 -0400, Jeff Layton wrote:
> > On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig
> > > <hch@infradead.org> wrote:
> > > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > > Okay, good to know, but I was hoping that there we could come
> > > > > up with
> > > > > an explicit list of filesystems that maintain their own private
> > > > > inode
> > > > > numbers outside of inode-i_ino.
> > > > 
> > > > Anything using iget5_locked is a good start.  Add to that file
> > > > systems
> > > > implementing their own inode cache (at least xfs and bcachefs).
> > > 
> > > Also good to know, thanks.  However, at this point the lack of a
> > > clear
> > > answer is making me wonder a bit more about inode numbers in the
> > > view
> > > of VFS developers; do you folks care about inode numbers?  I'm not
> > > asking to start an argument, it's a genuine question so I can get a
> > > better understanding about the durability and sustainability of
> > > inode->i_no.  If all of you (the VFS folks) aren't concerned about
> > > inode numbers, I suspect we are going to have similar issues in the
> > > future and we (the LSM folks) likely need to move away from
> > > reporting
> > > inode numbers as they aren't reliably maintained by the VFS layer.
> > > 
> > 
> > Like Christoph said, the kernel doesn't care much about inode
> > numbers.
> > 
> > People care about them though, and sometimes we have things in the
> > kernel that report them in some fashion (tracepoints, procfiles,
> > audit
> > events, etc.). Having those match what the userland stat() st_ino
> > field
> > tells you is ideal, and for the most part that's the way it works.
> > 
> > The main exception is when people use 32-bit interfaces (somewhat
> > rare
> > these days), or they have a 32-bit kernel with a filesystem that has
> > a
> > 64-bit inode number space (NFS being one of those). The NFS client
> > has
> > basically hacked around this for years by tracking its own fileid
> > field
> > in its inode. That's really a waste though. That could be converted
> > over to use i_ino instead if it were always wide enough.
> > 
> > It'd be better to stop with these sort of hacks and just fix this the
> > right way once and for all, by making i_ino 64 bits everywhere.
> 
> Nope.
> 
> That won't fix glibc, which is the main problem NFS has to work around.
> 

True, but that's really a separate problem.

It also doesn't inform how we track inode numbers inside the kernel.
Inode numbers have been 64 bits for years on "real" filesystems. If we
were designing this today, i_ino would be a u64, and we'd only hash
that down to 32 bits when necessary.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Trond Myklebust]

On Thu, 2024-10-17 at 13:59 -0400, Jeff Layton wrote:
> On Thu, 2024-10-17 at 17:09 +0000, Trond Myklebust wrote:
> > On Thu, 2024-10-17 at 13:05 -0400, Jeff Layton wrote:
> > > On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > > > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig
> > > > <hch@infradead.org> wrote:
> > > > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > > > Okay, good to know, but I was hoping that there we could
> > > > > > come
> > > > > > up with
> > > > > > an explicit list of filesystems that maintain their own
> > > > > > private
> > > > > > inode
> > > > > > numbers outside of inode-i_ino.
> > > > > 
> > > > > Anything using iget5_locked is a good start.  Add to that
> > > > > file
> > > > > systems
> > > > > implementing their own inode cache (at least xfs and
> > > > > bcachefs).
> > > > 
> > > > Also good to know, thanks.  However, at this point the lack of
> > > > a
> > > > clear
> > > > answer is making me wonder a bit more about inode numbers in
> > > > the
> > > > view
> > > > of VFS developers; do you folks care about inode numbers?  I'm
> > > > not
> > > > asking to start an argument, it's a genuine question so I can
> > > > get a
> > > > better understanding about the durability and sustainability of
> > > > inode->i_no.  If all of you (the VFS folks) aren't concerned
> > > > about
> > > > inode numbers, I suspect we are going to have similar issues in
> > > > the
> > > > future and we (the LSM folks) likely need to move away from
> > > > reporting
> > > > inode numbers as they aren't reliably maintained by the VFS
> > > > layer.
> > > > 
> > > 
> > > Like Christoph said, the kernel doesn't care much about inode
> > > numbers.
> > > 
> > > People care about them though, and sometimes we have things in
> > > the
> > > kernel that report them in some fashion (tracepoints, procfiles,
> > > audit
> > > events, etc.). Having those match what the userland stat() st_ino
> > > field
> > > tells you is ideal, and for the most part that's the way it
> > > works.
> > > 
> > > The main exception is when people use 32-bit interfaces (somewhat
> > > rare
> > > these days), or they have a 32-bit kernel with a filesystem that
> > > has
> > > a
> > > 64-bit inode number space (NFS being one of those). The NFS
> > > client
> > > has
> > > basically hacked around this for years by tracking its own fileid
> > > field
> > > in its inode. That's really a waste though. That could be
> > > converted
> > > over to use i_ino instead if it were always wide enough.
> > > 
> > > It'd be better to stop with these sort of hacks and just fix this
> > > the
> > > right way once and for all, by making i_ino 64 bits everywhere.
> > 
> > Nope.
> > 
> > That won't fix glibc, which is the main problem NFS has to work
> > around.
> > 
> 
> True, but that's really a separate problem.


Currently, the problem where the kernel needs to use one inode number
in iget5() and a different one when replying to stat() is limited to
the set of 64-bit kernels that can operate in 32-bit userland
compability mode. So mainly on x86_64 kernels that are set up to run in
i386 userland compatibility mode.

If you now decree that all kernels will use 64-bit inode numbers
internally, then you've suddenly expanded the problem to encompass all
the remaining 32-bit kernels. In order to avoid stat() returning
EOVERFLOW to the applications, they too will have to start generating
separate 32-bit inode numbers.

> 
> It also doesn't inform how we track inode numbers inside the kernel.
> Inode numbers have been 64 bits for years on "real" filesystems. If
> we
> were designing this today, i_ino would be a u64, and we'd only hash
> that down to 32 bits when necessary.

"I'm doing a (free) operating system (just a hobby, won't be big and
professional like gnu) for 386(486) AT clones."

History is a bitch...

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[hch]

On Thu, Oct 17, 2024 at 05:09:17PM +0000, Trond Myklebust wrote:
> > It'd be better to stop with these sort of hacks and just fix this the
> > right way once and for all, by making i_ino 64 bits everywhere.
> 
> Nope.
> 
> That won't fix glibc, which is the main problem NFS has to work around.

There's no Problem in glibc here, it's mostly the Linux syscall layer
being stupid for the 32-bit non-LFS interface.

That being said with my XFS hat on we've not really had issue with
32-bit non-LFS code for a long time.  Not saying it doesn't exist,
but it's probably limited to retro computing by now.  The way we
support it is with the inode32 option that never allocates larger
inode numbers.  I think something similar should work for NFS where
an option is required for the ino mangling.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Paul Moore]

On Thu, Oct 17, 2024 at 1:05 PM Jeff Layton <jlayton@kernel.org> wrote:
> On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > Okay, good to know, but I was hoping that there we could come up with
> > > > an explicit list of filesystems that maintain their own private inode
> > > > numbers outside of inode-i_ino.
> > >
> > > Anything using iget5_locked is a good start.  Add to that file systems
> > > implementing their own inode cache (at least xfs and bcachefs).
> >
> > Also good to know, thanks.  However, at this point the lack of a clear
> > answer is making me wonder a bit more about inode numbers in the view
> > of VFS developers; do you folks care about inode numbers?  I'm not
> > asking to start an argument, it's a genuine question so I can get a
> > better understanding about the durability and sustainability of
> > inode->i_no.  If all of you (the VFS folks) aren't concerned about
> > inode numbers, I suspect we are going to have similar issues in the
> > future and we (the LSM folks) likely need to move away from reporting
> > inode numbers as they aren't reliably maintained by the VFS layer.
> >
>
> Like Christoph said, the kernel doesn't care much about inode numbers.
>
> People care about them though, and sometimes we have things in the
> kernel that report them in some fashion (tracepoints, procfiles, audit
> events, etc.). Having those match what the userland stat() st_ino field
> tells you is ideal, and for the most part that's the way it works.
>
> The main exception is when people use 32-bit interfaces (somewhat rare
> these days), or they have a 32-bit kernel with a filesystem that has a
> 64-bit inode number space (NFS being one of those). The NFS client has
> basically hacked around this for years by tracking its own fileid field
> in its inode.

When I asked if the VFS dev cared about inode numbers this is more of
what I was wondering about.  Regardless of if the kernel itself uses
inode numbers for anything, it does appear that users do care about
inode numbers to some extent, and I wanted to know if the VFS devs
viewed the inode numbers as a first order UAPI interface/thing, or if
it was of lesser importance and not something the kernel was going to
provide much of a guarantee around.  Once again, I'm not asking this
to start a war, I'm just trying to get some perspective from the VFS
dev side of things.

> A lot of the changes can probably be automated via coccinelle. I'd
> probably start by turning all of the direct i_ino accesses into static
> inline wrapper function calls. The hard part will be parceling out that
> work into digestable chunks. If you can avoid "flag day" changes, then
> that's ideal.  You'd want a patch per subsystem so you can collect
> ACKs.
>
> The hardest part will probably be the format string changes. I'm not
> sure you can easily use coccinelle for that, so that may need to be
> done by hand or scripted with python or something.

Out of curiosity, is this on anyone's roadmap?  I've already got
enough work in security land to keep myself occupied until I'm hit by
that mythical bus, so I can't volunteer in good conscience, but I (and
many others in security land) would be grateful for a single,
consistent way to fetch inode numbers :)

-- 
paul-moore.com

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Jan Kara]

On Thu 17-10-24 16:21:34, Paul Moore wrote:
> On Thu, Oct 17, 2024 at 1:05 PM Jeff Layton <jlayton@kernel.org> wrote:
> > On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > > Okay, good to know, but I was hoping that there we could come up with
> > > > > an explicit list of filesystems that maintain their own private inode
> > > > > numbers outside of inode-i_ino.
> > > >
> > > > Anything using iget5_locked is a good start.  Add to that file systems
> > > > implementing their own inode cache (at least xfs and bcachefs).
> > >
> > > Also good to know, thanks.  However, at this point the lack of a clear
> > > answer is making me wonder a bit more about inode numbers in the view
> > > of VFS developers; do you folks care about inode numbers?  I'm not
> > > asking to start an argument, it's a genuine question so I can get a
> > > better understanding about the durability and sustainability of
> > > inode->i_no.  If all of you (the VFS folks) aren't concerned about
> > > inode numbers, I suspect we are going to have similar issues in the
> > > future and we (the LSM folks) likely need to move away from reporting
> > > inode numbers as they aren't reliably maintained by the VFS layer.
> > >
> >
> > Like Christoph said, the kernel doesn't care much about inode numbers.
> >
> > People care about them though, and sometimes we have things in the
> > kernel that report them in some fashion (tracepoints, procfiles, audit
> > events, etc.). Having those match what the userland stat() st_ino field
> > tells you is ideal, and for the most part that's the way it works.
> >
> > The main exception is when people use 32-bit interfaces (somewhat rare
> > these days), or they have a 32-bit kernel with a filesystem that has a
> > 64-bit inode number space (NFS being one of those). The NFS client has
> > basically hacked around this for years by tracking its own fileid field
> > in its inode.
> 
> When I asked if the VFS dev cared about inode numbers this is more of
> what I was wondering about.  Regardless of if the kernel itself uses
> inode numbers for anything, it does appear that users do care about
> inode numbers to some extent, and I wanted to know if the VFS devs
> viewed the inode numbers as a first order UAPI interface/thing, or if
> it was of lesser importance and not something the kernel was going to
> provide much of a guarantee around.  Once again, I'm not asking this
> to start a war, I'm just trying to get some perspective from the VFS
> dev side of things.

Well, we do care to not break our users. So our opinion about "first order
UAPI" doesn't matter that much. If userspace is using it, we have to
avoid breaking it. And there definitely is userspace depending on st_ino +
st_dev being unique identifier of a file / directory so we want to maintain
that as much as possible (at least as long as there's userspace depending
on it which I don't see changing in the near future).

That being said historically people have learned NFS has its quirks,
similarly as btrfs needing occasionally a special treatment and adapted to
it, bcachefs is new enough that userspace didn't notice yet, that's going
to be interesting.

There's another aspect that even 64-bits start to be expensive to pack
things into for some filesystems (either due to external protocol
constraints such as for AFS or due to the combination of features such as
subvolumes, snapshotting, etc.). Going to 128-bits for everybody seems
like a waste so at last LSF summit we've discussed about starting to push
file handles (output of name_to_handle_at(2)) as a replacement of st_ino
for file/dir identifier in a filesystem. For the kernel this would be
convenient because each filesystem can pack there what it needs. But
userspace guys were not thrilled by this (mainly due to the complexities of
dynamically sized identifier and passing it around). So this transition
isn't currently getting much traction and we'll see how things evolve.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Fri, Oct 18, 2024 at 02:25:43PM +0200, Jan Kara wrote:
> On Thu 17-10-24 16:21:34, Paul Moore wrote:
> > On Thu, Oct 17, 2024 at 1:05 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > > > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > > > Okay, good to know, but I was hoping that there we could come up with
> > > > > > an explicit list of filesystems that maintain their own private inode
> > > > > > numbers outside of inode-i_ino.
> > > > >
> > > > > Anything using iget5_locked is a good start.  Add to that file systems
> > > > > implementing their own inode cache (at least xfs and bcachefs).
> > > >
> > > > Also good to know, thanks.  However, at this point the lack of a clear
> > > > answer is making me wonder a bit more about inode numbers in the view
> > > > of VFS developers; do you folks care about inode numbers?  I'm not
> > > > asking to start an argument, it's a genuine question so I can get a
> > > > better understanding about the durability and sustainability of
> > > > inode->i_no.  If all of you (the VFS folks) aren't concerned about
> > > > inode numbers, I suspect we are going to have similar issues in the
> > > > future and we (the LSM folks) likely need to move away from reporting
> > > > inode numbers as they aren't reliably maintained by the VFS layer.
> > > >
> > >
> > > Like Christoph said, the kernel doesn't care much about inode numbers.
> > >
> > > People care about them though, and sometimes we have things in the
> > > kernel that report them in some fashion (tracepoints, procfiles, audit
> > > events, etc.). Having those match what the userland stat() st_ino field
> > > tells you is ideal, and for the most part that's the way it works.
> > >
> > > The main exception is when people use 32-bit interfaces (somewhat rare
> > > these days), or they have a 32-bit kernel with a filesystem that has a
> > > 64-bit inode number space (NFS being one of those). The NFS client has
> > > basically hacked around this for years by tracking its own fileid field
> > > in its inode.
> > 
> > When I asked if the VFS dev cared about inode numbers this is more of
> > what I was wondering about.  Regardless of if the kernel itself uses
> > inode numbers for anything, it does appear that users do care about
> > inode numbers to some extent, and I wanted to know if the VFS devs
> > viewed the inode numbers as a first order UAPI interface/thing, or if
> > it was of lesser importance and not something the kernel was going to
> > provide much of a guarantee around.  Once again, I'm not asking this
> > to start a war, I'm just trying to get some perspective from the VFS
> > dev side of things.
> 
> Well, we do care to not break our users. So our opinion about "first order
> UAPI" doesn't matter that much. If userspace is using it, we have to
> avoid breaking it. And there definitely is userspace depending on st_ino +
> st_dev being unique identifier of a file / directory so we want to maintain
> that as much as possible (at least as long as there's userspace depending
> on it which I don't see changing in the near future).
> 
> That being said historically people have learned NFS has its quirks,
> similarly as btrfs needing occasionally a special treatment and adapted to
> it, bcachefs is new enough that userspace didn't notice yet, that's going
> to be interesting.
> 
> There's another aspect that even 64-bits start to be expensive to pack
> things into for some filesystems (either due to external protocol
> constraints such as for AFS or due to the combination of features such as
> subvolumes, snapshotting, etc.). Going to 128-bits for everybody seems
> like a waste so at last LSF summit we've discussed about starting to push
> file handles (output of name_to_handle_at(2)) as a replacement of st_ino
> for file/dir identifier in a filesystem. For the kernel this would be
> convenient because each filesystem can pack there what it needs. But
> userspace guys were not thrilled by this (mainly due to the complexities of
> dynamically sized identifier and passing it around). So this transition
> isn't currently getting much traction and we'll see how things evolve.

It's also not an answer for every filesystem. For example, you don't
want to use file handles for pidfds when you are guaranteed that the
inode numbers will be unique. So file handles will not be used for that
where a simple statx() and comparing inode numbers can do.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christian Brauner]

On Thu, Oct 17, 2024 at 01:05:54PM -0400, Jeff Layton wrote:
> On Thu, 2024-10-17 at 11:15 -0400, Paul Moore wrote:
> > On Thu, Oct 17, 2024 at 10:58 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > On Thu, Oct 17, 2024 at 10:54:12AM -0400, Paul Moore wrote:
> > > > Okay, good to know, but I was hoping that there we could come up with
> > > > an explicit list of filesystems that maintain their own private inode
> > > > numbers outside of inode-i_ino.
> > > 
> > > Anything using iget5_locked is a good start.  Add to that file systems
> > > implementing their own inode cache (at least xfs and bcachefs).
> > 
> > Also good to know, thanks.  However, at this point the lack of a clear
> > answer is making me wonder a bit more about inode numbers in the view
> > of VFS developers; do you folks care about inode numbers?  I'm not
> > asking to start an argument, it's a genuine question so I can get a
> > better understanding about the durability and sustainability of
> > inode->i_no.  If all of you (the VFS folks) aren't concerned about
> > inode numbers, I suspect we are going to have similar issues in the
> > future and we (the LSM folks) likely need to move away from reporting
> > inode numbers as they aren't reliably maintained by the VFS layer.
> > 
> 
> Like Christoph said, the kernel doesn't care much about inode numbers.
> 
> People care about them though, and sometimes we have things in the
> kernel that report them in some fashion (tracepoints, procfiles, audit
> events, etc.). Having those match what the userland stat() st_ino field
> tells you is ideal, and for the most part that's the way it works.
> 
> The main exception is when people use 32-bit interfaces (somewhat rare
> these days), or they have a 32-bit kernel with a filesystem that has a
> 64-bit inode number space (NFS being one of those). The NFS client has
> basically hacked around this for years by tracking its own fileid field
> in its inode. That's really a waste though. That could be converted
> over to use i_ino instead if it were always wide enough.
> 
> It'd be better to stop with these sort of hacks and just fix this the
> right way once and for all, by making i_ino 64 bits everywhere.
> 
> A lot of the changes can probably be automated via coccinelle. I'd
> probably start by turning all of the direct i_ino accesses into static
> inline wrapper function calls. The hard part will be parceling out that
> work into digestable chunks. If you can avoid "flag day" changes, then
> that's ideal.  You'd want a patch per subsystem so you can collect
> ACKs. 
> 
> The hardest part will probably be the format string changes. I'm not
> sure you can easily use coccinelle for that, so that may need to be
> done by hand or scripted with python or something.

The problem where we're dealing with 64 bit inode numbers even on 32 bit
systems is one problem and porting struct inode to use a 64 bit type for
i_ino is a good thing that I agree we should explore.

I'm still not sure how that would stolve the audit problem though. The
inode numbers that audit reports, even if always 64 bit, are not unique
with e.g., btrfs and bcachefs. Audit would need to report additional
information for both filesystems like the subvolume number which would
make this consistent.

We should port to 64 bit and yes that'll take some time. Then audit may
want to grow support to report additional information such as the
subvolume number. And that can be done in various ways without having to
burden struct inode_operations with any of this. Or, document the 20
year reality that audit inode numbers aren't unique on some filesystems.

Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Christoph Hellwig]

On Wed, Oct 16, 2024 at 04:23:02PM +0200, Christian Brauner wrote:
> And I don't buy that is suddenly rush hour for this. Seemingly no one
> noticed this in the past idk how many years.

Asuming this showed up with the initial import of kernel/audit.c that
would be more than 20 years.