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

[Yonghong Song <yonghong.song@linux.dev>]

On 7/6/25 4:54 PM, Song Liu wrote:
>
>> On Jul 4, 2025, at 10:40 AM, Yonghong Song <yonghong.song@linux.dev> 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().

Looks like __path_walk_parent() works for the root of mounted fs.
If this is the case, the implementation is correct. It could be
good to add some comments to clarify.

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