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

[Dominique Martinet <asmadeus@codewreck.org>]

Jérémy Jean wrote on Thu, Oct 29, 2026 at 11:56:03AM +0100:
> I am not so familiar with that code base and I have a hard time
> deciding the actual impact of the lock.

Each request is already taking the lock twice (when allocating/freeing
the tag in the idr), so I'm honestly not sure how much adding a third
locking call will change in practice without testing (and if I wanted to
be pedantic I'd say I can't even test this properly as I don't have any
numa hardware that'd be impacted more strongly by this)...

One of my worries was that some transports call this lookup function in
a irq (e.g. rdma depending on hardware), but that's another thing I
can't test /shrug

> 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.

That's pretty much what I had in mind, yes.
I wasn't thinking of the _aquire/_release refcount variants but after
checking I agree they're needed, so I'm tempted to say it's another
argument in favor of stopping SLAB_TYPESAFE_BY_RCU...!

On the opposite side if we keep your current v1 then I'd get rid of all
references to SLAB_TYPESAFE_BY_RCU - it's no longer needed with the
lock, and the allocation no longer needs to split refcount init and set
either.

Anyway, please give me a bit of time to run some quick benchmark with
and without lock even if it likely won't mean much, and we can iterate
from there; I don't think this warrants a v2 patch at this point.

(It will be at best a few days, I'm not expecting to take this in for
7.4)

Thanks,
-- 
Dominique Martinet | Asmadeus