Re: [PATCH] 9p: pin request under client lock in p9_tag_lookup

[Jérémy Jean]

On 2026-10-29 06:50, Dominique Martinet wrote:
> Jérémy Jean wrote on Wed, Sep 30, 2026 at 08:13:58PM +0000:
> > 9P clients keep separate request tables but share one request cache.
> > p9_tag_lookup() can race with the final p9_req_put() and pin a request
>
> Ah.. Right, just checking the tag isn't enough, we also need to check
> the client... I'm not sure why I had assumed the caches were not 
> merged,
> they're only not merged with different types..
>
> So technically we could add a reference or some kind of id for the
> client and check that, but given SLAB_TYPESAFE_BY_RCU is hard to grok I
> guess it's much simpler to just take the lock and be done with it...
> (in that case we can get rid of SLAB_TYPESAFE_BY_RCU altogether, the
> lock is enough guarantee that the memory is valid until we get the ref
> here)
>
> It's far from a new bug so I'd rather not rush this just to go back to
> rcu a few weeks later, give me a bit of time to check how much impact
> taking the lock here adds so we can decide. There's enough leeway in 
> the
> p9_req_t struct that we can check the client for free if we care...
>
> Meanwhile, if someone else has an opinon feel free to chip in

Hello Dominique,

Thanks for the reply, and I'm happy to discuss the best way to fix
the UAF.

I am not so familiar with that code base and I have a hard time
deciding the actual impact of the lock. As you said, it might be
possible to keep p9_tag_lookup() lockless by storing the owning
client in each request and then validating both the client and the
tag after successfully incrementing the refcount.

The two helpers refcount_inc_not_zero_acquire() and
refcount_set_release() should ensure that the client and tag checks
see the newly initialized request state after reuse. Then, the
mismatched reference could be dropped through req->client. This
should stop a stale lookup from mistaking a recycled request from
another client for the original one.

Below is a tentative patch that applies this strategy. I have
verified with my reproducer that it indeed fixed the UAF. What
do you think of it? Let me know if you'd want me to send a v2.

Best regards,
Jérémy

---

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 55c6cb54bd254..a46187833cb8d 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -84,6 +84,7 @@ struct p9_req_t {
        int status;
        int t_err;
        refcount_t refcount;
+       struct p9_client *client;
        wait_queue_head_t wq;
        struct p9_fcall tc;
        struct p9_fcall rc;
@@ -333,7 +334,7 @@ static inline void p9_req_get(struct p9_req_t *r)

 static inline int p9_req_try_get(struct p9_req_t *r)
 {
-       return refcount_inc_not_zero(&r->refcount);
+       return refcount_inc_not_zero_acquire(&r->refcount);
 }

 int p9_req_put(struct p9_client *c, struct p9_req_t *r);
diff --git a/net/9p/client.c b/net/9p/client.c
index ef64546c6d52a..31e4bfa4901e3 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -181,9 +181,10 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint 
t_size, uint r_size,
        p9pdu_reset(&req->rc);
        req->t_err = 0;
        req->status = REQ_STATUS_ALLOC;
+       req->client = c;
        /* refcount needs to be set to 0 before inserting into the idr
         * so p9_tag_lookup does not accept a request that is not fully
-        * initialized. refcount_set to 2 below will mark request ready.
+        * initialized. refcount_set_release to 2 below will mark 
request ready.
         */
        refcount_set(&req->refcount, 0);
        init_waitqueue_head(&req->wq);
@@ -212,7 +213,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint 
t_size, uint r_size,
         * callback), so p9_client_cb eats the second ref there
         * as the pointer is duplicated directly by virtqueue_add_sgs()
         */
-       refcount_set(&req->refcount, 2);
+       refcount_set_release(&req->refcount, 2);

        return req;

@@ -243,12 +244,12 @@ struct p9_req_t *p9_tag_lookup(struct p9_client 
*c, u16 tag)
                /* We have to be careful with the req found under 
rcu_read_lock
                 * Thanks to SLAB_TYPESAFE_BY_RCU we can safely try to 
get the
                 * ref again without corrupting other data, then check 
again
-                * that the tag matches once we have the ref
+                * that the client and tag match once we have the ref
                 */
                if (!p9_req_try_get(req))
                        goto again;
-               if (req->tc.tag != tag) {
-                       p9_req_put(c, req);
+               if (req->client != c || req->tc.tag != tag) {
+                       p9_req_put(req->client, req);
                        goto again;
                }
        }