* [bug report] bcache: explicitly make cache_set only have single cache
@ 2020-11-02 13:02 Dan Carpenter
2020-11-02 14:31 ` Coly Li
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-11-02 13:02 UTC (permalink / raw)
To: colyli; +Cc: linux-bcache
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
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] bcache: explicitly make cache_set only have single cache
2020-11-02 13:02 [bug report] bcache: explicitly make cache_set only have single cache Dan Carpenter
@ 2020-11-02 14:31 ` Coly Li
2020-11-02 14:53 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Coly Li @ 2020-11-02 14:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-bcache
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] bcache: explicitly make cache_set only have single cache
2020-11-02 14:31 ` Coly Li
@ 2020-11-02 14:53 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-11-02 14:53 UTC (permalink / raw)
To: Coly Li; +Cc: linux-bcache
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-02 14:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-02 13:02 [bug report] bcache: explicitly make cache_set only have single cache Dan Carpenter
2020-11-02 14:31 ` Coly Li
2020-11-02 14:53 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox