Messages in this thread | | | From | Schspa Shi <> | Subject | Re: [PATCH] 9p: fix crash when transaction killed | Date | Thu, 01 Dec 2022 10:26:12 +0800 |
| |
asmadeus@codewreck.org writes:
> 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.
Here is my misunderstanding. I thought you meant that there would be a loop on the client side to wait for the refcount to become a non-zero value. Actually, there is no such loop.
> > 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...
-- BRs Schspa Shi
| |