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

[Tingmao Wang <m@maowtm.org>]

On 6/2/25 18:35, Mickaël Salaün wrote:
> On Sat, May 31, 2025 at 02:51:22PM +0100, Tingmao Wang wrote:
>> [...]
>>
>> / # cd /
>> / # cp /linux/samples/landlock/sandboxer .
>> / # mkdir a b
>> / # mkdir a/foo
>> / # echo baz > a/foo/bar
>> / # mount --bind a b
>> / # LL_FS_RO=/ LL_FS_RW=/ ./sandboxer bash
>> Executing the sandboxed command...
>> / # cd /b/foo
>> /b/foo # cat bar
>> baz
>> /b/foo # mv /a/foo /foo
>> /b/foo # cd ..     # <- We're now disconnected
>> bash: cd: ..: No such file or directory
>> /b/foo # cat bar
>> baz                # <- but landlock still lets us read the file
>>
>> However, I think this patch will change this behavior due to the use of
>> path_connected
>>
>> root@10a8fff999ce:/# mkdir a b
>> root@10a8fff999ce:/# mkdir a/foo
>> root@10a8fff999ce:/# echo baz > a/foo/bar
>> root@10a8fff999ce:/# mount --bind a b
>> root@10a8fff999ce:/# LL_FS_RO=/ LL_FS_RW=/ ./sandboxer bash
>> Executing the sandboxed command...
>> bash: cannot set terminal process group (191): Inappropriate ioctl for device
>> bash: no job control in this shell
>> root@10a8fff999ce:/# cd /b/foo
>> root@10a8fff999ce:/b/foo# cat bar
>> baz
>> root@10a8fff999ce:/b/foo# mv /a/foo /foo
>> root@10a8fff999ce:/b/foo# cd ..
>> bash: cd: ..: No such file or directory
>> root@10a8fff999ce:/b/foo# cat bar
>> cat: bar: Permission denied
> 
> This is a good test case, we should add a test for that.

Agreed with Mickaël that I will work on a Landlock selftest for this.

> 
>>
>> I'm not sure if the original behavior was intentional, but since this
>> technically counts as a functional changes, just pointing this out.
> 
> This is indeed an issue.

Because in the non-Landlocked case, applications with a disconnected CWD
still can access files under its CWD, just not above, my thinking is that
it's best to keep the current Landlock behaviour.  Mickaël might reply
here too but he also thought that, from the point of view of the person
creating the policy, the current behaviour is less surprising.

>
>>
>> Also I'm slightly worried about the performance overhead of doing
>> path_connected for every hop in the iteration (but ultimately it's
>> Mickaël's call).
> 
> Yes, we need to check with a benchmark.  We might want to keep the
> walker_path.dentry == walker_path.mnt->mnt_root check inlined.

I think this will depend on how the final implementation goes, but if we
can check it only once at the very end (potentially for free by having
logic that can realize the walk never reached path.mnt?), I would think it
ought to not make a difference.

For Song's benefit, if you want to do it, here are some scripts that might
come in handy in benchmarking (created by Mickaël):
https://github.com/landlock-lsm/landlock-test-tools/pull/16
and comparing results / BPF-based overhead tracing:
https://github.com/landlock-lsm/landlock-test-tools/pull/17

> >> At least for Landlock, I think if we want to block all
>> access to disconnected files, as long as we eventually realize we have
>> been disconnected (by doing the "if dentry == path.mnt" check once when we
>> reach root), and in that case deny access, we should be good.
>>
>>
>>> @@ -918,12 +915,15 @@ static bool is_access_to_paths_allowed(
>>>  				allowed_parent1 = true;
>>>  				allowed_parent2 = true;
>>>  			}
>>> +			goto walk_done;
>>> +		case PATH_PARENT_SAME_MOUNT:
>>>  			break;
>>> +		default:
>>> +			WARN_ON_ONCE(1);
>>> +			goto walk_done;
>>>  		}
>>> -		parent_dentry = dget_parent(walker_path.dentry);
>>> -		dput(walker_path.dentry);
>>> -		walker_path.dentry = parent_dentry;
>>>  	}
>>> +walk_done:
>>>  	path_put(&walker_path);
>>>  
>>>  	if (!allowed_parent1) {
>>
>>