Re: [PATCH 3/3] qcow2: Fix corruption bug in qcow2_detect_metadata_preallocation()

[Kevin Wolf <kwolf@redhat.com>]

Am 24.10.2019 um 12:46 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 23.10.2019 18:26, Kevin Wolf wrote:
> > qcow2_detect_metadata_preallocation() calls qcow2_get_refcount() which
> > requires s->lock to be taken to protect its accesses to the refcount
> > table and refcount blocks. However, nothing in this code path actually
> > took the lock. This could cause the same cache entry to be used by two
> > requests at the same time, for different tables at different offsets,
> > resulting in image corruption.
> > 
> > As it would be preferable to base the detection on consistent data (even
> > though it's just heuristics), let's take the lock not only around the
> > qcow2_get_refcount() calls, but around the whole function.
> > 
> > This patch takes the lock in qcow2_co_block_status() earlier and asserts
> > in qcow2_detect_metadata_preallocation() that we hold the lock.
> > 
> > Fixes: 69f47505ee66afaa513305de0c1895a224e52c45
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Michael Weiser <michael@weiser.dinsnail.net>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Oh, I'm very sorry. I was going to backport this patch, and found that it's
> fixed in our downstream long ago, even before last upstream version patch sent :(

Seriously? Is your downstream QEMU so divergent from upstream that you
wouldn't notice something like this? I think you really have to improve
something there.

I mean, we have a serious data corruptor in the 4.1 release and I wasted
days to debug this, and you've been sitting on a fix for months without
telling anyone? This isn't a memory leak or something, this kills
people's images. It's eating their data.

Modifying an image format driver requires utmost care and I think I'll
try to make sure to allow only few qcow2 changes per release in the
future. Too many changes were made in too short time, and with too
little care, and there are even more qcow2 patches pending. Please check
whether these series actually match your downstream code. Anyway, we'll
tread very carefully now with the pending patches, even if it means
delaying them for another release or two. Stability is way more
important for an image format driver than new features and
optimisations.

Do whatever you need to fix your downstream process, but seriously, this
must not ever happen again.

Kevin