linux-kernel - Re: [PATCH 12/13] vfs: count unlinked inodes

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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 28 Oct 2011 10:20:06 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	viro@...IV.linux.org.uk, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, jack@...e.cz,
	akpm@...ux-foundation.org, toshi.okajima@...fujitsu.com
Subject: Re: [PATCH 12/13] vfs: count unlinked inodes

Christoph Hellwig <hch@...radead.org> writes:

> On Fri, Oct 28, 2011 at 09:53:53AM +0200, Miklos Szeredi wrote:
>> On Fri, 2011-10-28 at 02:08 -0400, Christoph Hellwig wrote:
>> > This one gets me a repeated spew of WARN_ONs during XFS log recover,
>> > repeating the following pattern:
>> 
>> The assumption here is that set_nlink() is called with a non-zero count.
>> The point of introducing set_nlink() was to verify this assumption,
>> otherwise pending delete accounting will be screwed up.
>> 
>> If setting i_nlink to zero is not a bug in xfs we can do two things:
>> 
>>  1) do a conditional clear_nlink() in xfs_setup_inode() to document that
>> nlink can indeed be zero
>> 
>>  2) remove the warning from set_nlink().  That may hide some cases where
>> i_nlink was cleared without the intention of the filesystem but it will
>> work just fine wrt. the pending delete accounting. 
>
> I suspect 2 might be the better option.  The pattern we see here in
> XFS is pretty typical for transactional filesytems - when we mount a
> filesystem after an unclean shutdown we need to drop all inodes that
> were open but unlinked when the system crashed, and that typically
> means reading inodes from disk that have a zero i_nlink.  Maybe some
> filesystems never instanciate a VFS inode for it, but I suspect many
> do.
>
> For XFS we will actually see it during regularly testing as we have
> an ioctl that simulates a shutdown and can thus trigger log recovery
> easily, while for others like ext4 you'd actually have to do a real
> reset of your (physical or virtual) machine.

Okay, so a WARN_ON is not warranted.  Perhaps a printk instead?

While initializing nlink with zero normally shouldn't be a problem it
might indicate something unexpected.

Thanks,
Miklos

---
 fs/inode.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c	2011-10-27 22:26:12.000000000 +0200
+++ linux-2.6/fs/inode.c	2011-10-28 10:14:18.000000000 +0200
@@ -320,7 +320,8 @@ EXPORT_SYMBOL(clear_nlink);
  */
 void set_nlink(struct inode *inode, unsigned int nlink)
 {
-	if (WARN_ON(!nlink)) {
+	if (!nlink) {
+		printk(KERN_INFO "set_nlink() clearing i_nlink on %s inode %li\n", inode->i_sb->s_type->name, inode->i_ino);
 		clear_nlink(inode);
 	} else {
 		/* Yes, some filesystems do change nlink from zero to one */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/