diff options
author | Dominique Martinet <asmadeus@codewreck.org> | 2022-12-05 15:39:01 +0300 |
---|---|---|
committer | Dominique Martinet <asmadeus@codewreck.org> | 2022-12-13 07:02:15 +0300 |
commit | 1a4f69ef15ec29b213e2b086b2502644e8ef76ee (patch) | |
tree | 6be80dae09b8edee7e7e10cac8b79a56b7508e95 /net/9p/trans_virtio.c | |
parent | a31b3cffbd8e5d032dcb267bf94ee48d71c1a28b (diff) | |
download | linux-1a4f69ef15ec29b213e2b086b2502644e8ef76ee.tar.xz |
9p/client: fix data race on req->status
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.
Link: https://lkml.kernel.org/r/20221205124756.426350-1-asmadeus@codewreck.org
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Suggested-by: Marco Elver <elver@google.com>
Acked-by: Marco Elver <elver@google.com>
Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
Diffstat (limited to 'net/9p/trans_virtio.c')
-rw-r--r-- | net/9p/trans_virtio.c | 9 |
1 files changed, 5 insertions, 4 deletions
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 19bccfa0d593..3c27ffb781e3 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -262,7 +262,7 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req) p9_debug(P9_DEBUG_TRANS, "9p debug: virtio request\n"); - req->status = REQ_STATUS_SENT; + WRITE_ONCE(req->status, REQ_STATUS_SENT); req_retry: spin_lock_irqsave(&chan->lock, flags); @@ -468,7 +468,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req, inlen = n; } } - req->status = REQ_STATUS_SENT; + WRITE_ONCE(req->status, REQ_STATUS_SENT); req_retry_pinned: spin_lock_irqsave(&chan->lock, flags); @@ -531,9 +531,10 @@ req_retry_pinned: spin_unlock_irqrestore(&chan->lock, flags); kicked = 1; p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n"); - err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD); + err = wait_event_killable(req->wq, + READ_ONCE(req->status) >= REQ_STATUS_RCVD); // RERROR needs reply (== error string) in static data - if (req->status == REQ_STATUS_RCVD && + if (READ_ONCE(req->status) == REQ_STATUS_RCVD && unlikely(req->rc.sdata[4] == P9_RERROR)) handle_rerror(req, in_hdr_len, offs, in_pages); |