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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

10.09.2021 14:42, Stefano Garzarella wrote:
> 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()?

Maybe. Deeper audit is needed to understand do we need to fix something and how to do it properly. Maybe I'll dig into it a bit later.. Mirror code is so overcomplicated.

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

My knowing of mirror code is not good too, unfortunately.

> 
> Thanks,
> Stefano
> 


-- 
Best regards,
Vladimir