Re: [PATCH v5 bpf-next 1/5] namei: Introduce new helper function path_walk_parent()

[Song Liu <songliubraving@xxxxxxxx>]

> On Jul 4, 2025, at 10:40 AM, Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
[...]
>> +static struct dentry *__path_walk_parent(struct path *path, const struct path *root, int flags)
>>  {
>> - struct dentry *parent;
>> -
>> - if (path_equal(&nd->path, &nd->root))
>> + if (path_equal(path, root))
>>   goto in_root;
>> - if (unlikely(nd->path.dentry == nd->path.mnt->mnt_root)) {
>> - struct path path;
>> + if (unlikely(path->dentry == path->mnt->mnt_root)) {
>> + struct path new_path;
>>  - if (!choose_mountpoint(real_mount(nd->path.mnt),
>> -       &nd->root, &path))
>> + if (!choose_mountpoint(real_mount(path->mnt),
>> +       root, &new_path))
>>   goto in_root;
>> - path_put(&nd->path);
>> - nd->path = path;
>> - nd->inode = path.dentry->d_inode;
>> - if (unlikely(nd->flags & LOOKUP_NO_XDEV))
>> + path_put(path);
>> + *path = new_path;
>> + if (unlikely(flags & LOOKUP_NO_XDEV))
>>   return ERR_PTR(-EXDEV);
>>   }
>>   /* rare case of legitimate dget_parent()... */
>> - parent = dget_parent(nd->path.dentry);
>> + return dget_parent(path->dentry);
> 
> I have some confusion with this patch when crossing mount boundary.
> 
> In d_path.c, we have
> 
> static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
>                          const struct path *root, struct prepend_buffer *p)
> {
>        while (dentry != root->dentry || &mnt->mnt != root->mnt) {
>                const struct dentry *parent = READ_ONCE(dentry->d_parent);
> 
>                if (dentry == mnt->mnt.mnt_root) {
>                        struct mount *m = READ_ONCE(mnt->mnt_parent);
>                        struct mnt_namespace *mnt_ns;
> 
>                        if (likely(mnt != m)) {
>                                dentry = READ_ONCE(mnt->mnt_mountpoint);
>                                mnt = m;
>                                continue;
>                        }
>                        /* Global root */
>                        mnt_ns = READ_ONCE(mnt->mnt_ns);
>                        /* open-coded is_mounted() to use local mnt_ns */
>                        if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
>                                return 1;       // absolute root
>                        else
>                                return 2;       // detached or not attached yet
>                }
> 
>                if (unlikely(dentry == parent))
>                        /* Escaped? */
>                        return 3;
> 
>                prefetch(parent);
>                if (!prepend_name(p, &dentry->d_name))
>                        break;
>                dentry = parent;
>        }
>        return 0;
> }
> 
> At the mount boundary and not at root mount, the code has
> dentry = READ_ONCE(mnt->mnt_mountpoint);
> mnt = m; /* 'mnt' will be parent mount */
> continue;
> 
> After that, we have
> const struct dentry *parent = READ_ONCE(dentry->d_parent);
> if (dentry == mnt->mnt.mnt_root) {
> /* assume this is false */
> }
> ...
> prefetch(parent);
>        if (!prepend_name(p, &dentry->d_name))
>                break;
>        dentry = parent;
> 
> So the prepend_name(p, &dentry->d_name) is actually from mnt->mnt_mountpoint.

I am not quite following the question. In the code below:

               if (dentry == mnt->mnt.mnt_root) {
                       struct mount *m = READ_ONCE(mnt->mnt_parent);
                       struct mnt_namespace *mnt_ns;

                       if (likely(mnt != m)) {
                               dentry = READ_ONCE(mnt->mnt_mountpoint);
                               mnt = m;
                               continue;
/* We either continue, here */

                       }
                       /* Global root */
                       mnt_ns = READ_ONCE(mnt->mnt_ns);
                       /* open-coded is_mounted() to use local mnt_ns */
                       if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
                               return 1;       // absolute root
                       else
                               return 2;       // detached or not attached yet
/* Or return here */
               }

So we will not hit prepend_name(). Does this answer the 
question?

> 
> In your above code, maybe we should return path->dentry in the below if statement?
> 
>        if (unlikely(path->dentry == path->mnt->mnt_root)) {
>                struct path new_path;
> 
>                if (!choose_mountpoint(real_mount(path->mnt),
>                                       root, &new_path))
>                        goto in_root;
>                path_put(path);
>                *path = new_path;
>                if (unlikely(flags & LOOKUP_NO_XDEV))
>                        return ERR_PTR(-EXDEV);
> + return path->dentry;
>        }
>        /* rare case of legitimate dget_parent()... */
>        return dget_parent(path->dentry);
> 
> Also, could you add some selftests cross mount points? This will
> have more coverages with __path_walk_parent().

Yeah, I will try to add more tests in the next revision. 

Thanks,
Song