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.
Thanks.
Coly Li
