Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221205124756.426350-1-asmadeus@codewreck.org>
In-Reply-To: <20221205124756.426350-1-asmadeus@codewreck.org>
From:   Marco Elver <elver@google.com>
Date:   Mon, 5 Dec 2022 14:06:18 +0100
Message-ID: <CANpmjNNjSat=TkXUmv351yPGn=6SV27NaXe80dU69Ggy5SHxsg@mail.gmail.com>
Subject: Re: [PATCH] 9p/client: fix data race on req->status
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     Christian Schoenebeck <linux_oss@crudebyte.com>,
        v9fs-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org,
        Naresh Kamboju <naresh.kamboju@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 5 Dec 2022 at 13:50, Dominique Martinet <asmadeus@codewreck.org> wrote:
>
> KCSAN reported a race between writing req->status in p9_client_cb and
> accessing it in p9_client_rpc's wait_event.
>
> Accesses to req itself is protected by the data barrier (writing req
> fields, write barrier, writing status // reading status, read barrier,
> reading other req fields), but status accesses themselves apparently
> also must be annotated properly with WRITE_ONCE/READ_ONCE when we
> access it without locks.
>
> Follows:
>  - error paths writing status in various threads all can notify
> p9_client_rpc, so these all also need WRITE_ONCE
>  - there's a similar read loop in trans_virtio for zc case that also
> needs READ_ONCE
>  - other reads in trans_fd should be protected by the trans_fd lock and
> lists state machine, as corresponding writers all are within trans_fd
> and should be under the same lock. If KCSAN complains on them we likely
> will have something else to fix as well, so it's better to leave them
> unmarked and look again if required.
>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Suggested-by: Marco Elver <elver@google.com>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

Acked-by: Marco Elver <elver@google.com>

In case you're interested, KCSAN has a strict mode that is more
aggressive about which data races it reports (by default we're hiding
several classes of data races). One such class is data races due to
missing memory barriers, where e.g. an unmarked operation can be
reordered in such a way (by compiler or CPU) that a concurrent racy
access occurs. This mode can be enabled with CONFIG_KCSAN_STRICT=y.
It's most effective with some good stress tests for the subsystem of
interest. See https://docs.kernel.org/dev-tools/kcsan.html#modeling-weak-memory

Thanks,
-- Marco