linux-ext4 - Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference

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:	Sun, 21 Nov 2010 22:16:19 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Milton Miller <miltonm@....com>
Cc:	Minchan Kim <minchan.kim@...il.com>, linux-kernel@...r.kernel.org,
	linux-mm@...r.kernel.org, linux-ext4@...r.kernel.org,
	"Ted Ts'o" <tytso@....edu>, Arun Bhanu <ab@...nbhanu.com>
Subject: Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

On Sun, Nov 21, 2010 at 09:31:14PM -0600, Milton Miller wrote:
> On 2010-11-22 at around 0:38:49, Minchan Kim wrote:
> > On Mon, Nov 22, 2010 at 2:37 AM, Ted Ts'o <tytso@....edu> wrote:
> > > On Mon, Nov 22, 2010 at 12:39:49AM +0900, Minchan Kim wrote:
> > > >
> > > > I think it's no problem.
> > > >
> > > > That's because migration always holds lock_page on the file page.
> > > > So the page couldn't remove from radix.
> > >
> > > It may be "ok" in that it won't cause a race, but it still leaves an
> > > unsightly warning if LOCKDEP is enabled, and LOCKDEP warnings will
> > > cause /proc_lock_stat to be disabled.  So I think it still needs to be
> > > fixed by adding rcu_read_lock()/rcu_read_unlock() to
> > > migrate_page_move_mapping().
> > >
> > >                                                     - Ted
> > >
> > 
> > Yes. if it is really "ok" about race, we will add rcu_read_lock with
> > below comment to prevent false positive.
> > "suppress RCU lockdep false positives".
> > But I am not sure it's good although rcu_read_lock is little cost.
> > Whenever we find false positive, should we add rcu_read_lock to
> > suppress although it's no problem in real product?
> > Couldn't we provide following function? (or we might have already it
> > but I missed it. )
> > 
> > /*
> >  * Suppress RCU lockdep false positive.
> >  */
> > #ifdef CONFIG_PROVE_RCU
> > #define rcu_read_lock_suppress rcu_read_lock
> > #else
> > #define rcu_read_lock_suppress
> > #endif
> 
> No, you don't need anything like this, as rcu_dereference_check already
> takes a test for alternate locking.
> 
> However, looking more closely at the code, it appears this is the
> "the tree is write locked" case as described in radix-tree.h
> 
> Looking at rcupdate.h, perhaps we need a version of radix_tree_deref_slot
> that uses rcu_dereference_protected?
> 
> Copying Paul McKenney for rcu ...

This approach could work.  One way of doing it would be to add a second
argument:

static inline void *radix_tree_deref_slot_check(void **pslot, int ldc)
{
	void *ret = rcu_dereference_check(*pslot, ldc);
	if (unlikely(radix_tree_is_indirect_ptr(ret)))
		ret = RADIX_TREE_RETRY;
	return ret;
}

static inline void *radix_tree_deref_slot(void **pslot)
{
	return radix_tree_deref_slot_check(pslot, rcu_read_lock_held());
}

Another alternative would have radix_tree_deref_slot() pass "1" into
the "ldc" argument, which reduces splats but at the expense of failing
to detect problems.  ;-)

							Thanx, Paul
--
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