The final bdrv_set_backing_hd() could be working on already freed nodes
because the commit job drops its references (through BlockBackends) to
both overlay_bs and top already a bit earlier.

One way to trigger the bug is hot unplugging a disk for which
blockdev_mark_auto_del() cancels the block job.

Fix this by taking BDS-level references while we're still using the
nodes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/commit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index a3028b2..af6fa68 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
     int ret = data->ret;
     bool remove_commit_top_bs = false;
 
+    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
+    bdrv_ref(top);
+    bdrv_ref(overlay_bs);
+
     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
      * the normal backing chain can be restored. */
     blk_unref(s->base);
@@ -124,6 +128,9 @@ static void commit_complete(BlockJob *job, void *opaque)
     if (remove_commit_top_bs) {
         bdrv_set_backing_hd(overlay_bs, top, &error_abort);
     }
+
+    bdrv_unref(overlay_bs);
+    bdrv_unref(top);
 }
 
 static void coroutine_fn commit_run(void *opaque)
-- 
1.8.3.1

On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> The final bdrv_set_backing_hd() could be working on already freed nodes
> because the commit job drops its references (through BlockBackends) to
> both overlay_bs and top already a bit earlier.
>
> One way to trigger the bug is hot unplugging a disk for which
> blockdev_mark_auto_del() cancels the block job.
>
> Fix this by taking BDS-level references while we're still using the
> nodes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block/commit.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/block/commit.c b/block/commit.c
> index a3028b2..af6fa68 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
>      int ret = data->ret;
>      bool remove_commit_top_bs = false;
>
> +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> +    bdrv_ref(top);
> +    bdrv_ref(overlay_bs);
> +
>      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>       * the normal backing chain can be restored. */
>      blk_unref(s->base);

Hi -- coverity complains about this change, because bdrv_ref()
assumes that its argument is not NULL, but later on in commit_complete()
we have a check
    "if (overlay_bs && ...)"
which assumes its argument might be NULL. (CID 1376205)

Which is correct?

thanks
-- PMM

Am 13.06.2017 um 18:12 hat Peter Maydell geschrieben:
> On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> > diff --git a/block/commit.c b/block/commit.c
> > index a3028b2..af6fa68 100644
> > --- a/block/commit.c
> > +++ b/block/commit.c
> > @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
> >      int ret = data->ret;
> >      bool remove_commit_top_bs = false;
> >
> > +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> > +    bdrv_ref(top);
> > +    bdrv_ref(overlay_bs);
> > +
> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> >       * the normal backing chain can be restored. */
> >      blk_unref(s->base);
> 
> Hi -- coverity complains about this change, because bdrv_ref()
> assumes that its argument is not NULL, but later on in commit_complete()
> we have a check
>     "if (overlay_bs && ...)"
> which assumes its argument might be NULL. (CID 1376205)
> 
> Which is correct?

I saw the Coverity report and am looking into it. It's not completely
clear to me yet which is correct, but I suspect it can be NULL.

Kevin

On 13 June 2017 at 17:46, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.06.2017 um 18:12 hat Peter Maydell geschrieben:
>> On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
>> > diff --git a/block/commit.c b/block/commit.c
>> > index a3028b2..af6fa68 100644
>> > --- a/block/commit.c
>> > +++ b/block/commit.c
>> > @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
>> >      int ret = data->ret;
>> >      bool remove_commit_top_bs = false;
>> >
>> > +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
>> > +    bdrv_ref(top);
>> > +    bdrv_ref(overlay_bs);
>> > +
>> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>> >       * the normal backing chain can be restored. */
>> >      blk_unref(s->base);
>>
>> Hi -- coverity complains about this change, because bdrv_ref()
>> assumes that its argument is not NULL, but later on in commit_complete()
>> we have a check
>>     "if (overlay_bs && ...)"
>> which assumes its argument might be NULL. (CID 1376205)
>>
>> Which is correct?
>
> I saw the Coverity report and am looking into it. It's not completely
> clear to me yet which is correct, but I suspect it can be NULL.

Just a nudge on this one -- I don't think there's been a patch sent
to the list for this check-after-use ?

(It's one of just 7 coverity issues left which haven't had at least
a patch sent to the list now...)

thanks
-- PMM

Am 09.07.2017 um 19:09 hat Peter Maydell geschrieben:
> On 13 June 2017 at 17:46, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 13.06.2017 um 18:12 hat Peter Maydell geschrieben:
> >> On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> >> > diff --git a/block/commit.c b/block/commit.c
> >> > index a3028b2..af6fa68 100644
> >> > --- a/block/commit.c
> >> > +++ b/block/commit.c
> >> > @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
> >> >      int ret = data->ret;
> >> >      bool remove_commit_top_bs = false;
> >> >
> >> > +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> >> > +    bdrv_ref(top);
> >> > +    bdrv_ref(overlay_bs);
> >> > +
> >> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> >> >       * the normal backing chain can be restored. */
> >> >      blk_unref(s->base);
> >>
> >> Hi -- coverity complains about this change, because bdrv_ref()
> >> assumes that its argument is not NULL, but later on in commit_complete()
> >> we have a check
> >>     "if (overlay_bs && ...)"
> >> which assumes its argument might be NULL. (CID 1376205)
> >>
> >> Which is correct?
> >
> > I saw the Coverity report and am looking into it. It's not completely
> > clear to me yet which is correct, but I suspect it can be NULL.
> 
> Just a nudge on this one -- I don't think there's been a patch sent
> to the list for this check-after-use ?
> 
> (It's one of just 7 coverity issues left which haven't had at least
> a patch sent to the list now...)

As far as I can tell, this can't currently be triggered. I intended to
fix it with some work on the commit block job that I need to do anyway,
and which would potentially enable a way to trigger it. But it turned
out that this is a bit more complicated than I thought.

So maybe I'd better just post a very small patch that silences Coverity
(without making a practical difference) until I can finish the real
thing.

Kevin