block: avoid use-after-free on throttle data

Hello Ming Lei,

This is a semi-automatic email about new static checker warnings.

The patch ee37eddbfa9e: "block: avoid use-after-free on throttle data" from Mar 18, 2022, leads to the following Smatch complaint:

block/blk-throttle.c:1189 throtl_pending_timer_fn()
error: we previously assumed 'tg' could be null (see line 1147)

block/blk-throttle.c
1146 /* throtl_data may be gone, so figure out request queue by blkg */
1147 if (tg)

                    ^^

The patch adds a new check

1148 q = tg->pd.blkg->q;
1149 else
1150 q = td->queue;
1151
1152 spin_lock_irq(&q->queue_lock);
1153
1154 if (!q->root_blkg)
1155 goto out_unlock;
1156
1157 if (throtl_can_upgrade(td, NULL))
1158 throtl_upgrade_state(td);
1159
1160 again:
1161 parent_sq = sq->parent_sq;
1162 dispatched = false;
1163
1164 while (true) {
1165 throtl_log(sq, "dispatch nr_queued=%u read=%u write=%u",
1166 sq->nr_queued[READ] + sq->nr_queued[WRITE],
1167 sq->nr_queued[READ], sq->nr_queued[WRITE]);
1168
1169 ret = throtl_select_dispatch(sq);
1170 if (ret) {
1171 throtl_log(sq, "bios disp=%u", ret);
1172 dispatched = true;
1173 }
1174
1175 if (throtl_schedule_next_dispatch(sq, false))
1176 break;
1177
1178 /* this dispatch windows is still open, relax and repeat */
1179 spin_unlock_irq(&q->queue_lock);
1180 cpu_relax();
1181 spin_lock_irq(&q->queue_lock);
1182 }
1183
1184 if (!dispatched)
1185 goto out_unlock;
1186
1187 if (parent_sq) {
1188 /* @parent_sq is another throl_grp, propagate dispatch */

  1189			if (tg->flags & THROTL_TG_WAS_EMPTY) {
                            ^^^^^^^^^

But the old code dereferences "tg" without checking.

  1190				tg_update_disptime(tg);
  1191				if (!throtl_schedule_next_dispatch(parent_sq, false)) {

regards,
dan carpenter

Hello Dan,

Here if 'parent_sq' isn't NULL, tg won't be NULL, see sq_to_tg()

Thanks,
Ming

Thanks. It would have taken me a while to find sq_to_tg(). Smatch is supposed to figure out this stuff but somehow it's not working.

Smatch knows that if "tg" is non-NULL then parent_sq is non-NULL. And it knows that if sq->parent_sq is NULL then tg is NULL. But somehow it can't figure out that if sq->parent_sq is non-NULL then tg is non-NULL...

:/

Something to investigate but I ran out of time today.

regards,
dan carpenter