Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()

[Vladimir Sementsov-Ogievskiy]

10.09.2021 11:56, Stefano Garzarella wrote:
> In mirror_iteration() we call mirror_wait_on_conflicts() with
> `self` parameter set to NULL.
>
> Starting from commit d44dae1a7c we dereference `self` pointer in
> mirror_wait_on_conflicts() without checks if it is not NULL.
>
> Backtrace:
>    Program terminated with signal SIGSEGV, Segmentation fault.
>    #0  mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, 
> bytes=<optimized out>)
>        at ../block/mirror.c:172
>    172                  self->waiting_for_op = op;
>    [Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]
>    (gdb) bt
>    #0  mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, 
> bytes=<optimized out>)
>        at ../block/mirror.c:172
>    #1  0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized 
> out>) at ../block/mirror.c:491
>    #2  0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at 
> ../job.c:917
>    #3  0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, 
> i1=<optimized out>)
>        at ../util/coroutine-ucontext.c:173
>    #4  0x00007f0909975820 in ?? () at 
> ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91
>        from /usr/lib64/libc.so.6
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404
> Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in 
> mirror_wait_on_conflicts")
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> I'm not familiar with this code so maybe we can fix the bug differently.
>
> Running iotests and the test in bugzilla, everything seems okay.
>
> Thanks,
> Stefano
> ---
>   block/mirror.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 98fc66eabf..6c834d6a7b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -169,9 +169,16 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp 
> *self,
>                       continue;
>                   }
>   
> -                self->waiting_for_op = op;
> +                if (self) {
> +                    self->waiting_for_op = op;
> +                }
> +
>                   qemu_co_queue_wait(&op->waiting_requests, NULL);
> -                self->waiting_for_op = NULL;
> +
> +                if (self) {
> +                    self->waiting_for_op = NULL;
> +                }
> +
>                   break;
>               }
>           }

Hi Stefano!

The patch is almost OK. The only thing is, I think, you should also put "if (op->waiting_for_op) 
{continue;}" code above into "if (self)" too. Look at the comment above: if we don't have 
"self", than we are not in the list and nobody will wait for us. This means that we should wait for 
all.

So, with my suggested fix, you'll simply roll-back d44dae1a7c for the case of 
self==NULL, which is an additional point of safety.


Still, I wonder, where we check for conflicting requests when create usual 
MirrorOp. We don't call mirror_wait_on_conflicts() in mirror_perform...

--
Best regards,
Vladimir