| Message ID | 20260718043750.7-1-bbnpreetsingh@gmail.com |
|---|---|
| Headers | show |
| Series | closefrom/close_range: protect every pseudo fd, then drop one | expand |
Hi Richard, Mark, A gentle ping on this series and the close_range v2 it stacks on (which Richard okayed on-list on 16 Jul). I noticed pseudo master has moved since (now ca47829, with the memory-handling and openat2 changes). I've checked that both series still apply cleanly on top of current master, so no rebase is needed. Happy to rebase and resend anyway if that's more convenient. Thanks, Baban
Hi Baban, On Tue, 2026-07-28 at 05:08 +0000, Babanpreet Singh wrote: > A gentle ping on this series and the close_range v2 it stacks on > (which Richard okayed on-list on 16 Jul). > > I noticed pseudo master has moved since (now ca47829, with the > memory-handling and openat2 changes). I've checked that both series > still apply cleanly on top of current master, so no rebase is needed. > Happy to rebase and resend anyway if that's more convenient. Sorry, we haven't forgotten about this, we even did discuss it briefly yesterday! Mark has struggled for time and I've been juggling a few other issues. There were a few fixes ahead of yours in the queue, I have those merged as you mentioned. We then have yours and the logging fixes along with some tests Mark worked on which are next. I've pushed your changes onto master-next where there were some minor conflicts with other changes. I'll run some wider tests on the resulting branch. Cheers, Richard
On Tue, 2026-07-28 at 09:14 +0100, Richard Purdie via lists.yoctoproject.org wrote: > Hi Baban, > > On Tue, 2026-07-28 at 05:08 +0000, Babanpreet Singh wrote: > > A gentle ping on this series and the close_range v2 it stacks on > > (which Richard okayed on-list on 16 Jul). > > > > I noticed pseudo master has moved since (now ca47829, with the > > memory-handling and openat2 changes). I've checked that both series > > still apply cleanly on top of current master, so no rebase is > > needed. > > Happy to rebase and resend anyway if that's more convenient. > > Sorry, we haven't forgotten about this, we even did discuss it > briefly > yesterday! Mark has struggled for time and I've been juggling a few > other issues. > > There were a few fixes ahead of yours in the queue, I have those > merged > as you mentioned. We then have yours and the logging fixes along with > some tests Mark worked on which are next. > > I've pushed your changes onto master-next where there were some minor > conflicts with other changes. I'll run some wider tests on the > resulting branch. Further testing showed the problems came back so I think I messed up the testing somewhere along the way, sorry :/. I'm pretty sure the close_range() implementation is the point things break so there must be some subtle issue in that code... I'd suggest we reorder the patches and get the skip fds and drop the prefix fd patches merged/queued, then come back to how to get close_range working. I still thing one common op codepath for both cases makes the most sense. Cheers, Richard
On Tue, 2026-07-28 at 16:48 +0100, Richard Purdie via lists.yoctoproject.org wrote: > On Tue, 2026-07-28 at 09:14 +0100, Richard Purdie via > lists.yoctoproject.org wrote: > > Hi Baban, > > > > On Tue, 2026-07-28 at 05:08 +0000, Babanpreet Singh wrote: > > > A gentle ping on this series and the close_range v2 it stacks on > > > (which Richard okayed on-list on 16 Jul). > > > > > > I noticed pseudo master has moved since (now ca47829, with the > > > memory-handling and openat2 changes). I've checked that both series > > > still apply cleanly on top of current master, so no rebase is > > > needed. > > > Happy to rebase and resend anyway if that's more convenient. > > > > Sorry, we haven't forgotten about this, we even did discuss it > > briefly > > yesterday! Mark has struggled for time and I've been juggling a few > > other issues. > > > > There were a few fixes ahead of yours in the queue, I have those > > merged > > as you mentioned. We then have yours and the logging fixes along with > > some tests Mark worked on which are next. > > > > I've pushed your changes onto master-next where there were some minor > > conflicts with other changes. I'll run some wider tests on the > > resulting branch. > > Further testing showed the problems came back so I think I messed up > the testing somewhere along the way, sorry :/. I'm pretty sure the > close_range() implementation is the point things break so there must be > some subtle issue in that code... > > I'd suggest we reorder the patches and get the skip fds and drop the > prefix fd patches merged/queued, then come back to how to get > close_range working. I still thing one common op codepath for both > cases makes the most sense. I think the missing piece is this: diff --git a/pseudo_client.c b/pseudo_client.c index ca37a80..a1c7052 100644 --- a/pseudo_client.c +++ b/pseudo_client.c @@ -1633,7 +1633,7 @@ pseudo_client_op(pseudo_op_t op, int access, int fd, int dirfd, const char *path } } - if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op != OP_CLOSEFROM && op != OP_DUP + if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op != OP_CLOSEFROM && op != OP_CLOSE_RANGE && op != OP_DUP && pseudo_client_ignore_path_chroot(path, 0)) { if (op == OP_OPEN) { /* Sanitise the path to have no trailing slash as this is convention in the database */ Cheers, Richard
On Tue, 2026-07-28 at 16:59 +0100, Richard Purdie wrote: > I think the missing piece is this: > > - if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op != OP_CLOSEFROM && op != OP_DUP > + if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op != OP_CLOSEFROM && op != OP_CLOSE_RANGE && op != OP_DUP Sorry for the breakage, and thanks for chasing it down. I can reproduce it both ways on master-next at 6d65d74, with a probe that opens an fd and calls close_range(fd, ~0U, 0): no PSEUDO_*_PATHS set OK PSEUDO_IGNORE_PATHS covering the fd's path SIGSEGV PSEUDO_INCLUDE_PATHS set, path not in it SIGSEGV same env, closefrom() instead OK all of the above, plus your line OK For OP_CLOSE_RANGE the path comes from fd_path(lowfd), the ignore check hits it, and pseudo_client_op() returns early with result still 0, which close_range.c then dereferences. Two things before I respin. Do you want the three fd-protect patches resent, or will you carry the master-next commits since you have already resolved the conflicts? And the ignore-path line: fold it into the close_range patch with a Suggested-by, or keep it as your own commit ahead of it? Either works for me. Thanks, Baban
On Wed, 2026-07-29 at 05:08 +0000, Babanpreet Singh wrote: > On Tue, 2026-07-28 at 16:59 +0100, Richard Purdie wrote: > > I think the missing piece is this: > > > > - if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op != OP_CLOSEFROM && op != OP_DUP > > + if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op != OP_CLOSEFROM && op != OP_CLOSE_RANGE && op != OP_DUP > > Sorry for the breakage, and thanks for chasing it down. > > I can reproduce it both ways on master-next at 6d65d74, with a probe that > opens an fd and calls close_range(fd, ~0U, 0): > > no PSEUDO_*_PATHS set OK > PSEUDO_IGNORE_PATHS covering the fd's path SIGSEGV > PSEUDO_INCLUDE_PATHS set, path not in it SIGSEGV > same env, closefrom() instead OK > all of the above, plus your line OK > > For OP_CLOSE_RANGE the path comes from fd_path(lowfd), the ignore check > hits it, and pseudo_client_op() returns early with result still 0, which > close_range.c then dereferences. > > Two things before I respin. Do you want the three fd-protect patches > resent, or will you carry the master-next commits since you have already > resolved the conflicts? And the ignore-path line: fold it into the > close_range patch with a Suggested-by, or keep it as your own commit ahead > of it? Either works for me. You can fold the above piece in with a suggested by, it doesn't make sense in a commit on its own. I can take the other fd-protect patches if you're ok with that change, I need to update the commit messages. Whilst most builds are testing cleanly, we are seeing these two failures: https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/4403 https://autobuilder.yoctoproject.org/valkyrie/#/builders/23/builds/4466 which look like a pseudo problem, and that is with the current pseudo master-next. I think there is still some issue remaining somewhere unfortunately. I'm afraid I don't know what it is. Cheers, Richard
On Wed, 2026-07-29 at 07:19 +0100, Richard Purdie via lists.yoctoproject.org wrote: > On Wed, 2026-07-29 at 05:08 +0000, Babanpreet Singh wrote: > > On Tue, 2026-07-28 at 16:59 +0100, Richard Purdie wrote: > > > I think the missing piece is this: > > > > > > - if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op > > > != OP_CLOSEFROM && op != OP_DUP > > > + if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op > > > != OP_CLOSEFROM && op != OP_CLOSE_RANGE && op != OP_DUP > > > > Sorry for the breakage, and thanks for chasing it down. > > > > I can reproduce it both ways on master-next at 6d65d74, with a > > probe that > > opens an fd and calls close_range(fd, ~0U, 0): > > > > no PSEUDO_*_PATHS set OK > > PSEUDO_IGNORE_PATHS covering the fd's path SIGSEGV > > PSEUDO_INCLUDE_PATHS set, path not in it SIGSEGV > > same env, closefrom() instead OK > > all of the above, plus your line OK > > > > For OP_CLOSE_RANGE the path comes from fd_path(lowfd), the ignore > > check > > hits it, and pseudo_client_op() returns early with result still 0, > > which > > close_range.c then dereferences. > > > > Two things before I respin. Do you want the three fd-protect > > patches > > resent, or will you carry the master-next commits since you have > > already > > resolved the conflicts? And the ignore-path line: fold it into the > > close_range patch with a Suggested-by, or keep it as your own > > commit ahead > > of it? Either works for me. > > You can fold the above piece in with a suggested by, it doesn't make > sense in a commit on its own. I can take the other fd-protect patches > if you're ok with that change, I need to update the commit messages. > > Whilst most builds are testing cleanly, we are seeing these two > failures: > > https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/4403 > https://autobuilder.yoctoproject.org/valkyrie/#/builders/23/builds/4466 > > which look like a pseudo problem, and that is with the current pseudo > master-next. I think there is still some issue remaining somewhere > unfortunately. I'm afraid I don't know what it is. One of those doesn't look to be pseudo, the arm one definitely does though. It looks like it may be arch specific. Cheers, Richard
On Wed, 2026-07-29 at 09:14 +0100, Richard Purdie via lists.yoctoproject.org wrote: > On Wed, 2026-07-29 at 07:19 +0100, Richard Purdie via > lists.yoctoproject.org wrote: > > On Wed, 2026-07-29 at 05:08 +0000, Babanpreet Singh wrote: > > > On Tue, 2026-07-28 at 16:59 +0100, Richard Purdie wrote: > > > > I think the missing piece is this: > > > > > > > > - if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op > > > > != OP_CLOSEFROM && op != OP_DUP > > > > + if (op != OP_CHROOT && op != OP_CHDIR && op != OP_CLOSE && op > > > > != OP_CLOSEFROM && op != OP_CLOSE_RANGE && op != OP_DUP > > > > > > Sorry for the breakage, and thanks for chasing it down. > > > > > > I can reproduce it both ways on master-next at 6d65d74, with a > > > probe that > > > opens an fd and calls close_range(fd, ~0U, 0): > > > > > > no PSEUDO_*_PATHS set OK > > > PSEUDO_IGNORE_PATHS covering the fd's path SIGSEGV > > > PSEUDO_INCLUDE_PATHS set, path not in it SIGSEGV > > > same env, closefrom() instead OK > > > all of the above, plus your line OK > > > > > > For OP_CLOSE_RANGE the path comes from fd_path(lowfd), the ignore > > > check > > > hits it, and pseudo_client_op() returns early with result still 0, > > > which > > > close_range.c then dereferences. > > > > > > Two things before I respin. Do you want the three fd-protect > > > patches > > > resent, or will you carry the master-next commits since you have > > > already > > > resolved the conflicts? And the ignore-path line: fold it into the > > > close_range patch with a Suggested-by, or keep it as your own > > > commit ahead > > > of it? Either works for me. > > > > You can fold the above piece in with a suggested by, it doesn't make > > sense in a commit on its own. I can take the other fd-protect patches > > if you're ok with that change, I need to update the commit messages. > > > > Whilst most builds are testing cleanly, we are seeing these two > > failures: > > > > https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/4403 > > https://autobuilder.yoctoproject.org/valkyrie/#/builders/23/builds/4466 > > > > which look like a pseudo problem, and that is with the current pseudo > > master-next. I think there is still some issue remaining somewhere > > unfortunately. I'm afraid I don't know what it is. > > One of those doesn't look to be pseudo, the arm one definitely does > though. It looks like it may be arch specific. It took hours but I was able to work out this was a missing __open64_2 wrapper so I've sent a patch for that. That is unrelated to the close_range patch but somehow implementing that triggers a different codepath in systemd which then broke. I'll retest things and check there isn't anything else. Cheers, Richard
Hi Richard, Yes, that's fine by me, thanks for folding it in and for redoing the commit messages. I see the fd-protect patches on master with 1.9.10 on top. I've also read through the close_range respin on master-next. The clamp and the OP_CLOSE_RANGE exemption match what I reproduced here. Sorry the __open64_2 hunt cost you an afternoon. Happy to run the close_range probe and the OE reproducer against current master-next and report back if another data point would help. I'll add a note to bug 16339 once the close_range side lands. Cheers, Baban
Hi Baban, On Thu, 2026-07-30 at 05:07 +0000, Babanpreet Singh wrote: > Yes, that's fine by me, thanks for folding it in and for redoing the > commit messages. I see the fd-protect patches on master with 1.9.10 on > top. > > I've also read through the close_range respin on master-next. The clamp > and the OP_CLOSE_RANGE exemption match what I reproduced here. > > Sorry the __open64_2 hunt cost you an afternoon. Happy to run the > close_range probe and the OE reproducer against current master-next and > report back if another data point would help. > > I'll add a note to bug 16339 once the close_range side lands. We're close, we just need a new version of the patch which makes close_range and close_from use the same OP call within pseudo. Do you want to write that or should I? Cheers, Richard
This is the follow-up from the close_range v2 thread [1], in the order Richard suggested and Mark agreed with there: protect the whole set of descriptors first, then remove pseudo_prefix_dir_fd on top as a separately revertable commit. This has been built on top of v2. While writing a probe to demonstrate the missing protection I noticed something: the sweeps can close a descriptor that is already on the protect list. The startfd computation uses >, so a protected descriptor exactly equal to the running startfd is stepped around by neither the by-hand loop nor the kernel sweep. My understanding is that the default layout reliably produces that case: pseudo moves its own descriptors up with F_DUPFD from PSEUDO_MIN_FD, so the local state fd and connect_fd end up on consecutive numbers and connect_fd lands exactly on the computed startfd. If I am reading the history right, this means every closefrom(3) under pseudo has quietly closed the server connection since 21ff2fb in 2021, and it went unnoticed because the client transparently reconnects on the next operation. Patch 1 addresses that. Patches 2 and 3 are the protection and removal we discussed on the thread. To convince myself I tested with a probe that locates pseudo's descriptors via /proc/self/fd, sweeps with close_range(3, ~0U, 0) or closefrom(3), and checks what survived. On the v2 baseline: pwd lock fd (lckpwdf held) : fd 3 closed by sweep PSEUDO_PREFIX dir fd : fd 20 closed by sweep server socket fd : fd 22 closed by sweep After patch 1 the socket survives, after patch 2 everything survives, and after patch 3 the prefix descriptor no longer exists to need protecting. [1] https://lore.kernel.org/yocto-patches/20260716055633.7-1-bbnpreetsingh@gmail.com/ Babanpreet Singh (3): pseudo_client: step fully past pseudo's own fds when computing startfd pseudo_client: step around all of pseudo's own fds in closefrom/close_range pseudo_client: remove the unused pseudo_prefix_dir_fd doc/program_flow | 1 - pseudo.h | 1 + pseudo_client.c | 90 ++++++++++++++---------------------------------- pseudo_client.h | 1 - 4 files changed, 27 insertions(+), 66 deletions(-)