On 6/10/25 18:26, Song Liu wrote:
> On Tue, Jun 10, 2025 at 10:19 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
>>
>> On Fri, Jun 06, 2025 at 02:30:11PM -0700, Song Liu wrote:
>>> [...]
>>> + * Returns:
>>> + * true - if @path is updated to its parent.
>>> + * false - if @path is already the root (real root or @root).
>>> + */
>>> +bool path_walk_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;
>>> + }
>>> +
>>> + if (unlikely(IS_ROOT(path->dentry)))
>>
>> path would be updated while false is returned, which is not correct.
>
> Good catch.. How about the following:
>
> bool path_walk_parent(struct path *path, const struct path *root)
> {
> struct dentry *parent;
> bool ret = false;
>
> 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;
> ret = true;
> }
>
> if (unlikely(IS_ROOT(path->dentry)))
> return ret;
Returning true here would be the wrong semantic right? This whole thing
is only possible when some mount shadows "/". Say if you have a landlock
rule on the old "/", but then we mount a new "/" and chroot into it (via
"/.."), the landlock rule on the old "/" should not apply, but if we
change *path and return true here then this will "expose" that old "/" to
landlock.
A quick suggestion although I haven't tested anything - maybe we should do
a special case check for IS_ROOT inside the
if (unlikely(path->dentry == path->mnt->mnt_root))
? Before "path_put(path);", if IS_ROOT(p.dentry) then we just path_get(p)
and return false.
>
> parent = dget_parent(path->dentry);
> dput(path->dentry);
> path->dentry = parent;
> return true;
> }
>
> Thanks,
> Song
