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

[Stefano Garzarella]

On Fri, Sep 10, 2021 at 01:37:40PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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.

Right, I'll send a v2 with this change.

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

IIUC in mirror_iteration() we call mirror_wait_on_conflicts() at the 
beginning, then in the cycle we call mirror_perform() where we create a 
new MirrorOp and we add it in the `ops_in_flight` list.

At this point, should we call mirror_wait_on_conflicts()?

I need to understand the code better to follow you... :-)

Thanks,
Stefano