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

[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.

> 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?

Kevin