Re: [bug report] bcache: explicitly make cache_set only have single cache

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Nov 02, 2020 at 10:31:27PM +0800, Coly Li wrote:
> On 2020/11/2 21:02, Dan Carpenter wrote:
> > Hello Coly Li,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch 697e23495c94: "bcache: explicitly make cache_set only have 
> > single cache" from Oct 1, 2020, leads to the following Smatch 
> > complaint:
> > 
> >     drivers/md/bcache/super.c:2157 register_cache_set()
> >     error: we previously assumed 'c->cache' could be null (see line 2125)
> > 
> > drivers/md/bcache/super.c
> >   2124			if (!memcmp(c->set_uuid, ca->sb.set_uuid, 16)) {
> >   2125				if (c->cache)
> >                                     ^^^^^^^^
> > 
> >   2126					return "duplicate cache set member";
> >   2127	
> >   2128				goto found;
> >                                 ^^^^^^^^^^
> > "c->cache" is NULL on this path.
> > 
> >   2129			}
> >   2130	
> >   2131		c = bch_cache_set_alloc(&ca->sb);
> >   2132		if (!c)
> >   2133			return err;
> >   2134	
> >   2135		err = "error creating kobject";
> >   2136		if (kobject_add(&c->kobj, bcache_kobj, "%pU", c->set_uuid) ||
> >   2137		    kobject_add(&c->internal, &c->kobj, "internal"))
> >   2138			goto err;
> >   2139	
> >   2140		if (bch_cache_accounting_add_kobjs(&c->accounting, &c->kobj))
> >   2141			goto err;
> >   2142	
> >   2143		bch_debug_init_cache_set(c);
> >   2144	
> >   2145		list_add(&c->list, &bch_cache_sets);
> >   2146	found:
> >   2147		sprintf(buf, "cache%i", ca->sb.nr_this_dev);
> >   2148		if (sysfs_create_link(&ca->kobj, &c->kobj, "set") ||
> >   2149		    sysfs_create_link(&c->kobj, &ca->kobj, buf))
> >   2150			goto err;
> >   2151	
> >   2152		kobject_get(&ca->kobj);
> >   2153		ca->set = c;
> >   2154		ca->set->cache = ca;
> >   2155	
> >   2156		err = "failed to run cache set";
> >   2157		if (run_cache_set(c) < 0)
> >                     ^^^^^^^^^^^^^^^^
> > c->cache gets dereferenced inside this function without checking when we
> > do "c->nbuckets = ca->sb.nbuckets;".
> > 
> >   2158			goto err;
> >   2159	
> 
> 
> Hi Dan,
> 
> Hmm, let me check. It seems the trick is at line 2153 and 2154,
> 
> 2153		ca->set = c;
> 2154		ca->set->cache = ca;
> 
> "ca->set->cache = ca" equals to "c->cache = ca", so c->cache is
> initialized and safe. Yes we can write line 2154 as "c->cache = ca", but
> my motivation was little, event for readability.

Argh....  Of course.  Sorry, for the noise.  I feel like this must be a
regression in Smatch which is why it didn't generate a warning earlier.
I'll look into it.

regards,
dan carpenter