linux-kernel - Re: [PATCH block/for-linus] writeback: fix a subtle race condition in I

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]

Message-ID: <20141104173443.GF14459@htj.dyndns.org>
Date:	Tue, 4 Nov 2014 12:34:43 -0500
From:	Tejun Heo <tj@...nel.org>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	Jan Kara <jack@...e.cz>, Mikulas Patocka <mpatocka@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH block/for-linus] writeback: fix a subtle race condition
 in I_DIRTY clearing

On Fri, Oct 24, 2014 at 03:38:21PM -0400, Tejun Heo wrote:
> After invoking ->dirty_inode(), __mark_inode_dirty() does smp_mb() and
> tests inode->i_state locklessly to see whether it already has all the
> necessary I_DIRTY bits set.  The comment above the barrier doesn't
> contain any useful information - memory barriers can't ensure "changes
> are seen by all cpus" by itself.
> 
> And it sure enough was broken.  Please consider the following
> scenario.
> 
>  CPU 0					CPU 1
>  -------------------------------------------------------------------------------
> 
> 					enters __writeback_single_inode()
> 					grabs inode->i_lock
> 					tests PAGECACHE_TAG_DIRTY which is clear
>  enters __set_page_dirty()
>  grabs mapping->tree_lock
>  sets PAGECACHE_TAG_DIRTY
>  releases mapping->tree_lock
>  leaves __set_page_dirty()
> 
>  enters __mark_inode_dirty()
>  smp_mb()
>  sees I_DIRTY_PAGES set
>  leaves __mark_inode_dirty()
> 					clears I_DIRTY_PAGES
> 					releases inode->i_lock
> 
> Now @inode has dirty pages w/ I_DIRTY_PAGES clear.  This doesn't seem
> to lead to an immediately critical problem because requeue_inode()
> later checks PAGECACHE_TAG_DIRTY instead of I_DIRTY_PAGES when
> deciding whether the inode needs to be requeued for IO and there are
> enough unintentional memory barriers inbetween, so while the inode
> ends up with inconsistent I_DIRTY_PAGES flag, it doesn't fall off the
> IO list.
> 
> The lack of explicit barrier may also theoretically affect the other
> I_DIRTY bits which deal with metadata dirtiness.  There is no
> guarantee that a strong enough barrier exists between
> I_DIRTY_[DATA]SYNC clearing and write_inode() writing out the dirtied
> inode.  Filesystem inode writeout path likely has enough stuff which
> can behave as full barrier but it's theoretically possible that the
> writeout may not see all the updates from ->dirty_inode().
> 
> Fix it by adding an explicit smp_mb() after I_DIRTY clearing.  Note
> that I_DIRTY_PAGES needs a special treatment as it always needs to be
> cleared to be interlocked with the lockless test on
> __mark_inode_dirty() side.  It's cleared unconditionally and
> reinstated after smp_mb() if the mapping still has dirty pages.
> 
> Also add comments explaining how and why the barriers are paired.
> 
> Lightly tested.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>
> Cc: Jan Kara <jack@...e.cz>
> Cc: Mikulas Patocka <mpatocka@...hat.com>
> Cc: Jens Axboe <axboe@...nel.dk>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: stable@...r.kernel.org

Jens, can you please route this one?

Thanks.

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