linux-kernel - Re: [PATCH] btrfs: Fix data race in log_conflicting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL3q7H5SAEs75APMgRLNGZD+Mg6ic04+78M_rseabtidf1w05w@mail.gmail.com>
Date: Fri, 1 Nov 2024 08:44:12 +0000
From: Filipe Manana <fdmanana@...nel.org>
To: Hao-ran Zheng <zhenghaoran@...a.edu.cn>
Cc: clm@...com, josef@...icpanda.com, dsterba@...e.com, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	baijiaju1990@...il.com, 21371365@...a.edu.cn
Subject: Re: [PATCH] btrfs: Fix data race in log_conflicting_inodes

On Fri, Nov 1, 2024 at 3:52 AM Hao-ran Zheng <zhenghaoran@...a.edu.cn> wrote:
>
> The Data Race occurs when the `log_conflicting_inodes()` function is
> executed in different threads at the same time. When one thread assigns
> a value to `ctx->logging_conflict_inodes` while another thread performs
> an `if(ctx->logging_conflict_inodes)` judgment or modifies it at the
> same time, a data contention problem may arise.

No, there's no problem at all.
A log context is thread local, it's never shared between threads.

>
> Further, an atomicity violation may also occur here. Consider the
> following case, when a thread A `if(ctx->logging_conflict_inodes)`
> passes the judgment, the execution switches to another thread B, at
> which time the value of `ctx->logging_conflict_inodes` has not yet
> been assigned true, which would result in multiple threads executing
> `log_conflicting_inodes()`.

No. When you make such claims, please provide a sequence diagram that
shows how the tasks interact, what their call stacks are, so that we
can see where the race happens.

But again, this is completely wrong because a log context (struct
btrfs_log_ctx) is never shared between threads.

Thanks.

>
> To address this issue, it is recommended to add locks to protect
> `logging_conflict_inodes` in the `btrfs_log_ctx` structure, and lock
> protection during assignment and judgment. This modification ensures
> that the value of `ctx->logging_conflict_inodes` does not change during
> the validation process, thereby maintaining its integrity.
>
> Signed-off-by: Hao-ran Zheng <zhenghaoran@...a.edu.cn>
> ---
>  fs/btrfs/tree-log.c | 7 +++++++
>  fs/btrfs/tree-log.h | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 9637c7cdc0cf..9cdbf280ca9a 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2854,6 +2854,7 @@ void btrfs_init_log_ctx(struct btrfs_log_ctx *ctx, struct btrfs_inode *inode)
>         INIT_LIST_HEAD(&ctx->conflict_inodes);
>         ctx->num_conflict_inodes = 0;
>         ctx->logging_conflict_inodes = false;
> +       spin_lock_init(&ctx->logging_conflict_inodes_lock);
>         ctx->scratch_eb = NULL;
>  }
>
> @@ -5779,16 +5780,20 @@ static int log_conflicting_inodes(struct btrfs_trans_handle *trans,
>                                   struct btrfs_log_ctx *ctx)
>  {
>         int ret = 0;
> +       unsigned long logging_conflict_inodes_flags;
>
>         /*
>          * Conflicting inodes are logged by the first call to btrfs_log_inode(),
>          * otherwise we could have unbounded recursion of btrfs_log_inode()
>          * calls. This check guarantees we can have only 1 level of recursion.
>          */
> +       spin_lock_irqsave(&ctx->conflict_inodes_lock, logging_conflict_inodes_flags);

Even if this was remotely correct, why the irqsave? The fsync code is
never called under irq context.

>         if (ctx->logging_conflict_inodes)
> +               spin_unlock_irqrestore(&ctx->conflict_inodes_lock, logging_conflict_inodes_flags);
>                 return 0;
>
>         ctx->logging_conflict_inodes = true;
> +       spin_unlock_irqrestore(&ctx->conflict_inodes_lock, logging_conflict_inodes_flags);
>
>         /*
>          * New conflicting inodes may be found and added to the list while we
> @@ -5869,7 +5874,9 @@ static int log_conflicting_inodes(struct btrfs_trans_handle *trans,
>                         break;
>         }
>
> +       spin_lock_irqsave(&ctx->conflict_inodes_lock, logging_conflict_inodes_flags);
>         ctx->logging_conflict_inodes = false;
> +       spin_unlock_irqrestore(&ctx->conflict_inodes_lock, logging_conflict_inodes_flags);
>         if (ret)
>                 free_conflicting_inodes(ctx);
>
> diff --git a/fs/btrfs/tree-log.h b/fs/btrfs/tree-log.h
> index dc313e6bb2fa..0f862d0c80f2 100644
> --- a/fs/btrfs/tree-log.h
> +++ b/fs/btrfs/tree-log.h
> @@ -44,6 +44,7 @@ struct btrfs_log_ctx {
>         struct list_head conflict_inodes;
>         int num_conflict_inodes;
>         bool logging_conflict_inodes;
> +       spinlock_t logging_conflict_inodes_lock;
>         /*
>          * Used for fsyncs that need to copy items from the subvolume tree to
>          * the log tree (full sync flag set or copy everything flag set) to
> --
> 2.34.1
>
>