Repository navigation
Backport #104 to 6.18-lts: 9p/xen: harden the session save and restore path - #111
Open
edera-rebase-app[bot] wants to merge 5 commits into
Open
edera-rebase-app[bot] wants to merge 5 commits into
edera-rebase-app[bot] wants to merge 5 commits into
Conversation
xen_9pfs_front_ctl() answers a backend Rlerror with -(int)ecode, and the ecode is the backend's to choose. An ecode of 0 came back as 0 and any ecode above INT_MAX came back positive, and the function's contract is "reply length or negative errno", so a caller took either for a good reply of that length. xen_9pfs_front_suspend() then read total and next from beyond the short error reply and computed the chunk length from the bogus "reply length": an ecode of 0xfff00000 had it copy about 1 MiB of uninitialised buffer into a session chunk and later hand it to the backend in Tsession_restore, and 0x80000010 had it memcpy() some 2 GiB past the reply buffer. Take only a plausible errno from an Rlerror, 1..MAX_ERRNO, and answer anything else with -EPROTO, so a control request can only ever return a negative errno or a reply that carries the id and body it asked for. Signed-off-by: Ariadne Conill <[email protected]> (cherry picked from commit 47a8726)
The Connected handler replays the fids saved at suspend from xenwatch, after do_suspend() has returned and processes run again. It checked priv->client once and then read priv->client->msize for every chunk, each of which waits up to 5 s on the backend. Meanwhile a umount or a fresh mount blocked on the still-disconnected frontend can be killed: p9_client_destroy() then runs p9_xen_close(), which clears priv->client with no lock the replay holds, and frees the client. The next chunk dereferences NULL, or the freed client, and xenwatch oopses, which stops all xenbus processing for the guest. Record the owning client and its msize when the session is saved, with userspace frozen and the client known to be alive, and have the replay use those. It compares priv->client with the owner by address only, before starting and before each chunk, and gives up if they differ: a client destroyed in between does not want its fids back, and nothing is dereferenced that could have gone away. Signed-off-by: Ariadne Conill <[email protected]> (cherry picked from commit 1aed70c)
xen_9pfs_front_suspend() freed the session saved by the previous suspend first thing, before checking whether the frontend is connected. A guest suspended again before the backend had reconnected it, which is what happens when a restore is retried or a saved domain is forked more than once, thus lost the fids it was carrying, and the eventual restore reset the session instead of replaying them. Only free the old session once there is a connected client whose fids replace it. A frontend the backend has not reconnected since the last suspend has nothing newer to save, and the Connected handler frees the session once it has been replayed, so what is kept is never stale. Signed-off-by: Ariadne Conill <[email protected]> (cherry picked from commit 373e994)
… page's xen_9pfs_front_free_rings() took the data pages' ring_order and grant refs from the intf page, which is shared with the backend and writable by it. Upstream only freed rings from remove(), with the backend gone; since the frontend reconnects after a restore it also frees them from xen_9pfs_front_disconnect() on every resume, while a backend that still serves the old rings may be running. A backend that rewrote ref[] had the frontend end foreign access on grant refs that belong to other devices, and one that rewrote ring_order had it walk past the array or free the wrong number of pages. Keep the refs in the ring as they are granted and use those, with the order the frontend chose, to tear the ring down. Signed-off-by: Ariadne Conill <[email protected]> (cherry picked from commit 88cb2b5)
xen_9pfs_front_ctl() stores the reply buffer and its capacity, then sets ctl_pending with a plain WRITE_ONCE(), and the response worker tests the flag with READ_ONCE() before copying into the buffer. Nothing orders the two, so on a weakly ordered machine the worker can see ctl_pending set while ctl_reply still names the previous call's buffer, a stack array or kvmalloc()ed memory that has been freed, and copy the reply into that. Publish the buffer with smp_store_release() and test the flag with smp_load_acquire(). Signed-off-by: Ariadne Conill <[email protected]> (cherry picked from commit 193a379)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #104 from
edera/mainlinetoedera/6.18-lts(492d0ef30003), prepared by Claude. Run log.Warning
Merge with Rebase and merge, so
edera/6.18-ltsstays linear.Independent checks
Builds: success (x86_64 and arm64 vmlinux with the Edera features built in).
492d0ef30003403c8cc309a9aeab5323e9e8 9p/xen: end foreign access on the grant refs we granted, not the intf page's88cb2b50c54f 9p/xen: end foreign access on the grant refs we granted, not the intf page'sAdapted commits (source +/- lines vs. backport)
Claude's report
Claude left no report (backport step: success).