qcow2 api not secured by mutex lock

qcow2 api not secured by mutex lock

[Vladimir Sementsov-Ogievskiy]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

Hi!

Some time ago, we've faced and fixed the fact that qcow2 bitmap api doesn't
call qcow2_co_mutex_lock, before accessing qcow2 metadata. This was solved by
moving qcow2_co_remove_persistent_dirty_bitmap and
qcow2_co_can_store_new_dirty_bitmap to coroutine and call qcow2_co_mutex_lock.

Now I decided to look at big picture (it is attached).

Boxes are qcow2 driver api, green border means that function calls qcow2_co_mutex_lock
(it doesn't guarantee, that exactly child node call is locked, but it is something).

In the picture there are just all functions, calling qcow2_cache_get/put.. Not all the
functions, that needs locking, but again, it is something.

So, accordingly to the picture, it seems that the following functions lacks locking:

qcow2_co_create

qcow2_snapshot_*
   (but it is both drained and aio context locked, so should be safe, yes?)

qcow2_reopen_bitmaps_rw
qcow2_store_persistent_dirty_bitmaps

qcow2_amend_options

qcow2_make_empty

===

Checking green nodes:

qcow2_co_invalidate_cache actually calls qcow2_close unlocked, it's another reason to fix qcow2_store_persistent_dirty_bitmaps

qcow2_write_snapshots actually called unlocked from qcow2_check_fix_snapshot_table.. It seems unsafe.

===


Not complete audit of course.. What do you think about it? I think, I at least should move
qcow2_store_persistent_dirty_bitmaps and qcow2_reopen_bitmaps_rw to coroutine,
like qcow2_co_remove_persistent_dirty_bitmap.


-- 
Best regards,
Vladimir

[-- Attachment #2: master-block-qcow2-filtered.svg --]
[-- Type: image/svg+xml, Size: 122306 bytes --]

Re: qcow2 api not secured by mutex lock

[Kevin Wolf]

Am 18.12.2019 um 11:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi!
> 
> Some time ago, we've faced and fixed the fact that qcow2 bitmap api doesn't
> call qcow2_co_mutex_lock, before accessing qcow2 metadata. This was solved by
> moving qcow2_co_remove_persistent_dirty_bitmap and
> qcow2_co_can_store_new_dirty_bitmap to coroutine and call qcow2_co_mutex_lock.
> 
> Now I decided to look at big picture (it is attached).
> 
> Boxes are qcow2 driver api, green border means that function calls qcow2_co_mutex_lock
> (it doesn't guarantee, that exactly child node call is locked, but it is something).
> 
> In the picture there are just all functions, calling qcow2_cache_get/put.. Not all the
> functions, that needs locking, but again, it is something.
> 
> So, accordingly to the picture, it seems that the following functions lacks locking:
> 
> qcow2_co_create

This should be easy to fix. It's also relatively harmless because it's
unlikely that the image that is being created is accessed by someone
else (the user would have to query the auto-generated node name and
start something on it - at which point they deserve what they get).

> qcow2_snapshot_*
>    (but it is both drained and aio context locked, so should be safe, yes?)

If you checked that these conditions are true, it should be safe.

> qcow2_reopen_bitmaps_rw
> qcow2_store_persistent_dirty_bitmaps

Reopen drains the image, so I think this is safe in practice.

If we want to do something about it anyway (e.g. move it to a coroutine
so it can take a lock) the question is where to do that. Maybe even for
.bdrv_reopen_* in general?

> qcow2_amend_options

Only qemu-img so far, so no concurrency. We're about to add
blockdev-amend in QMP, though, so this looks like something that should
take the lock.

In fact, is taking the lock enough or should it actually drain the node,
too?

> qcow2_make_empty

This one should certainly drain. It is used not only in qemu-img, but
also in HMP commit and apparently also in replication.

This one might be a bug that could become visible in practice. Unlikely
for HMP commit (because it takes a while and is holding the BQL, so no
new guest requests will be processed), except maybe for cases where
there is nothing to commit.

> ===
> 
> Checking green nodes:
> 
> qcow2_co_invalidate_cache actually calls qcow2_close unlocked, it's
> another reason to fix qcow2_store_persistent_dirty_bitmaps

Might be. Do we want a .bdrv_co_close?

> qcow2_write_snapshots actually called unlocked from
> qcow2_check_fix_snapshot_table.. It seems unsafe.

This is curious, I'm not sure why you would drop the lock there. Max?

bdrv_flush() calls would have to replaced with qcow2_write_caches() to
avoid a deadlock, but otherwise I don't see why we would want to drop
the lock.

Of course, this should only be called from qemu-img check, so in
practice it's probably not a bug.

Kevin

Re: qcow2 api not secured by mutex lock

[Vladimir Sementsov-Ogievskiy]

19.12.2019 13:02, Kevin Wolf wrote:
> Am 18.12.2019 um 11:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi!
>>
>> Some time ago, we've faced and fixed the fact that qcow2 bitmap api doesn't
>> call qcow2_co_mutex_lock, before accessing qcow2 metadata. This was solved by
>> moving qcow2_co_remove_persistent_dirty_bitmap and
>> qcow2_co_can_store_new_dirty_bitmap to coroutine and call qcow2_co_mutex_lock.
>>
>> Now I decided to look at big picture (it is attached).
>>
>> Boxes are qcow2 driver api, green border means that function calls qcow2_co_mutex_lock
>> (it doesn't guarantee, that exactly child node call is locked, but it is something).
>>
>> In the picture there are just all functions, calling qcow2_cache_get/put.. Not all the
>> functions, that needs locking, but again, it is something.
>>
>> So, accordingly to the picture, it seems that the following functions lacks locking:
>>
>> qcow2_co_create
> 
> This should be easy to fix. It's also relatively harmless because it's
> unlikely that the image that is being created is accessed by someone
> else (the user would have to query the auto-generated node name and
> start something on it - at which point they deserve what they get).
> 
>> qcow2_snapshot_*
>>     (but it is both drained and aio context locked, so should be safe, yes?)
> 
> If you checked that these conditions are true, it should be safe.
> 
>> qcow2_reopen_bitmaps_rw
>> qcow2_store_persistent_dirty_bitmaps
> 
> Reopen drains the image, so I think this is safe in practice.
> 
> If we want to do something about it anyway (e.g. move it to a coroutine
> so it can take a lock) the question is where to do that. Maybe even for
> .bdrv_reopen_* in general?
> 
>> qcow2_amend_options
> 
> Only qemu-img so far, so no concurrency. We're about to add
> blockdev-amend in QMP, though, so this looks like something that should
> take the lock.
> 
> In fact, is taking the lock enough or should it actually drain the node,
> too?
> 
>> qcow2_make_empty
> 
> This one should certainly drain. It is used not only in qemu-img, but
> also in HMP commit and apparently also in replication.
> 
> This one might be a bug that could become visible in practice. Unlikely
> for HMP commit (because it takes a while and is holding the BQL, so no
> new guest requests will be processed), except maybe for cases where
> there is nothing to commit.
> 
>> ===
>>
>> Checking green nodes:
>>
>> qcow2_co_invalidate_cache actually calls qcow2_close unlocked, it's
>> another reason to fix qcow2_store_persistent_dirty_bitmaps
> 
> Might be. Do we want a .bdrv_co_close?
> 
>> qcow2_write_snapshots actually called unlocked from
>> qcow2_check_fix_snapshot_table.. It seems unsafe.
> 
> This is curious, I'm not sure why you would drop the lock there. Max?
> 
> bdrv_flush() calls would have to replaced with qcow2_write_caches() to
> avoid a deadlock, but otherwise I don't see why we would want to drop
> the lock.
> 
> Of course, this should only be called from qemu-img check, so in
> practice it's probably not a bug.
> 

Thanks for analysis! I'll continue thinking on this and come with patches
(or new questions).


-- 
Best regards,
Vladimir

Re: qcow2 api not secured by mutex lock

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1495 bytes --]

On 19.12.19 11:02, Kevin Wolf wrote:
> Am 18.12.2019 um 11:28 hat Vladimir Sementsov-Ogievskiy geschrieben:

[...]

>> qcow2_write_snapshots actually called unlocked from
>> qcow2_check_fix_snapshot_table.. It seems unsafe.
> 
> This is curious, I'm not sure why you would drop the lock there. Max?

I don’t remember why but it may certainly have to do with the fact that
everything that calls qcow2_write_snapshots() (i.e., qcow2_snapshot_*)
does so without having taken the lock.  I suppose I simply assumed this
would have to be how it’s done.

I don’t think it’s a problem right now because you can only check (and
repair) the image from qemu-img (or when it is opened with the dirty
flag set), so there shouldn’t be concurrent I/O.

Anyway.  I tried to remove it and then 261 hangs.  This is because
qcow2_write_snapshots() calls bdrv_flush(bs) twice.  It would have to
drop the lock around those calls at least.  I’m actually not sure
whether this is safe to do (in the sense of whether it’s fundamentally
safer than just not holding the lock at all and trusting that there are
no concurrent requests).

In any case, it’s also not purely trivial, because if we were to make
qcow2_write_snapshots() drop the locks around bdrv_flush(), all of its
callers would in turn need to take the lock around it.  (I’m not saying
that is difficult, I’m just saying it’s more difficult than dropping
three lines in qcow2_write_snapshots()).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: qcow2 api not secured by mutex lock

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 889 bytes --]

On 19.12.19 11:02, Kevin Wolf wrote:
> Am 18.12.2019 um 11:28 hat Vladimir Sementsov-Ogievskiy geschrieben:

[...]

>> qcow2_write_snapshots actually called unlocked from
>> qcow2_check_fix_snapshot_table.. It seems unsafe.
> 
> This is curious, I'm not sure why you would drop the lock there. Max?
> 
> bdrv_flush() calls would have to replaced with qcow2_write_caches() to
> avoid a deadlock, but otherwise I don't see why we would want to drop
> the lock.
> 
> Of course, this should only be called from qemu-img check, so in
> practice it's probably not a bug.

Maybe I should have read all of this before replying...  Is
qcow2_write_caches() all that we want?  I mean, bdrv_flush() also
flushes the children (well, at least the file child right now).

OTOH qcow2_write_snapshots() probably does not need to take care of
actually flushing @bs, does it?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: qcow2 api not secured by mutex lock

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

Am 19.12.2019 um 11:35 hat Max Reitz geschrieben:
> On 19.12.19 11:02, Kevin Wolf wrote:
> > Am 18.12.2019 um 11:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 
> [...]
> 
> >> qcow2_write_snapshots actually called unlocked from
> >> qcow2_check_fix_snapshot_table.. It seems unsafe.
> > 
> > This is curious, I'm not sure why you would drop the lock there. Max?
> > 
> > bdrv_flush() calls would have to replaced with qcow2_write_caches() to
> > avoid a deadlock, but otherwise I don't see why we would want to drop
> > the lock.
> > 
> > Of course, this should only be called from qemu-img check, so in
> > practice it's probably not a bug.
> 
> Maybe I should have read all of this before replying...  Is
> qcow2_write_caches() all that we want?  I mean, bdrv_flush() also
> flushes the children (well, at least the file child right now).

You're probably right and we want to call qcow2_cache_flush() instead. I
forgot that we split these functions.

> OTOH qcow2_write_snapshots() probably does not need to take care of
> actually flushing @bs, does it?

I think it wants to get the right ordering to avoid corruption on
crashes, so we certainly do want to flush all the way down to the disk.

At least after qcow2_alloc_clusters(), it needs to flush bs itself; for
the second one, it could be enough to flush bs->file, but flushing bs
can't hurt.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]