[1/2] qcow2: Undo leaked allocations in co_writev

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 <mreitz@redhat.com>
---
 block/qcow2.c | 7 +++++++
 1 file changed, 7 insertions(+)

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 <mreitz@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?

Also, shouldn't it be QCOW2_DISCARD_OTHER?

Kevin

On 2013-10-10 14:26, 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 <mreitz@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?

What error conditions are there which can occur between 
qcow2_alloc_cluster_link_l2 and the L2Meta unwinding? If all 
qcow2_alloc_cluster_link_l2 calls are successful, the list is empty and 
the while loop either goes into another iteration or the function 
returns successfully (without any further need to unwind the list). If 
some call fails, all previous (successful) calls have already been 
removed from the list, therefore the unwinding only affects L2Meta 
request with failed calls to qcow2_alloc_cluster_link_l2 (or ones where 
that function wasn't called at all).

If the "currently" implied that this will turn out bad if there is a new 
error condition between a successful call to qcow2_alloc_cluster_link_l2 
and the removal of the L2Meta request from the list: Yes, that's true, 
of course. However, as you've said, currently, there is no such 
condition; and I don't see why it should be introduced. The sole purpose 
of the list seems to be (to me) to execute qcow2_alloc_cluster_link_l2 
on every of its elements. Thus, as soon as qcow2_alloc_cluster_link_l2 
is successful, the corresponding request should be removed from the list.

So, in case you do agree that it currently works fine, I would not 
consider it risky; if this patch is applied and some time in the future 
anything introduces a "goto fail" between qcow2_alloc_cluster_link_l2 
and l2_meta = next, this patch would simply have to make sure that 
qcow2_free_clusters isn't called in this case. In the probably very 
unlikely case all my previous assumptions and conclusions were true, I'd 
just add a comment in the qcow2_alloc_cluster_link_l2 loop informing 
about this case (“If you add a goto fail here, make sure to pay 
attention” or something along these lines).

> Also, shouldn't it be QCOW2_DISCARD_OTHER?

I'm always unsure about the discard flags. ;-)

I try to follow the rule of “use the specific type (or ‘other’) for 
freeing ‘out of the blue’, but use ‘always’ if it's just a very recent 
allocation that is being undone again”. I'd gladly accept better 
recommendations. ;-)

Max

Am 10.10.2013 um 14:54 hat Max Reitz geschrieben:
> If the "currently" implied that this will turn out bad if there is a
> new error condition between a successful call to
> qcow2_alloc_cluster_link_l2 and the removal of the L2Meta request
> from the list: Yes, that's true, of course.

Yes, that's the scenario. It seems easy to miss the error handling path
when reviewing a change to this code.

> However, as you've said,
> currently, there is no such condition; and I don't see why it should
> be introduced. The sole purpose of the list seems to be (to me) to
> execute qcow2_alloc_cluster_link_l2 on every of its elements. Thus,
> as soon as qcow2_alloc_cluster_link_l2 is successful, the
> corresponding request should be removed from the list.

If anything isn't complex enough in qcow2, think about how things will
turn out with Delayed COW and chances are that it does become complex.

For example, you can then have other requests running in parallel which
use the newly allocated cluster. You may decrease the refcount only
after the last of them has completed. This is just the first case that
comes to mind, I'm sure there's more fun to be had.

> So, in case you do agree that it currently works fine, I would not
> consider it risky; if this patch is applied and some time in the
> future anything introduces a "goto fail" between
> qcow2_alloc_cluster_link_l2 and l2_meta = next, this patch would
> simply have to make sure that qcow2_free_clusters isn't called in
> this case. In the probably very unlikely case all my previous
> assumptions and conclusions were true, I'd just add a comment in the
> qcow2_alloc_cluster_link_l2 loop informing about this case (“If you
> add a goto fail here, make sure to pay attention” or something along
> these lines).

Adding a comment there sounds like a fair compromise.

> >Also, shouldn't it be QCOW2_DISCARD_OTHER?
> 
> I'm always unsure about the discard flags. ;-)
> 
> I try to follow the rule of “use the specific type (or ‘other’) for
> freeing ‘out of the blue’, but use ‘always’ if it's just a very
> recent allocation that is being undone again”. I'd gladly accept
> better recommendations. ;-)

To be honest, I'm not sure if there are any legitimate use cases for
'always'... Discard is a slow operation, so unless there's a specific
reason anyway, I'd default to 'other' (or a specific type, of course).

Kevin

On 2013-10-10 15:57, Kevin Wolf wrote:
> Am 10.10.2013 um 14:54 hat Max Reitz geschrieben:
>> If the "currently" implied that this will turn out bad if there is a
>> new error condition between a successful call to
>> qcow2_alloc_cluster_link_l2 and the removal of the L2Meta request
>> from the list: Yes, that's true, of course.
> Yes, that's the scenario. It seems easy to miss the error handling path
> when reviewing a change to this code.
>
>> However, as you've said,
>> currently, there is no such condition; and I don't see why it should
>> be introduced. The sole purpose of the list seems to be (to me) to
>> execute qcow2_alloc_cluster_link_l2 on every of its elements. Thus,
>> as soon as qcow2_alloc_cluster_link_l2 is successful, the
>> corresponding request should be removed from the list.
> If anything isn't complex enough in qcow2, think about how things will
> turn out with Delayed COW and chances are that it does become complex.
>
> For example, you can then have other requests running in parallel which
> use the newly allocated cluster. You may decrease the refcount only
> after the last of them has completed. This is just the first case that
> comes to mind, I'm sure there's more fun to be had.

Well, yes, I somehow thought of something like this; but I thought 
that'd be the problem of qcow2_alloc_cluster_link_l2 and as soon as that 
function has been called, though maybe we cannot remove it from the 
requests in flight (global for the BDS), but we still should remove it 
from the local l2meta list.

>> So, in case you do agree that it currently works fine, I would not
>> consider it risky; if this patch is applied and some time in the
>> future anything introduces a "goto fail" between
>> qcow2_alloc_cluster_link_l2 and l2_meta = next, this patch would
>> simply have to make sure that qcow2_free_clusters isn't called in
>> this case. In the probably very unlikely case all my previous
>> assumptions and conclusions were true, I'd just add a comment in the
>> qcow2_alloc_cluster_link_l2 loop informing about this case (“If you
>> add a goto fail here, make sure to pay attention” or something along
>> these lines).
> Adding a comment there sounds like a fair compromise.

Okay, I'll add it.

>>> Also, shouldn't it be QCOW2_DISCARD_OTHER?
>> I'm always unsure about the discard flags. ;-)
>>
>> I try to follow the rule of “use the specific type (or ‘other’) for
>> freeing ‘out of the blue’, but use ‘always’ if it's just a very
>> recent allocation that is being undone again”. I'd gladly accept
>> better recommendations. ;-)
> To be honest, I'm not sure if there are any legitimate use cases for
> 'always'... Discard is a slow operation, so unless there's a specific
> reason anyway, I'd default to 'other' (or a specific type, of course).

Seems easy enough to remember, thanks. ;-)

Max

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 <mreitz@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