linux-kernel - Re: [PATCH v2] fs: improve dump_inode() to safely access inode fields.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <tno2xwbsdiq674hlvcfxmzo357wia3b6b2jxddgh4u2yvygmic@ygtdea6prr33>
Date: Sat, 10 Jan 2026 09:55:10 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Yuto Ohnuki <ytohnuki@...zon.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] fs: improve dump_inode() to safely access inode
 fields.

On Fri, Jan 09, 2026 at 03:40:19PM +0000, Yuto Ohnuki wrote:
> Use get_kernel_nofault() to safely access inode and related structures
> (superblock, file_system_type) to avoid crashing when the inode pointer
> is invalid. This allows the same pattern as dump_mapping().
> 
> Note: The original access method for i_state and i_count is preserved,
> as get_kernel_nofault() is unnecessary once the inode structure is
> verified accessible.
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Yuto Ohnuki <ytohnuki@...zon.com>
> ---
> Changes in v2:
> - Merged NULL inode->i_sb check with invalid sb check as pointed out
>   by Jan Kara;
> - Link to v1: https://lore.kernel.org/linux-fsdevel/20260101165304.34516-1-ytohnuki@amazon.com/
> ---
>  fs/inode.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 521383223d8a..c2113e4a9a6a 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2984,24 +2984,52 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
>  EXPORT_SYMBOL(mode_strip_sgid);
>  
>  #ifdef CONFIG_DEBUG_VFS
> -/*
> - * Dump an inode.
> - *
> - * TODO: add a proper inode dumping routine, this is a stub to get debug off the
> - * ground.
> +/**
> + * dump_inode - dump an inode.
> + * @inode: inode to dump
> + * @reason: reason for dumping
>   *
> - * TODO: handle getting to fs type with get_kernel_nofault()?
> - * See dump_mapping() above.
> + * If inode is an invalid pointer, we don't want to crash accessing it,
> + * so probe everything depending on it carefully with get_kernel_nofault().
>   */
>  void dump_inode(struct inode *inode, const char *reason)
>  {
> -	struct super_block *sb = inode->i_sb;
> +	struct super_block *sb;
> +	struct file_system_type *s_type;
> +	const char *fs_name_ptr;
> +	char fs_name[32] = {};
> +	umode_t mode;
> +	unsigned short opflags;
> +	unsigned int flags;
> +	unsigned int state;
> +	int count;
> +
> +	pr_warn("%s encountered for inode %px\n", reason, inode);
> +
> +	if (get_kernel_nofault(sb, &inode->i_sb) ||
> +	    get_kernel_nofault(mode, &inode->i_mode) ||
> +	    get_kernel_nofault(opflags, &inode->i_opflags) ||
> +	    get_kernel_nofault(flags, &inode->i_flags)) {
> +		pr_warn("invalid inode:%px\n", inode);
> +		return;
> +	}
>  
> -	pr_warn("%s encountered for inode %px\n"
> -		"fs %s mode %ho opflags 0x%hx flags 0x%x state 0x%x count %d\n",
> -		reason, inode, sb->s_type->name, inode->i_mode, inode->i_opflags,
> -		inode->i_flags, inode_state_read_once(inode), atomic_read(&inode->i_count));
> -}
> +	state = inode_state_read_once(inode);
> +	count = atomic_read(&inode->i_count);
> +
> +	if (!sb ||
> +	    get_kernel_nofault(s_type, &sb->s_type) || !s_type ||
> +	    get_kernel_nofault(fs_name_ptr, &s_type->name) || !fs_name_ptr) {
> +		pr_warn("invalid sb:%px mode:%ho opflags:0x%x flags:0x%x state:0x%x count:%d\n",
> +			sb, mode, opflags, flags, state, count);
> +		return;
> +	}
> +
> +	if (strncpy_from_kernel_nofault(fs_name, fs_name_ptr, sizeof(fs_name) - 1) < 0)
> +		strscpy(fs_name, "<invalid>");
>  
> +	pr_warn("fs:%s mode:%ho opflags:0x%x flags:0x%x state:0x%x count:%d\n",
> +		fs_name, mode, opflags, flags, state, count);
> +}

It would be good to avoid duplication of the pr_warn stuff as the format
string is expected to change over time. I guess you could retain
"<invalid>" for the case where sb was unreadable? Or even denote it,
perhaps with "<unknown, sb unreadable>" or similar.


I don't really care as long as tere is one pr_warn dumping the state.

This bit:
+	pr_warn("invalid inode:%px\n", inode);

could still print the passed reason. "invalid inode:" is a little
misleaing, perhaps "unreadable inode" would be better?

As a side note I was told kernel printk supports %# for printing hash
values, perhaps a good opportunity to squeeze this in intead of 0x. One
will have to test it indeed gives the expected result.