Re: [PATCH v10 10/21] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

Am 04/08/2022 um 18:35 schrieb Kevin Wolf:
> Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben:
>> Once job lock is used and aiocontext is removed, mirror has
>> to perform job operations under the same critical section,
>> using the helpers prepared in previous commit.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Can you explain in the commit message what the TOC/TOU case is that this
> patch is addressing? It's not obvious to me why you picked exactly these
> places to add locking.

Well after thinking about it, mentioning TOC/TOU doesn't really make
sense here. I'll remove the "to avoid TOC/TOU" in the title.

> 
>> diff --git a/block/mirror.c b/block/mirror.c
>> index d8ecb9efa2..b38676e19d 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -654,9 +654,13 @@ static int mirror_exit_common(Job *job)
>>      BlockDriverState *target_bs;
>>      BlockDriverState *mirror_top_bs;
>>      Error *local_err = NULL;
>> -    bool abort = job->ret < 0;
>> +    bool abort;
>>      int ret = 0;
>>  
>> +    WITH_JOB_LOCK_GUARD() {
>> +        abort = job->ret < 0;
>> +    }
> 
> This is the most mysterious hunk to me. The only thing that should
> modify job->ret is the caller of this function anyway, but let's assume
> for a moment that another thread could write to it.
> 
> Then why is it only important that we hold the lock when we're reading
> the value, but not any more when we are actually using it? And what is
> the TOC/TOU that this fixes?

No TOC/TOU, no sense for this fix too. I'll remove this hunk too.

Emanuele
> 
> Kevin
>