Messages in this thread | | | Date | Wed, 30 Nov 2022 22:34:56 +0900 | From | asmadeus@codewrec ... | Subject | Re: [PATCH] 9p: fix crash when transaction killed |
| |
Schspa Shi wrote on Wed, Nov 30, 2022 at 09:15:12PM +0800: > >> If the req was newly alloced(It was at a new page), refcount maybe not > >> 0, there will be problem in this case. It seems we can't relay on this. > >> > >> We need to set the refcount to zero before add it to idr in p9_tag_alloc. > > > > Hmm, if it's reused then it's zero by definition, but if it's a new > > allocation (uninitialized) then anything goes; that lookup could find > > and increase it before the refcount_set, and we'd have an off by one > > leading to use after free. Good catch! > > > > Initializing it to zero will lead to the client busy-looping until after > > the refcount is properly set, which should work. > > Why? It looks no different from the previous process here. Initializing > it to zero should makes no difference.
I do not understand this remark. If this is a freed request it will be zero, because we freed the request as the refcount hit zero, but if it's a newly allocated request then the memory is uninitalized, and the lookup can get anything.
In that case we want refcount to be zero to have the check in p9_tag_lookup to not use the request until we set the refcount to 2.
> > Setting refcount early might have us use an re-used req before the tag > > has been changed so that one cannot move. > > > > Could you test with just that changed if syzbot still reproduces this > > bug? (perhaps add a comment if you send this) > > > > I have upload a new v2 change for this. But I can't easily reproduce > this problem.
Ah, I read that v2 as you actually ran some tests with this, sorry for the misuderstanding.
Well, it's a fix anyway, so it cannot hurt to apply... -- Dominique
| |