Re: [Devel] [PATCH hci-8.1 v9 09/10] dm-qcow2: flag the image for check after a reverted L1/L2 writeback

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

>> @@ -1362,6 +1362,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   		if (cleared)
>>   			continue; /* no cluster was allocated */
>>   
>> +		wbd->md->status |= MD_LX_REVERTED;
>>   		spin_unlock(&qcow2->md_pages_lock);
>>   		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>>   
> [Severity: Medium]
> Can the lock be left unheld on the WARN path in this loop?
>
> The loop body in revert_l_entries_update() continues like this after the
> quoted lines:
>
> 	ret = qcow2_handle_r1r2_maps(qcow2, pos, NULL, &r1, &r2, false);
> 	if (WARN_ON_ONCE(ret <= 0))
> 		continue;
>
> 	mark_cluster_unused(qcow2, r2.md, r2.index_in_page, pos);
> 	spin_lock(&qcow2->md_pages_lock);
>
> The lock is dropped just above, but the continue skips the closing
> spin_lock(). The next iteration then reads wbd->md->page, calls
> set_u64_to_be_page(), and executes the new store to wbd->md->status
> without md_pages_lock. It may also reach spin_unlock() a second time on a
> lock that is not held.
>
> The caller does not expect this:
>
> complete_wbd() {
> 	spin_lock_irqsave(&qcow2->md_pages_lock, flags);
> 	revert_l_entries_update(qcow2, wbd);
> 	clear_writeback_status(qcow2, wbd->md, wbd->ret,
> 			       &wait_list, &end_list);
> 	spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
> 	...
> }
>
> If the lock is not re-acquired, clear_writeback_status() and
> spin_unlock_irqrestore() run on a lock that is no longer owned. That can
> corrupt the spinlock and IRQ state, and it races with other users of
> md_pages_lock.
>
> qcow2_handle_r1r2_maps() can return a value <= 0 in several cases. It
> returns -ENOENT when the R1 entry is zero, and -EIO on an invalid R1 entry
> or when the refcount sanity check fails (entry > 1 with no snapshots). It
> returns 0 when an R1/R2 page is not cached.
>
> The comment says R1/R2 "should be cached", but I could not show that none
> of these returns can happen. For example, a failed md read or a failed
> relocate_refcount_table() can erase a cached page.
>
> The unlock/relock imbalance is not new, but this patch adds an
> unlocked-path store to md->status in the same loop. The later patch in the
> series, "dm-qcow2: truncate unused tail of image on suspend", does not
> change this handling.
>
> Could the WARN path re-take md_pages_lock before continuing, or could the
> loop be restructured so the lock state is the same on every path?
The lock imbalance on the WARN path has been there since the driver was
introduced, and this patch doesn't make it worse: the new MD_LX_REVERTED
store sits before spin_unlock(), so like the existing 
set_u64_to_be_page() it
only runs unlocked after that path has already been taken, which is why the
fix belongs in a separate patch.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel