linux-kernel - Re: [PATCH] writeback: skip new or to-be-freed 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:	Mon, 8 Jun 2009 17:29:30 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	Artem Bityutskiy <dedekind1@...il.com>
Cc:	Jan Kara <jack@...e.cz>, Eric Sandeen <sandeen@...deen.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Masayoshi MIZUMA <m.mizuma@...fujitsu.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
	Nick Piggin <npiggin@...e.de>, Jeff Layton <jlayton@...hat.com>
Subject: Re: [PATCH] writeback: skip new or to-be-freed inodes

Hi Artem,

On Mon, Jun 08, 2009 at 03:03:10PM +0800, Artem Bityutskiy wrote:
> Wu Fengguang wrote:
> > The above race and warning didn't turn up because writeback_inodes() holds
> > the s_umount lock, so generic_forget_inode() finds MS_ACTIVE and returns
> > early. But we are not sure the UBIFS calls and future callers will guarantee
> > that. So skip I_WILL_FREE inodes for the sake of safety.
>
> The inode states are a bit vague for me, but vs. UBIFS - feel
> free to ask questions.

Thank you. Basically I'm not sure if UBIFS guarantees it won't be
unmounted (hence the MS_ACTIVE bit is on) when calling
generic_sync_sb_inodes() in shrink_liability() and ubifs_sync_fs().

Thanks,
Fengguang

PS: our previous discussions

        > > Another possibility:
        > >
        > > generic_forget_inode
        > >   inode->i_state |= I_WILL_FREE;
        > >   spin_unlock(&inode_lock);
        > >                                                 generic_sync_sb_inodes()
        > >                                                   spin_lock(&inode_lock);
        > >                                                   __iget(inode);
        > >                                                   __writeback_single_inode
        > >                                                     // see non zero i_count
        > >                                                     WARN_ON(inode->i_state & I_WILL_FREE);
        > >
        > > I'm wondering why didn't we saw reports on the last WARN_ON()?
        > > Did we missed something?
        >   I meant the above race in my description ;-). Anyway, the race can happen
        > only if we are unmounting the filesystem (normally, we bail out on
        > sb->s_flags & MS_ACTIVE check - yes, it's a bit hidden and it also took me
        > a while to understand why we weren't seeing tons of warnings...).

        Ah OK. Just checked that all three callers of generic_sync_sb_inodes():
        - writeback_inodes(): umount prevented
        - pohmelfs_kill_super(): just before umount
        - ubifs calls: too complex to be obvious..
        At least the first two cases are safe, so we didn't see the error report ;)

--
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/