On Thu, Sep 14, 2023 at 04:38:11PM +0300, Dan Carpenter wrote:
> Hello Kent Overstreet,
>
> The patch 502bed819cc3: "bcachefs: Kill bch2_fs_usage_scratch_get()"
> from Apr 3, 2021 (linux-next), leads to the following Smatch static
> checker warning:
>
> fs/bcachefs/buckets.c:1352 bch2_trans_fs_usage_apply()
> error: we previously assumed 'trans->disk_res' could be null (see line
> 1302)
>
> fs/bcachefs/buckets.c
> 1296 int bch2_trans_fs_usage_apply(struct btree_trans *trans,
> 1297 struct replicas_delta_list *deltas)
> 1298 {
> 1299 struct bch_fs *c = trans->c;
> 1300 static int warned_disk_usage = 0;
> 1301 bool warn = false;
> 1302 unsigned disk_res_sectors = trans->disk_res ?
> trans->disk_res->sectors : 0;
> ^^^^^^^^^^^^^^^
> This code is quite subtle, but the static checker says that we check
> for NULL here.
>
> 1303 struct replicas_delta *d = deltas->d, *d2;
> 1304 struct replicas_delta *top = (void *) deltas->d +
> deltas->used;
> 1305 struct bch_fs_usage *dst;
> 1306 s64 added = 0, should_not_have_added;
> 1307 unsigned i;
> 1308
> 1309 percpu_down_read(&c->mark_lock);
> 1310 preempt_disable();
> 1311 dst = fs_usage_ptr(c, trans->journal_res.seq, false);
> 1312
> 1313 for (d = deltas->d; d != top; d = replicas_delta_next(d)) {
> 1314 switch (d->r.data_type) {
> 1315 case BCH_DATA_btree:
> 1316 case BCH_DATA_user:
> 1317 case BCH_DATA_parity:
> 1318 added += d->delta;
>
> And we set added to non-zero here so that doesn't seem related. Or the
> relationship is not obvious.
>
> 1319 }
> 1320
> 1321 if (__update_replicas(c, dst, &d->r, d->delta))
> 1322 goto need_mark;
> 1323 }
> 1324
> 1325 dst->nr_inodes += deltas->nr_inodes;
> 1326
> 1327 for (i = 0; i < BCH_REPLICAS_MAX; i++) {
> 1328 added +=
> deltas->persistent_reserved[i];
> 1329 dst->reserved +=
> deltas->persistent_reserved[i];
> 1330 dst->persistent_reserved[i] +=
> deltas->persistent_reserved[i];
> 1331 }
> 1332
> 1333 /*
> 1334 * Not allowed to reduce sectors_available except by getting
> a
> 1335 * reservation:
> 1336 */
> 1337 should_not_have_added = added - (s64) disk_res_sectors;
> 1338 if (unlikely(should_not_have_added > 0)) {
> 1339 u64 old, new, v =
> atomic64_read(&c->sectors_available);
> 1340
> 1341 do {
> 1342 old = v;
> 1343 new = max_t(s64, 0, old -
> should_not_have_added);
> 1344 } while ((v = atomic64_cmpxchg(&c->sectors_available,
> 1345 old, new)) != old);
> 1346
> 1347 added -= should_not_have_added;
> 1348 warn = true;
> 1349 }
> 1350
> 1351 if (added > 0) {
> --> 1352 trans->disk_res->sectors -= added;
> ^^^^^^^^^^^^^^^^^
> Unchecked dereference.
If disk_res was NULL, disk_res_sectors was 0, and added gets completely
zeroed out - not a bug.
