Re: [RFC PATCH 3/3] Restart pathwalk on rename seqcount change

[Tingmao Wang <m@maowtm.org>]

On 6/4/25 01:45, Tingmao Wang wrote:
> This fixes the issue mentioned in the previous patch, by essentially
> having two "modes" for the pathwalk code - in the pathwalk_ref == false
> case we don't take references and just inspect `d_parent` (unless we have
> to `follow_up`).  In the pathwalk_ref == true case, this is the same as
> before.
> 
> When we detect any renames during a pathwalk_ref == false walk, we restart
> with pathwalk_ref == true, re-initializing the layer masks.  I'm not sure
> if this is completely correct in regards to is_dom_check - but seems to
> work for now.  I can revisit this later.
> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>  security/landlock/fs.c | 109 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 98 insertions(+), 11 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 923737412cfa..6dff5fb6b181 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -771,6 +771,9 @@ static bool is_access_to_paths_allowed(
>  		_layer_masks_child2[LANDLOCK_NUM_ACCESS_FS];
>  	layer_mask_t(*layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS] = NULL,
>  	(*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL;
> +	unsigned int rename_seqcount;
> +	bool pathwalk_ref = false;
> +	const struct landlock_rule *rule;
>  
>  	if (!access_request_parent1 && !access_request_parent2)
>  		return true;
> @@ -811,6 +814,7 @@ static bool is_access_to_paths_allowed(
>  
>  	rcu_read_lock();
>  
> +restart_pathwalk:
>  	if (unlikely(dentry_child1)) {
>  		landlock_unmask_layers(
>  			find_rule_rcu(domain, dentry_child1),
> @@ -833,13 +837,32 @@ static bool is_access_to_paths_allowed(
>  	}
>  
>  	walker_path = *path;
> +
> +	/*
> +	 * Attempt to do a pathwalk without taking dentry references first,
> +	 * but if any rename happens while we are doing this, give up and do a
> +	 * walk with dget_parent instead.  See comments in
> +	 * collect_domain_accesses().
> +	 */
> +
> +	if (!pathwalk_ref) {
> +		rename_seqcount = read_seqbegin(&rename_lock);
> +		if (rename_seqcount % 2 == 1) {
> +			pathwalk_ref = true;
> +			path_get(&walker_path);
> +		}
> +	} else {
> +		path_get(&walker_path);
> +	}
> +
> +	rule = find_rule_rcu(domain, walker_path.dentry);
> +
>  	/*
>  	 * We need to walk through all the hierarchy to not miss any relevant
>  	 * restriction.
>  	 */
>  	while (true) {
>  		struct dentry *parent_dentry;
> -		const struct landlock_rule *rule;
>  
>  		/*
>  		 * If at least all accesses allowed on the destination are
> @@ -881,7 +904,6 @@ static bool is_access_to_paths_allowed(
>  				break;
>  		}
>  
> -		rule = find_rule_rcu(domain, walker_path.dentry);
>  		allowed_parent1 = allowed_parent1 ||
>  				  landlock_unmask_layers(
>  					  rule, access_masked_parent1,
> @@ -899,13 +921,16 @@ static bool is_access_to_paths_allowed(
>  jump_up:
>  		if (walker_path.dentry == walker_path.mnt->mnt_root) {
>  			/* follow_up gets the parent and puts the passed in path */
> -			path_get(&walker_path);
> +			if (!pathwalk_ref)
> +				path_get(&walker_path);
>  			if (follow_up(&walker_path)) {
> -				path_put(&walker_path);
> +				if (!pathwalk_ref)
> +					path_put(&walker_path);
>  				/* Ignores hidden mount points. */
>  				goto jump_up;
>  			} else {
> -				path_put(&walker_path);
> +				if (!pathwalk_ref)
> +					path_put(&walker_path);
>  				/*
>  				 * Stops at the real root.  Denies access
>  				 * because not all layers have granted access.
> @@ -925,10 +950,27 @@ static bool is_access_to_paths_allowed(
>  			}
>  			break;
>  		}
> -		parent_dentry = walker_path.dentry->d_parent;
> -		walker_path.dentry = parent_dentry;
> +		if (!pathwalk_ref) {
> +			parent_dentry = walker_path.dentry->d_parent;
> +
> +			rule = find_rule_rcu(domain, parent_dentry);
> +			if (read_seqretry(&rename_lock, rename_seqcount)) {
> +				pathwalk_ref = true;
> +				goto restart_pathwalk;
> +			} else {
> +				walker_path.dentry = parent_dentry;
> +			}
> +		} else {
> +			parent_dentry = dget_parent(walker_path.dentry);
> +			dput(walker_path.dentry);
> +			walker_path.dentry = parent_dentry;
> +			rule = find_rule_rcu(domain, walker_path.dentry);
> +		}
>  	}
>  
> +	if (pathwalk_ref)
> +		path_put(&walker_path);
> +
>  	rcu_read_unlock();
>  
>  	if (!allowed_parent1) {
> @@ -1040,22 +1082,55 @@ static bool collect_domain_accesses(
>  {
>  	unsigned long access_dom;
>  	bool ret = false;
> +	bool pathwalk_ref = false;
> +	unsigned int rename_seqcount;
> +	const struct landlock_rule *rule;
> +	struct dentry *parent_dentry;
>  
>  	if (WARN_ON_ONCE(!domain || !mnt_root || !dir || !layer_masks_dom))
>  		return true;
>  	if (is_nouser_or_private(dir))
>  		return true;
>  
> +	rcu_read_lock();
> +
> +restart_pathwalk:
>  	access_dom = landlock_init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>  					       layer_masks_dom,
>  					       LANDLOCK_KEY_INODE);
>  
> -	rcu_read_lock();
> +	/*
> +	 * Attempt to do a pathwalk without taking dentry references first, but
> +	 * if any rename happens while we are doing this, give up and do a walk
> +	 * with dget_parent instead.  This prevents wrong denials in the
> +	 * presence of a move followed by an immediate rmdir of the old parent,
> +	 * where even when both the original and the new parent has allow
> +	 * rules, we might still hit a negative dentry (the deleted old parent)
> +	 * and being unable to find either rules.
> +	 */
> +
> +	if (!pathwalk_ref) {
> +		rename_seqcount = read_seqbegin(&rename_lock);
> +		if (rename_seqcount % 2 == 1) {
> +			pathwalk_ref = true;
> +			dget(dir);
> +		}
> +	} else {
> +		dget(dir);
> +	}
> +	rule = find_rule_rcu(domain, dir);
> +	/*
> +	 * We don't need to check rename_seqcount here because we haven't
> +	 * followed any d_parent yet, and the d_inode of the path being
> +	 * accessed can't change under us as we have ref on path.dentry.  But
> +	 * once we start walking up the path, we need to check the seqcount to
> +	 * make sure the rule we got isn't based on a wrong/changing/negative
> +	 * dentry.
> +	 */
>  
>  	while (true) {
>  		/* Gets all layers allowing all domain accesses. */
> -		if (landlock_unmask_layers(find_rule_rcu(domain, dir), access_dom,
> -					   layer_masks_dom,
> +		if (landlock_unmask_layers(rule, access_dom, layer_masks_dom,
>  					   ARRAY_SIZE(*layer_masks_dom))) {
>  			/*
>  			 * Stops when all handled accesses are allowed by at
> @@ -1069,9 +1144,21 @@ static bool collect_domain_accesses(
>  		if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir)))
>  			break;
>  
> -		dir = dir->d_parent;
> +		if (!pathwalk_ref) {
> +			parent_dentry = dir->d_parent;
> +			rule = find_rule_rcu(domain, dir);
> +			if (read_seqretry(&rename_lock, rename_seqcount)) {
> +				pathwalk_ref = true;
> +				goto restart_pathwalk;
> +			} else {
> +				dir = parent_dentry;
> +			}
> +		}

Forgot else branch here

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 6dff5fb6b181..885121b1beef 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1153,6 +1153,11 @@ static bool collect_domain_accesses(
 			} else {
 				dir = parent_dentry;
 			}
+		} else {
+			parent_dentry = dget_parent(dir);
+			dput(dir);
+			dir = parent_dentry;
+			rule = find_rule_rcu(domain, dir);
 		}
 	}
 

>  	}
>  
> +	if (pathwalk_ref)
> +		dput(dir);
> +
>  	rcu_read_unlock();
>  
>  	return ret;