[PATCH] 9p/virtio: add a read barrier in p9_virtio_zc_request

[PATCH] 9p/virtio: add a read barrier in p9_virtio_zc_request

[Dominique Martinet]

The request receiving thread writes into request then marks the request
valid in p9_client_cb by setting status after a write barrier.

p9_virtio_zc_request must like p9_client_rpc issue a read barrier after
getting notified of the new request status before reading other request
files.

(This has been noticed while fixing the usage of READ/WRITE_ONCE macros
for request status)

Link: https://lkml.kernel.org/r/167052961.MU3OA6Uzks@silver
Reported-by: Christian Schoenebeck <linux_oss@crudebyte.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 net/9p/trans_virtio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 3c27ffb781e3..98425c63b3c3 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -533,6 +533,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
 	err = wait_event_killable(req->wq,
 			          READ_ONCE(req->status) >= REQ_STATUS_RCVD);
+
+	/* Make sure our req is coherent with regard to updates in other
+	 * threads - echoes to wmb() in the callback like p9_client_rpc
+	 */
+	smp_rmb();
+
 	// RERROR needs reply (== error string) in static data
 	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
 	    unlikely(req->rc.sdata[4] == P9_RERROR))
-- 
2.38.1

Re: [PATCH] 9p/virtio: add a read barrier in p9_virtio_zc_request

[Christian Schoenebeck]

On Saturday, December 10, 2022 1:10:44 AM CET Dominique Martinet wrote:
> The request receiving thread writes into request then marks the request
> valid in p9_client_cb by setting status after a write barrier.
> 
> p9_virtio_zc_request must like p9_client_rpc issue a read barrier after
> getting notified of the new request status before reading other request
> files.
> 
> (This has been noticed while fixing the usage of READ/WRITE_ONCE macros
> for request status)
> 
> Link: https://lkml.kernel.org/r/167052961.MU3OA6Uzks@silver
> Reported-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
>  net/9p/trans_virtio.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 3c27ffb781e3..98425c63b3c3 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -533,6 +533,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
>  	err = wait_event_killable(req->wq,
>  			          READ_ONCE(req->status) >= REQ_STATUS_RCVD);
> +
> +	/* Make sure our req is coherent with regard to updates in other
> +	 * threads - echoes to wmb() in the callback like p9_client_rpc
> +	 */
> +	smp_rmb();
> +

Oh, I had p9_client_zc_rpc() for this in mind, but I see why you chose this
place in p9_virtio_zc_request() instead. LGTM

I also made some tests to check whether this barrier would hurt performance,
but I measured no difference. So this should be good to go:

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

>  	// RERROR needs reply (== error string) in static data
>  	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
>  	    unlikely(req->rc.sdata[4] == P9_RERROR))
> 

Re: [PATCH] 9p/virtio: add a read barrier in p9_virtio_zc_request

[Dominique Martinet]

Christian Schoenebeck wrote on Mon, Dec 12, 2022 at 02:35:39PM +0100:
> > diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> > index 3c27ffb781e3..98425c63b3c3 100644
> > --- a/net/9p/trans_virtio.c
> > +++ b/net/9p/trans_virtio.c
> > @@ -533,6 +533,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
> >  	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
> >  	err = wait_event_killable(req->wq,
> >  			          READ_ONCE(req->status) >= REQ_STATUS_RCVD);
> > +
> > +	/* Make sure our req is coherent with regard to updates in other
> > +	 * threads - echoes to wmb() in the callback like p9_client_rpc
> > +	 */
> > +	smp_rmb();
> > +
> 
> Oh, I had p9_client_zc_rpc() for this in mind, but I see why you chose this
> place in p9_virtio_zc_request() instead. LGTM

Yes, we access req data here so as much as it'd make more sense to keep
it symetrical in p9_client_zc_rpc (like p9_client_rpc) I think we need
it here.

> I also made some tests to check whether this barrier would hurt performance,
> but I measured no difference. So this should be good to go:

Thanks!
The assembly generated with the barrier is actually slightly shorter for
x86_64, but it's hard to tell the actual performance impact....

> Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

Cheers, I've queued this patch as well: let's make that this merge
windows' batch unless a problem comes up.

-- 
Dominique

Re: [PATCH] 9p/virtio: add a read barrier in p9_virtio_zc_request

[Dominique Martinet]

(Your mailer breaks threads, please have a look at how to make it send
In-Reply-To and/or References headers)

Hillf Danton wrote on Tue, Dec 13, 2022 at 02:59:01PM +0800:
> On 10 Dec 2022 09:10:44 +0900 Dominique Martinet <asmadeus@codewreck.org>
> > @@ -533,6 +533,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
> >  	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
> >  	err = wait_event_killable(req->wq,
> >  			          READ_ONCE(req->status) >= REQ_STATUS_RCVD);
> > +
> > +	/* Make sure our req is coherent with regard to updates in other
> > +	 * threads - echoes to wmb() in the callback like p9_client_rpc
> > +	 */
> > +	smp_rmb();
> > +
> >  	// RERROR needs reply (== error string) in static data
> >  	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
> >  	    unlikely(req->rc.sdata[4] == P9_RERROR))
> 
> No sense can be made without checking err before req->status,
> given the comment below. Worse after this change.

Hmm, I don't see how it's worse (well, it makes it more likely for
req->status to be RCVD after the barrier without the rest of the data
being coherent I guess), but it's definitely incorrect, yes...
Thanks for bringing it up.


Having another look I also don't see how this can possibly be safe at
all: if a process is killed during waiting here, p9_virtio_zc_request
will drop pages it reserved for the response (in the need_drop case) and
sg lists will be freed but the response can still come for a while --
these need to be dropped only after flush has been handled.

If these buffers are reused while the response comes we'll be overriding
some random data...


This isn't an easy fix, I'll just drop this patch for now; but I guess
we should try to address that next cycle.

Perhaps I can try to find time to dust off my async flush code, some
other fix might have resolved the race I used to see with it...

-- 
Dominique