On Wed, 08 Jan 2025, Christopher Bii wrote: > It all depends what the desired behavior is. When a rootdir is set, do > we want any exports to possibly fall outside the rootdir path? If no, it > is beneficial to use this approach since resolving paths within chrooted > env will fail if the path does not exist within the chroot. Allowing for > a "sandboxing" mechanism of sort. But with this approach you have > proposed, the issue might arise where someone with access to an nfs > export can create a symlink to say "/". In that scenario the kernel will > end up exporting the system root. Which can be bad. You could of course > do as you have stated and check if it is within the boundaries of the > rootdir, but it would result to the same thing. I think the chrooted > thread that can be repurposed is the best approach here. > > The majority of the patch is simplifying what I believed to have been an > unmaintainable approach of spawning tasks within the worker thread is > created to run chrooted. I'm not at all familiar with the "work queue" and nfs_path.c and chroot code so I cannot comment on whether the current approach is unmaintainable. Maybe it is. But all these details need to appear in the patches that you send. Don't just tell us what you are changing, tell us why. Say that is is unmaintainable and give some reason for us to believe you - a reason that we can check by looking at the code, and then see that your patch makes it unmaintainable. And don't just say "there is a security vulnerability". Explain that it is because the that real_path happens before that root is prepended. All of that needs to be clear in the patches so that we can review them properly now, and so that when we look back at them in 2 years because some issue arose, we can remind our self what they were for and understand how that all relates to the new issue. Maybe the code in the patches is already fine. But the explanation in the patches also needs to be good before we can approve them. Thanks! NeilBrown
