linux-ext4 - Re: [patch] fix up lock order reversal in writeback

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:	Wed, 17 Nov 2010 14:56:52 +1100
From:	Nick Piggin <npiggin@...nel.dk>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Nick Piggin <npiggin@...nel.dk>, linux-fsdevel@...r.kernel.org,
	linux-ext4@...r.kernel.org, linux-btrfs@...r.kernel.org
Subject: Re: [patch] fix up lock order reversal in writeback

On Tue, Nov 16, 2010 at 12:32:52PM -0800, Andrew Morton wrote:
> On Tue, 16 Nov 2010 22:00:58 +1100
> Nick Piggin <npiggin@...nel.dk> wrote:
> 
> > I saw a lock order warning on ext4 trigger. This should solve it.
> 
> Send us the trace, please.

I lost it, sorry.

 
> The code comment implies that someone is calling down_read() under
> i_lock?  That would be bad, and I'd expect it to have produced a
> might_sleep() warning, not a lockdep trace.  

Sorry not i_lock, i_mutex. writeback_inodes_sb_if_idle is called by
ext4's write_begin function which is called with i_mutex held from
generic_file_buffered_write, I believe is the trace.

 
> And I don't see how we can call writeback_inodes_sb() under i_lock
> anyway, so I don't really have a clue what's going on here!
> 
> > Raciness shouldn't matter much, because writeback can stop just
> > after we make the test and return anyway (so the API is racy anyway).

> > 
> > Signed-off-by: Nick Piggin <npiggin@...nel.dk>
> > 
> > Index: linux-2.6/fs/fs-writeback.c
> > ===================================================================
> > --- linux-2.6.orig/fs/fs-writeback.c	2010-11-16 21:44:32.000000000 +1100
> > +++ linux-2.6/fs/fs-writeback.c	2010-11-16 21:49:37.000000000 +1100
> > @@ -1125,16 +1125,20 @@ EXPORT_SYMBOL(writeback_inodes_sb);
> >   *
> >   * Invoke writeback_inodes_sb if no writeback is currently underway.
> >   * Returns 1 if writeback was started, 0 if not.
> > + *
> > + * May be called inside i_lock. May not start writeback if locks cannot
> > + * be acquired.
> >   */
> >  int writeback_inodes_sb_if_idle(struct super_block *sb)
> >  {
> >  	if (!writeback_in_progress(sb->s_bdi)) {
> > -		down_read(&sb->s_umount);
> > -		writeback_inodes_sb(sb);
> > -		up_read(&sb->s_umount);
> > -		return 1;
> > -	} else
> > -		return 0;
> > +		if (down_read_trylock(&sb->s_umount)) {
> > +			writeback_inodes_sb(sb);
> > +			up_read(&sb->s_umount);
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> 
> And it's pretty generous to describe a s/down_read/down_read_trylock/
> as a "fix".  Terms like "bandaid" and "workaround" come to mind.

As much as the writeback_inodes_sb_if_idle API itself is a bandaid,
I suppose. (it doesn't do any rate limiting of the dirtier, it's racy,
it doesn't specify how much to writeback, it's synchronous, etc).

Anyway, I don't know, there's not much other option for 2.6.37 AFAIKS.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html