On 5/30/25 19:55, Song Liu wrote:
> On Fri, May 30, 2025 at 5:20 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> [...]
>>>
>>> If we update path_parent in this patchset with choose_mountpoint(),
>>> and use it in Landlock, we will close this race condition, right?
>>
>> choose_mountpoint() is currently private, but if we add a new filesystem
>> helper, I think the right approach would be to expose follow_dotdot(),
>> updating its arguments with public types. This way the intermediates
>> mount points will not be exposed, RCU optimization will be leveraged,
>> and usage of this new helper will be simplified.
>
> I think it is easier to add a helper similar to follow_dotdot(), but not with
> nameidata. follow_dotdot() touches so many things in nameidata, so it
> is better to keep it as-is. I am having the following:
>
> /**
> * path_parent - Find the parent of path
> * @path: input and output path.
> * @root: root of the path walk, do not go beyond this root. If @root is
> * zero'ed, walk all the way to real root.
> *
> * Given a path, find the parent path. Replace @path with the parent path.
> * If we were already at the real root or a disconnected root, @path is
> * not changed.
> *
> * Returns:
> * true - if @path is updated to its parent.
> * false - if @path is already the root (real root or @root).
> */
> bool path_parent(struct path *path, const struct path *root)
> {
> struct dentry *parent;
>
> if (path_equal(path, root))
> return false;
>
> if (unlikely(path->dentry == path->mnt->mnt_root)) {
> struct path p;
>
> if (!choose_mountpoint(real_mount(path->mnt), root, &p))
> return false;
> path_put(path);
> *path = p;
> return true;
> }
>
> if (unlikely(IS_ROOT(path->dentry)))
> return false;
>
> parent = dget_parent(path->dentry);
> if (unlikely(!path_connected(path->mnt, parent))) {
> dput(parent);
> return false;
> }
> dput(path->dentry);
> path->dentry = parent;
> return true;
> }
> EXPORT_SYMBOL_GPL(path_parent);
>
> And for Landlock, it is simply:
>
> if (path_parent(&walker_path, &root))
> continue;
>
> if (unlikely(IS_ROOT(walker_path.dentry))) {
> /*
> * Stops at disconnected or real root directories.
> * Only allows access to internal filesystems
> * (e.g. nsfs, which is reachable through
> * /proc/<pid>/ns/<namespace>).
> */
> if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
> allowed_parent1 = true;
> allowed_parent2 = true;
> }
> break;
Hi, maybe I'm missing the complete picture of this code, but since
path_parent doesn't change walker_path if it returns false (e.g. if it's
disconnected, or choose_mountpoint fails), I think this `break;` should be
outside the
if (unlikely(IS_ROOT(walker_path.dentry)))
right? (Assuming this whole thing is under a `while (true)`) Otherwise we
might get stuck at the current path and get infinite loop?
> }
>
> Does this look right?
>
> Thanks,
> Song
