[bug report] block: avoid use-after-free on throttle data

[bug report] block: avoid use-after-free on throttle data

[Dan Carpenter]

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

Re: [bug report] block: avoid use-after-free on throttle data

[Ming Lei]

Hello Dan,

On Tue, Mar 22, 2022 at 09:55:04AM +0300, Dan Carpenter wrote:
> 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.

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


Thanks,
Ming

Re: [bug report] block: avoid use-after-free on throttle data

[Dan Carpenter]

On Tue, Mar 22, 2022 at 03:42:28PM +0800, Ming Lei wrote:
> >   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.
> 
> Here if 'parent_sq' isn't NULL, tg won't be NULL, see sq_to_tg()
> 

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