[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free i
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit |
Date: |
Mon, 17 Sep 2018 13:37:15 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 17.09.2018 um 00:05 hat Max Reitz geschrieben:
> On 14.09.18 18:25, Kevin Wolf wrote:
> > Am 13.09.2018 um 22:55 hat Max Reitz geschrieben:
> >> On 13.09.18 14:52, Kevin Wolf wrote:
> >>> When starting an active commit job, other callbacks can run before
> >>> mirror_start_job() calls bdrv_ref() where needed and cause the nodes to
> >>> go away. Add another pair of bdrv_ref/unref() around it to protect
> >>> against this case.
> >>>
> >>> Signed-off-by: Kevin Wolf <address@hidden>
> >>> ---
> >>> block/mirror.c | 11 +++++++++++
> >>> 1 file changed, 11 insertions(+)
> >>
> >> Reviewed-by: Max Reitz <address@hidden>
> >>
> >> But... How?
> >
> > Tried to reproduce the other bug Paolo was concerned about (good there
> > is something like 'git reflog'!) and dug up this one instead.
> >
> > So the scenario seems to be test_stream_commit_1 in qemu-iotests 030.
> >
> > The backtrace when the BDS is deleted is the following:
> >
> > (rr) bt
> > #0 0x00007faeaf6145ec in __memset_sse2_unaligned_erms () at
> > /lib64/libc.so.6
> > #1 0x00007faeaf60414e in _int_free () at /lib64/libc.so.6
> > #2 0x00007faeaf6093be in free () at /lib64/libc.so.6
> > #3 0x00007faecaea6b4e in g_free () at /lib64/libglib-2.0.so.0
> > #4 0x000055c0c94f9d6c in bdrv_delete (bs=0x55c0cbe69240) at block.c:3590
> > #5 0x000055c0c94fbe82 in bdrv_unref (bs=0x55c0cbe69240) at block.c:4638
> > #6 0x000055c0c94f6761 in bdrv_root_unref_child (child=0x55c0cbe57af0) at
> > block.c:2188
> > #7 0x000055c0c94fe239 in block_job_remove_all_bdrv (job=0x55c0ccf9e220) at
> > blockjob.c:200
> > #8 0x000055c0c94fdf1f in block_job_free (job=0x55c0ccf9e220) at
> > blockjob.c:94
> > #9 0x000055c0c94ffe0d in job_unref (job=0x55c0ccf9e220) at job.c:368
> > #10 0x000055c0c95007cd in job_do_dismiss (job=0x55c0ccf9e220) at job.c:641
> > #11 0x000055c0c95008d9 in job_conclude (job=0x55c0ccf9e220) at job.c:667
> > #12 0x000055c0c9500b66 in job_finalize_single (job=0x55c0ccf9e220) at
> > job.c:735
> > #13 0x000055c0c94ff4fa in job_txn_apply (txn=0x55c0cbe19eb0,
> > fn=0x55c0c9500a62 <job_finalize_single>, lock=true) at job.c:151
> > #14 0x000055c0c9500e92 in job_do_finalize (job=0x55c0ccf9e220) at job.c:822
> > #15 0x000055c0c9501040 in job_completed_txn_success (job=0x55c0ccf9e220) at
> > job.c:872
> > #16 0x000055c0c950115c in job_completed (job=0x55c0ccf9e220, ret=0,
> > error=0x0) at job.c:892
> > #17 0x000055c0c92b572c in stream_complete (job=0x55c0ccf9e220,
> > opaque=0x55c0cc050bc0) at block/stream.c:96
>
> But isn't this just deletion of node1, i.e. the intermediate node of the
> stream job?
>
> The commit job happens above that (node3 and upwards), so all its BDSs
> shouldn't be affected. So even with the bdrv_ref()s introduced in this
> patch, I'd still expect node1 to be deleted exactly here.
Hm, I suppose that's true.
So it crashed because the drain in bdrv_reopen() didn't actually drain
the streaming job (which includes removing node1), so node1 ended up in
the ReopenQueue and when it disappeared in the middle of reopen, we got
a use after free.
Then it would be bug between job draining and bdrv_reopen() and the
active commit job would only be a victim of the bug without actually
doing anything wrong, and we can drop this patch.
Does this make sense?
Kevin
signature.asc
Description: PGP signature
- Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/17] block-backend: Decrease in_flight only after callback, (continued)
Re: [Qemu-devel] [PATCH v2 11/17] block-backend: Decrease in_flight only after callback, Max Reitz, 2018/09/13
[Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Kevin Wolf, 2018/09/13
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Max Reitz, 2018/09/13
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Max Reitz, 2018/09/13
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Kevin Wolf, 2018/09/14
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Max Reitz, 2018/09/16
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit,
Kevin Wolf <=
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Max Reitz, 2018/09/18
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Kevin Wolf, 2018/09/18
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Max Reitz, 2018/09/18
- Re: [Qemu-devel] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit, Kevin Wolf, 2018/09/20
[Qemu-devel] [PATCH v2 15/17] test-bdrv-drain: Test nested poll in bdrv_drain_poll_top_level(), Kevin Wolf, 2018/09/13
[Qemu-devel] [PATCH v2 13/17] blockjob: Lie better in child_job_drained_poll(), Kevin Wolf, 2018/09/13
[Qemu-devel] [PATCH v2 14/17] block: Remove aio_poll() in bdrv_drain_poll variants, Kevin Wolf, 2018/09/13
[Qemu-devel] [PATCH v2 16/17] job: Avoid deadlocks in job_completed_txn_abort(), Kevin Wolf, 2018/09/13