Re: [BUG] bugs in jbd2_dev_to_name() (was Re: [PATCH 00/11] [GIT PULL] more updates for the tag format)

[Theodore Tso <tytso@mit.edu>]

On Fri, Jun 19, 2009 at 04:14:23PM +0800, Li Zefan wrote:
> > 	rcu_read_lock();
> > 	if (devcache[i] && devcache[i]->device == device) {
> > 		ret = devcache[i]->devname;
> > 		rcu_read_unlock();
> > 		return ret;
> 
> It doesn't seem safe to dereference @ret outside rcu read section.

Note the comments at the beginning of the function:

    The caller should use rcu_read_lock() in order to make sure the
    device name stays valid until its done with it.  We use
    rcu_read_lock() as well to make sure we're safe in case the caller
    gets sloppy, and because rcu_read_lock() is cheap and can be safely
    nested.

I suppose I should change the wording to indicate that it adds a bit
more safety (as in, the crash won't happen inside this function, but
as far as the caller is concerned, all bets are off!)

> > 	spin_lock(&devname_cache_lock);
> > 	if (devcache[i]) {
> > 		if (devcache[i]->device == device) {
> > 			ret = devcache[i]->devname;
> > 			spin_unlock(&devname_cache_lock);
> > 			return ret;
> > 		}
> > 		call_rcu(&devcache[i]->rcu, free_devcache);
> > 	}
> > 	devcache[i] = kmalloc(sizeof(struct devname_cache), GFP_KERNEL);
> 
> kmalloc(GFP_KERNEL) called with spin_lock held..

Good catch, thanks.  I'll get a patch in to fix this.

							- Ted