On Thu, Oct 10, 2013 at 02:26:25PM +0200, Kevin Wolf wrote:
> Am 10.10.2013 um 10:52 hat Max Reitz geschrieben:
> > If the write request spans more than one L2 table,
> > qcow2_alloc_cluster_offset cannot handle the required allocations
> > atomically. This results in leaks if it allocated new clusters in any
> > but the last L2 table touched and an error occurs in qcow2_co_writev
> > before having established the L2 link. These non-atomic allocations
> > were, however, indeed successful and are therefore given to the caller
> > in the L2Meta list.
> >
> > If an error occurs in qcow2_co_writev and the L2Meta list is unwound,
> > all its remaining entries are clusters whose L2 links were not yet
> > established. Thus, all allocations in that list should be undone.
> >
> > Signed-off-by: Max Reitz <mre...@redhat.com>
> > ---
> > block/qcow2.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index b2489fb..6bedd5d 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1017,6 +1017,13 @@ fail:
> > while (l2meta != NULL) {
> > QCowL2Meta *next;
> >
> > + /* Undo all leaked allocations */
> > + if (l2meta->nb_clusters != 0) {
> > + qcow2_free_clusters(bs, l2meta->alloc_offset,
> > + l2meta->nb_clusters << s->cluster_bits,
> > + QCOW2_DISCARD_ALWAYS);
> > + }
> > +
> > if (l2meta->nb_clusters != 0) {
> > QLIST_REMOVE(l2meta, next_in_flight);
> > }
>
> This feels a bit risky.
>
> I think currently it does work, because qcow2_alloc_cluster_link_l2()
> can only return an error when it didn't update the L2 entry in the cache
> yet, but adding any error condition between that point and the L2Meta
> unwinding would result in corruption. I'm unsure, but perhaps a cluster
> leak is the lesser evil. Did you consider this? Do other people have an
> opinion on it?
I agree it's easy to make things worse by trying to free clusters in an
error path. I find these types of changes a lot of effort (to write and
review properly) for relatively little gain.
Stefan
