[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v10 10/21] block/mirror.c: use of job helpers in drivers to a
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v10 10/21] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU |
Date: |
Tue, 16 Aug 2022 16:23:54 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
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
>