Re: [PATCH v2 bpf-next 2/4] landlock: Use path_walk_parent()

["Mickaël Salaün" <mic@digikod.net>]

Landlock tests with hostfs fail:

ok 126 layout3_fs.hostfs.tag_inode_file
#  RUN           layout3_fs.hostfs.release_inodes ...
# fs_test.c:5555:release_inodes:Expected EACCES (13) == test_open(TMP_DIR, O_RDONLY) (0)

This specific test checks that an access to a (denied) mount point over
an allowed directory is indeed denied.

It's not clear to me the origin of the issue, but it seems to be related
to choose_mountpoint().

You can run these tests with `check-linux.sh build kselftest` from
https://github.com/landlock-lsm/landlock-test-tools

Just in case, please always run clang-format -i security/landlock/*.[ch]


On Mon, Jun 02, 2025 at 11:59:18PM -0700, Song Liu wrote:
> 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 | 31 ++++++++++---------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 6fee7c20f64d..3adac544dc9e 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,23 @@ 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 (path_walk_parent(&walker_path, &root))
> +			continue;

It would be better to avoid a "continue" statement but to just use an if
block.

> +
>  		if (unlikely(IS_ROOT(walker_path.dentry))) {
>  			/*
> -			 * Stops at disconnected root directories.  Only allows
> -			 * access to internal filesystems (e.g. nsfs, which is
> -			 * reachable through /proc/<pid>/ns/<namespace>).
> +			 * 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;
>  		}
> -		parent_dentry = dget_parent(walker_path.dentry);
> -		dput(walker_path.dentry);
> -		walker_path.dentry = parent_dentry;
> +		break;
>  	}
>  	path_put(&walker_path);
>  
> -- 
> 2.47.1
> 
>