Patchwork qed: fix use-after-free during l2 cache commit

login
register
mail settings
Submitter Stefan Hajnoczi
Date Sept. 30, 2011, 10:39 a.m.
Message ID <1317379151-11557-1-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/117085/
State New
Headers show

Comments

Stefan Hajnoczi - Sept. 30, 2011, 10:39 a.m.
QED's metadata caching strategy allows two parallel requests to race for
metadata lookup.  The first one to complete will populate the metadata
cache and the second one will drop the data it just read in favor of the
cached data.

There is a use-after-free in qed_read_l2_table_cb() and
qed_commit_l2_update() where l2_table->offset was used after the
l2_table may have been freed due to a metadata lookup race.  Fix this by
keeping the l2_offset in a local variable and not reaching into the
possibly freed l2_table.

Reported-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
Hi Amit,
Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
Does this patch fix the problem?

If not, please send details on your setup and how to reproduce the issue.

Thanks,
Stefan

 block/qed-table.c |    6 +++---
 block/qed.c       |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)
Amit Shah - Sept. 30, 2011, 11:27 a.m.
On (Fri) 30 Sep 2011 [11:39:11], Stefan Hajnoczi wrote:
> QED's metadata caching strategy allows two parallel requests to race for
> metadata lookup.  The first one to complete will populate the metadata
> cache and the second one will drop the data it just read in favor of the
> cached data.
> 
> There is a use-after-free in qed_read_l2_table_cb() and
> qed_commit_l2_update() where l2_table->offset was used after the
> l2_table may have been freed due to a metadata lookup race.  Fix this by
> keeping the l2_offset in a local variable and not reaching into the
> possibly freed l2_table.
> 
> Reported-by: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> Hi Amit,
> Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
> Does this patch fix the problem?

Yes, this fixes it.

Thanks,

		Amit
Stefan Hajnoczi - Sept. 30, 2011, 3:23 p.m.
On Fri, Sep 30, 2011 at 12:27 PM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Fri) 30 Sep 2011 [11:39:11], Stefan Hajnoczi wrote:
>> QED's metadata caching strategy allows two parallel requests to race for
>> metadata lookup.  The first one to complete will populate the metadata
>> cache and the second one will drop the data it just read in favor of the
>> cached data.
>>
>> There is a use-after-free in qed_read_l2_table_cb() and
>> qed_commit_l2_update() where l2_table->offset was used after the
>> l2_table may have been freed due to a metadata lookup race.  Fix this by
>> keeping the l2_offset in a local variable and not reaching into the
>> possibly freed l2_table.
>>
>> Reported-by: Amit Shah <amit.shah@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>> Hi Amit,
>> Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
>> Does this patch fix the problem?
>
> Yes, this fixes it.

Were you able to reliably reproduce the assertion failure before?

I wonder because this only happens when two metadata lookups race
(which is rare enough on my setup that I've never seen this failure).
It might be worth trying a few times.

Stefan
Stefan Hajnoczi - Sept. 30, 2011, 3:26 p.m.
On Fri, Sep 30, 2011 at 11:39 AM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> QED's metadata caching strategy allows two parallel requests to race for
> metadata lookup.  The first one to complete will populate the metadata
> cache and the second one will drop the data it just read in favor of the
> cached data.
>
> There is a use-after-free in qed_read_l2_table_cb() and
> qed_commit_l2_update() where l2_table->offset was used after the
> l2_table may have been freed due to a metadata lookup race.  Fix this by
> keeping the l2_offset in a local variable and not reaching into the
> possibly freed l2_table.
>
> Reported-by: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Please consider this for -stable.  It's a pretty small/simple fix and
can prevent an assertion failure.

Stefan
Amit Shah - Sept. 30, 2011, 3:49 p.m.
On (Fri) 30 Sep 2011 [16:23:30], Stefan Hajnoczi wrote:
> On Fri, Sep 30, 2011 at 12:27 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Fri) 30 Sep 2011 [11:39:11], Stefan Hajnoczi wrote:
> >> QED's metadata caching strategy allows two parallel requests to race for
> >> metadata lookup.  The first one to complete will populate the metadata
> >> cache and the second one will drop the data it just read in favor of the
> >> cached data.
> >>
> >> There is a use-after-free in qed_read_l2_table_cb() and
> >> qed_commit_l2_update() where l2_table->offset was used after the
> >> l2_table may have been freed due to a metadata lookup race.  Fix this by
> >> keeping the l2_offset in a local variable and not reaching into the
> >> possibly freed l2_table.
> >>
> >> Reported-by: Amit Shah <amit.shah@redhat.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> >> ---
> >> Hi Amit,
> >> Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
> >> Does this patch fix the problem?
> >
> > Yes, this fixes it.
> 
> Were you able to reliably reproduce the assertion failure before?

