bcache: explicitly make cache_set only have single cache

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

Quoted 51 lines · Dan Carpenter

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)

Quoted 6 lines

                    ^^^^^^^^^^^^^^^^

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.