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