Absolutely.

I even reverted the patch and tried the same image; same segfault
again.

> I wonder because this only happens when two metadata lookups race
> (which is rare enough on my setup that I've never seen this failure).
> It might be worth trying a few times.

Get the F16 beta-rc LXE live iso, install guest.  It doesn't cleanly
reboot, you have to kill the VM.  Next start of the VM produces this
segfault.

https://alt.fedoraproject.org/pub/alt/stage/16-Beta.RC2/Live/x86_64/Fedora-16-Beta-x86_64-Live-LXDE.iso


		Amit
Anthony Liguori - Oct. 3, 2011, 7:53 p.m.
On 09/30/2011 10:26 AM, Stefan Hajnoczi wrote:
> On Fri, Sep 30, 2011 at 11:39 AM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com>  wrote:
>> QED's metadata caching strategy allows two parallel requests to race for
>> metadata lookup.  The first one to complete will populate the metadata
>> cache and the second one will drop the data it just read in favor of the
>> cached data.
>>
>> There is a use-after-free in qed_read_l2_table_cb() and
>> qed_commit_l2_update() where l2_table->offset was used after the
>> l2_table may have been freed due to a metadata lookup race.  Fix this by
>> keeping the l2_offset in a local variable and not reaching into the
>> possibly freed l2_table.
>>
>> Reported-by: Amit Shah<amit.shah@redhat.com>
>> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
>
> Please consider this for -stable.  It's a pretty small/simple fix and
> can prevent an assertion failure.

Justin is looking to cut a stable release this week.  Since Kevin is out on 
holiday, unless anyone objects, I'll commit this so that it can make the next 
stable release.

Regards,

Anthony Liguori

>
> Stefan
>
>
Anthony Liguori - Oct. 5, 2011, 4:17 p.m.
On 09/30/2011 05:39 AM, Stefan Hajnoczi wrote:
> QED's metadata caching strategy allows two parallel requests to race for
> metadata lookup.  The first one to complete will populate the metadata
> cache and the second one will drop the data it just read in favor of the
> cached data.
>
> There is a use-after-free in qed_read_l2_table_cb() and
> qed_commit_l2_update() where l2_table->offset was used after the
> l2_table may have been freed due to a metadata lookup race.  Fix this by
> keeping the l2_offset in a local variable and not reaching into the
> possibly freed l2_table.
>
> Reported-by: Amit Shah<amit.shah@redhat.com>
> Signed-off-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
> Hi Amit,
> Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
> Does this patch fix the problem?
>
> If not, please send details on your setup and how to reproduce the issue.
>
> Thanks,
> Stefan
>
>   block/qed-table.c |    6 +++---
>   block/qed.c       |    4 ++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block/qed-table.c b/block/qed-table.c
> index d96afa8..f31f9ff 100644
> --- a/block/qed-table.c
> +++ b/block/qed-table.c
> @@ -222,21 +222,21 @@ static void qed_read_l2_table_cb(void *opaque, int ret)
>       QEDRequest *request = read_l2_table_cb->request;
>       BDRVQEDState *s = read_l2_table_cb->s;
>       CachedL2Table *l2_table = request->l2_table;
> +    uint64_t l2_offset = read_l2_table_cb->l2_offset;
>
>       if (ret) {
>           /* can't trust loaded L2 table anymore */
>           qed_unref_l2_cache_entry(l2_table);
>           request->l2_table = NULL;
>       } else {
> -        l2_table->offset = read_l2_table_cb->l2_offset;
> +        l2_table->offset = l2_offset;
>
>           qed_commit_l2_cache_entry(&s->l2_cache, l2_table);
>
>           /* This is guaranteed to succeed because we just committed the entry
>            * to the cache.
>            */
> -        request->l2_table = qed_find_l2_cache_entry(&s->l2_cache,
> -                                                    l2_table->offset);
> +        request->l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset);
>           assert(request->l2_table != NULL);
>       }
>
> diff --git a/block/qed.c b/block/qed.c
> index 624e261..e87dc4d 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -911,14 +911,14 @@ static void qed_commit_l2_update(void *opaque, int ret)
>       QEDAIOCB *acb = opaque;
>       BDRVQEDState *s = acb_to_s(acb);
>       CachedL2Table *l2_table = acb->request.l2_table;
> +    uint64_t l2_offset = l2_table->offset;
>
>       qed_commit_l2_cache_entry(&s->l2_cache, l2_table);
>
>       /* This is guaranteed to succeed because we just committed the entry to the
>        * cache.
>        */
> -    acb->request.l2_table = qed_find_l2_cache_entry(&s->l2_cache,
> -                                                    l2_table->offset);
> +    acb->request.l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset);
>       assert(acb->request.l2_table != NULL);
>
>       qed_aio_next_io(opaque, ret);
Kevin Wolf - Oct. 11, 2011, 2:22 p.m.
Am 30.09.2011 17:49, schrieb Amit Shah:
> On (Fri) 30 Sep 2011 [16:23:30], Stefan Hajnoczi wrote:
>> On Fri, Sep 30, 2011 at 12:27 PM, Amit Shah <amit.shah@redhat.com> wrote:
>>> On (Fri) 30 Sep 2011 [11:39:11], Stefan Hajnoczi wrote:
>>>> QED's metadata caching strategy allows two parallel requests to race for
>>>> metadata lookup.  The first one to complete will populate the metadata
>>>> cache and the second one will drop the data it just read in favor of the
>>>> cached data.
>>>>
>>>> There is a use-after-free in qed_read_l2_table_cb() and
>>>> qed_commit_l2_update() where l2_table->offset was used after the
>>>> l2_table may have been freed due to a metadata lookup race.  Fix this by
>>>> keeping the l2_offset in a local variable and not reaching into the
>>>> possibly freed l2_table.
>>>>
>>>> Reported-by: Amit Shah <amit.shah@redhat.com>
>>>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>> ---
>>>> Hi Amit,
>>>> Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
>>>> Does this patch fix the problem?
>>>
>>> Yes, this fixes it.
>>
>> Were you able to reliably reproduce the assertion failure before?
> 
> Absolutely.
> 
> I even reverted the patch and tried the same image; same segfault
> again.
> 
>> I wonder because this only happens when two metadata lookups race
>> (which is rare enough on my setup that I've never seen this failure).
>> It might be worth trying a few times.
> 
> Get the F16 beta-rc LXE live iso, install guest.  It doesn't cleanly
> reboot, you have to kill the VM.  Next start of the VM produces this
> segfault.
> 
> https://alt.fedoraproject.org/pub/alt/stage/16-Beta.RC2/Live/x86_64/Fedora-16-Beta-x86_64-Live-LXDE.iso

Can we try to artificially produce it in a qemu-iotests case?

Kevin
Stefan Hajnoczi - Oct. 12, 2011, 7:53 a.m.
On Tue, Oct 11, 2011 at 04:22:11PM +0200, Kevin Wolf wrote:
> Am 30.09.2011 17:49, schrieb Amit Shah:
> > On (Fri) 30 Sep 2011 [16:23:30], Stefan Hajnoczi wrote:
> >> On Fri, Sep 30, 2011 at 12:27 PM, Amit Shah <amit.shah@redhat.com> wrote:
> >>> On (Fri) 30 Sep 2011 [11:39:11], Stefan Hajnoczi wrote:
> >>>> QED's metadata caching strategy allows two parallel requests to race for
> >>>> metadata lookup.  The first one to complete will populate the metadata
> >>>> cache and the second one will drop the data it just read in favor of the
> >>>> cached data.
> >>>>
> >>>> There is a use-after-free in qed_read_l2_table_cb() and
> >>>> qed_commit_l2_update() where l2_table->offset was used after the
> >>>> l2_table may have been freed due to a metadata lookup race.  Fix this by
> >>>> keeping the l2_offset in a local variable and not reaching into the
> >>>> possibly freed l2_table.
> >>>>
> >>>> Reported-by: Amit Shah <amit.shah@redhat.com>
> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> >>>> ---
> >>>> Hi Amit,
> >>>> Thanks for reporting the assertion failure you saw at http://fpaste.org/CDuv/.
> >>>> Does this patch fix the problem?
> >>>
> >>> Yes, this fixes it.
> >>
> >> Were you able to reliably reproduce the assertion failure before?
> > 
> > Absolutely.
> > 
> > I even reverted the patch and tried the same image; same segfault
> > again.
> > 
> >> I wonder because this only happens when two metadata lookups race
> >> (which is rare enough on my setup that I've never seen this failure).
> >> It might be worth trying a few times.
> > 
> > Get the F16 beta-rc LXE live iso, install guest.  It doesn't cleanly
> > reboot, you have to kill the VM.  Next start of the VM produces this
> > segfault.
> > 
> > https://alt.fedoraproject.org/pub/alt/stage/16-Beta.RC2/Live/x86_64/Fedora-16-Beta-x86_64-Live-LXDE.iso
> 
> Can we try to artificially produce it in a qemu-iotests case?

I will take a look.

Stefan

Patch

diff --git a/block/qed-table.c b/block/qed-table.c
index d96afa8..f31f9ff 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -222,21 +222,21 @@  static void qed_read_l2_table_cb(void *opaque, int ret)
     QEDRequest *request = read_l2_table_cb->request;
     BDRVQEDState *s = read_l2_table_cb->s;
     CachedL2Table *l2_table = request->l2_table;
+    uint64_t l2_offset = read_l2_table_cb->l2_offset;
 
     if (ret) {
         /* can't trust loaded L2 table anymore */
         qed_unref_l2_cache_entry(l2_table);
         request->l2_table = NULL;
     } else {
-        l2_table->offset = read_l2_table_cb->l2_offset;
+        l2_table->offset = l2_offset;
 
         qed_commit_l2_cache_entry(&s->l2_cache, l2_table);
 
         /* This is guaranteed to succeed because we just committed the entry
          * to the cache.
          */
-        request->l2_table = qed_find_l2_cache_entry(&s->l2_cache,
-                                                    l2_table->offset);
+        request->l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset);
         assert(request->l2_table != NULL);
     }
 
diff --git a/block/qed.c b/block/qed.c
index 624e261..e87dc4d 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -911,14 +911,14 @@  static void qed_commit_l2_update(void *opaque, int ret)
     QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
     CachedL2Table *l2_table = acb->request.l2_table;
+    uint64_t l2_offset = l2_table->offset;
 
     qed_commit_l2_cache_entry(&s->l2_cache, l2_table);
 
     /* This is guaranteed to succeed because we just committed the entry to the
      * cache.
      */
-    acb->request.l2_table = qed_find_l2_cache_entry(&s->l2_cache,
-                                                    l2_table->offset);
+    acb->request.l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset);
     assert(acb->request.l2_table != NULL);
 
     qed_aio_next_io(opaque, ret);