Use path_walk_parent() to walk a path up to its parent.
No functional changes intended.
Signed-off-by: Song Liu <song@kernel.org>
---
security/landlock/fs.c | 30 ++++++++----------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 6fee7c20f64d..e26ab8c34dd4 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -837,8 +837,8 @@ static bool is_access_to_paths_allowed(
* restriction.
*/
while (true) {
- struct dentry *parent_dentry;
const struct landlock_rule *rule;
+ struct path root = {};
/*
* If at least all accesses allowed on the destination are
@@ -895,34 +895,20 @@ static bool is_access_to_paths_allowed(
/* Stops when a rule from each layer grants access. */
if (allowed_parent1 && allowed_parent2)
break;
-jump_up:
- if (walker_path.dentry == walker_path.mnt->mnt_root) {
- if (follow_up(&walker_path)) {
- /* Ignores hidden mount points. */
- goto jump_up;
- } else {
- /*
- * Stops at the real root. Denies access
- * because not all layers have granted access.
- */
- break;
- }
- }
- if (unlikely(IS_ROOT(walker_path.dentry))) {
+
+ if (unlikely(IS_ROOT(walker_path.dentry)) &&
+ (walker_path.mnt->mnt_flags & MNT_INTERNAL)) {
/*
* Stops at disconnected 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;
- }
+ allowed_parent1 = true;
+ allowed_parent2 = true;
break;
}
- parent_dentry = dget_parent(walker_path.dentry);
- dput(walker_path.dentry);
- walker_path.dentry = parent_dentry;
+ if (path_walk_parent(&walker_path, &root))
+ break;
}
path_put(&walker_path);
--
2.47.1On Mon, Jun 16, 2025 at 11:11:13PM -0700, Song Liu wrote:
> Use path_walk_parent() to walk a path up to its parent.
>
> No functional changes intended.
Using this helper actualy fixes the issue highlighted by Al. Even if it
was reported after the first version of this patch series, the issue
should be explained in the commit message and these tags should be
added:
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Closes: https://lore.kernel.org/r/20250529231018.GP2023217@ZenIV
Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
I like this new helper but we should have a clear plan to be able to
call such helper in a RCU read-side critical section before we merge
this series. We're still waiting for Christian.
I sent a patch to fix the handling of disconnected directories for
Landlock, and it will need to be backported:
https://lore.kernel.org/all/20250701183812.3201231-1-mic@digikod.net/
Unfortunately a rebase would be needed for the path_walk_parent patch,
but I can take it in my tree if everyone is OK.
However, users of path_walk_parent() would still have to properly deal
with such disconnected directories. The Landlock fix I sent takes a
safe approach by handling disconnected directories such as only their
mount point is actually taken into account for access control decision
(see rationale in the patch series). I'm wondering if
path_walk_parent() should not help its users avoid the same issue, or at
least force them to make an explicit and informed choice.
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
> security/landlock/fs.c | 30 ++++++++----------------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 6fee7c20f64d..e26ab8c34dd4 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -837,8 +837,8 @@ static bool is_access_to_paths_allowed(
> * restriction.
> */
> while (true) {
> - struct dentry *parent_dentry;
> const struct landlock_rule *rule;
> + struct path root = {};
>
> /*
> * If at least all accesses allowed on the destination are
> @@ -895,34 +895,20 @@ static bool is_access_to_paths_allowed(
> /* Stops when a rule from each layer grants access. */
> if (allowed_parent1 && allowed_parent2)
> break;
> -jump_up:
> - if (walker_path.dentry == walker_path.mnt->mnt_root) {
> - if (follow_up(&walker_path)) {
> - /* Ignores hidden mount points. */
> - goto jump_up;
> - } else {
> - /*
> - * Stops at the real root. Denies access
> - * because not all layers have granted access.
> - */
> - break;
> - }
> - }
> - if (unlikely(IS_ROOT(walker_path.dentry))) {
> +
> + if (unlikely(IS_ROOT(walker_path.dentry)) &&
> + (walker_path.mnt->mnt_flags & MNT_INTERNAL)) {
This would not fit well with the ongoing Landlock fix because
!MNT_INTERNAL root directories should also be handled specifically, but
only if they are not mount points.
> /*
> * Stops at disconnected 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;
> - }
> + allowed_parent1 = true;
> + allowed_parent2 = true;
> break;
> }
> - parent_dentry = dget_parent(walker_path.dentry);
> - dput(walker_path.dentry);
> - walker_path.dentry = parent_dentry;
> + if (path_walk_parent(&walker_path, &root))
> + break;
> }
> path_put(&walker_path);
>
> --
> 2.47.1
>
>
Hi Mickaël,
> On Jul 3, 2025, at 11:29 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On Mon, Jun 16, 2025 at 11:11:13PM -0700, Song Liu wrote:
>> Use path_walk_parent() to walk a path up to its parent.
>>
>> No functional changes intended.
>
> Using this helper actualy fixes the issue highlighted by Al. Even if it
> was reported after the first version of this patch series, the issue
> should be explained in the commit message and these tags should be
> added:
>
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Closes: https://lore.kernel.org/r/20250529231018.GP2023217@ZenIV
> Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
>
> I like this new helper but we should have a clear plan to be able to
> call such helper in a RCU read-side critical section before we merge
> this series. We're still waiting for Christian.
>
> I sent a patch to fix the handling of disconnected directories for
> Landlock, and it will need to be backported:
> https://lore.kernel.org/all/20250701183812.3201231-1-mic@digikod.net/
> Unfortunately a rebase would be needed for the path_walk_parent patch,
> but I can take it in my tree if everyone is OK.
The fix above also touches VFS code (makes path_connected available
out of namei.c. It probably should also go through VFS tree?
Maybe you can send 1/5 and 2/5 of this set (with necessary changes)
and your fix together to VFS tree. Then, I will see how to route the
BPF side patches.
Thanks,
Song
On Thu, Jul 03, 2025 at 10:27:02PM +0000, Song Liu wrote:
> Hi Mickaël,
>
> > On Jul 3, 2025, at 11:29 AM, Mickaël Salaün <mic@digikod.net> wrote:
> >
> > On Mon, Jun 16, 2025 at 11:11:13PM -0700, Song Liu wrote:
> >> Use path_walk_parent() to walk a path up to its parent.
> >>
> >> No functional changes intended.
> >
> > Using this helper actualy fixes the issue highlighted by Al. Even if it
> > was reported after the first version of this patch series, the issue
> > should be explained in the commit message and these tags should be
> > added:
> >
> > Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> > Closes: https://lore.kernel.org/r/20250529231018.GP2023217@ZenIV
> > Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
> >
> > I like this new helper but we should have a clear plan to be able to
> > call such helper in a RCU read-side critical section before we merge
> > this series. We're still waiting for Christian.
> >
> > I sent a patch to fix the handling of disconnected directories for
> > Landlock, and it will need to be backported:
> > https://lore.kernel.org/all/20250701183812.3201231-1-mic@digikod.net/
> > Unfortunately a rebase would be needed for the path_walk_parent patch,
> > but I can take it in my tree if everyone is OK.
>
> The fix above also touches VFS code (makes path_connected available
> out of namei.c. It probably should also go through VFS tree?
>
> Maybe you can send 1/5 and 2/5 of this set (with necessary changes)
> and your fix together to VFS tree. Then, I will see how to route the
> BPF side patches.
That could work, but because it would be much more Landlock-specific
code than VFS-specific code, and there will probably be a few versions
of my fixes, I'd prefer to keep this into my tree if VFS folks are OK.
BTW, my fixes already touch the VFS subsystem a bit.
However, as pointed out in my previous email, the disconnected directory
case should be carefully considered for the path_walk_parent() users to
avoid BPF LSM programs having the same issue I'm fixing for Landlock.
The safe approaches I can think of to avoid this issue for BPF programs
while making the interface efficient (by not calling path_connected()
after each path_walk_parent() call) is to either have some kind of
iterator as Tingmao proposed, or a callback function as Neil proposed.
The callback approach looks simpler and more future-proof, but I guess
you'll have to make it compatible with the eBPF runtime. I think the
best approach would be to have a VFS API with a callback, and a BPF
helper (leveraging this VFS API) with an iterator state.
I'm aware that this disconnected directory fix might delay your patch
series, but the good news is that it's an opportunity for eBPF programs
to not have the issue I'm fixing for Landlock.
>
> Thanks,
> Song
>
>
On Fri, Jul 04, 2025 at 11:00:37AM +0200, Mickaël Salaün wrote:
> On Thu, Jul 03, 2025 at 10:27:02PM +0000, Song Liu wrote:
> > Hi Mickaël,
> >
> > > On Jul 3, 2025, at 11:29 AM, Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > On Mon, Jun 16, 2025 at 11:11:13PM -0700, Song Liu wrote:
> > >> Use path_walk_parent() to walk a path up to its parent.
> > >>
> > >> No functional changes intended.
> > >
> > > Using this helper actualy fixes the issue highlighted by Al. Even if it
> > > was reported after the first version of this patch series, the issue
> > > should be explained in the commit message and these tags should be
> > > added:
> > >
> > > Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> > > Closes: https://lore.kernel.org/r/20250529231018.GP2023217@ZenIV
> > > Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
> > >
> > > I like this new helper but we should have a clear plan to be able to
> > > call such helper in a RCU read-side critical section before we merge
> > > this series. We're still waiting for Christian.
> > >
> > > I sent a patch to fix the handling of disconnected directories for
> > > Landlock, and it will need to be backported:
> > > https://lore.kernel.org/all/20250701183812.3201231-1-mic@digikod.net/
> > > Unfortunately a rebase would be needed for the path_walk_parent patch,
> > > but I can take it in my tree if everyone is OK.
> >
> > The fix above also touches VFS code (makes path_connected available
> > out of namei.c. It probably should also go through VFS tree?
> >
> > Maybe you can send 1/5 and 2/5 of this set (with necessary changes)
> > and your fix together to VFS tree. Then, I will see how to route the
> > BPF side patches.
>
> That could work, but because it would be much more Landlock-specific
> code than VFS-specific code, and there will probably be a few versions
> of my fixes, I'd prefer to keep this into my tree if VFS folks are OK.
> BTW, my fixes already touch the VFS subsystem a bit.
Under specific circumstances we will accept very minor changes to VFS
code to go through selected other trees depending on the amount of trust
between the respective trees. Afaict, your series just exports a
function. I'll take a look at it.
> On Jul 4, 2025, at 2:00 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On Thu, Jul 03, 2025 at 10:27:02PM +0000, Song Liu wrote:
>> Hi Mickaël,
>>
>>> On Jul 3, 2025, at 11:29 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>> On Mon, Jun 16, 2025 at 11:11:13PM -0700, Song Liu wrote:
>>>> Use path_walk_parent() to walk a path up to its parent.
>>>>
>>>> No functional changes intended.
>>>
>>> Using this helper actualy fixes the issue highlighted by Al. Even if it
>>> was reported after the first version of this patch series, the issue
>>> should be explained in the commit message and these tags should be
>>> added:
>>>
>>> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
>>> Closes: https://lore.kernel.org/r/20250529231018.GP2023217@ZenIV
>>> Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
>>>
>>> I like this new helper but we should have a clear plan to be able to
>>> call such helper in a RCU read-side critical section before we merge
>>> this series. We're still waiting for Christian.
>>>
>>> I sent a patch to fix the handling of disconnected directories for
>>> Landlock, and it will need to be backported:
>>> https://lore.kernel.org/all/20250701183812.3201231-1-mic@digikod.net/
>>> Unfortunately a rebase would be needed for the path_walk_parent patch,
>>> but I can take it in my tree if everyone is OK.
>>
>> The fix above also touches VFS code (makes path_connected available
>> out of namei.c. It probably should also go through VFS tree?
>>
>> Maybe you can send 1/5 and 2/5 of this set (with necessary changes)
>> and your fix together to VFS tree. Then, I will see how to route the
>> BPF side patches.
>
> That could work, but because it would be much more Landlock-specific
> code than VFS-specific code, and there will probably be a few versions
> of my fixes, I'd prefer to keep this into my tree if VFS folks are OK.
> BTW, my fixes already touch the VFS subsystem a bit.
>
> However, as pointed out in my previous email, the disconnected directory
> case should be carefully considered for the path_walk_parent() users to
> avoid BPF LSM programs having the same issue I'm fixing for Landlock.
> The safe approaches I can think of to avoid this issue for BPF programs
> while making the interface efficient (by not calling path_connected()
> after each path_walk_parent() call) is to either have some kind of
> iterator as Tingmao proposed, or a callback function as Neil proposed.
> The callback approach looks simpler and more future-proof, but I guess
> you'll have to make it compatible with the eBPF runtime. I think the
> best approach would be to have a VFS API with a callback, and a BPF
> helper (leveraging this VFS API) with an iterator state.
Since we are proposing an open-coded BPF iterator. Having a real
callback, which is no longer an open coded iterator, requires more
work. At the moment, it is easier to just add a path_connected call
in bpf_iter_path_next().
Thanks,
Song
© 2016 - 2025 Red Hat, Inc.