Re: [PATCH bpf-next 2/2] selftests/bpf: Add tests for bpf_get_dentry_xattr

[Song Liu <songliubraving@meta.com>]

Hi Christian, 

Thanks a lot for your comments.

> On Jul 26, 2024, at 4:51 AM, Christian Brauner <brauner@kernel.org> wrote:
> 
> On Fri, Jul 26, 2024 at 09:19:54AM GMT, Song Liu wrote:
>> Hi Christian, 
>> 
>>> On Jul 26, 2024, at 12:06 AM, Christian Brauner <brauner@kernel.org> wrote:
>> 
>> [...]
>> 
>>>> +
>>>> + for (i = 0; i < 10; i++) {
>>>> + ret = bpf_get_dentry_xattr(dentry, "user.kfunc", &value_ptr);
>>>> + if (ret == sizeof(expected_value) &&
>>>> +    !bpf_strncmp(value, ret, expected_value))
>>>> + matches++;
>>>> +
>>>> + prev_dentry = dentry;
>>>> + dentry = bpf_dget_parent(prev_dentry);
>>> 
>>> Why do you need to walk upwards and instead of reading the xattr values
>>> during security_inode_permission()?
>> 
>> In this use case, we would like to add xattr to the directory to cover
>> all files under it. For example, assume we have the following xattrs:
>> 
>>  /bin  xattr: user.policy_A = value_A
>>  /bin/gcc-6.9/ xattr: user.policy_A = value_B
>>  /bin/gcc-6.9/gcc xattr: user.policy_A = value_C
>> 
>> /bin/gcc-6.9/gcc will use value_C;
>> /bin/gcc-6.9/<other_files> will use value_B;
>> /bin/<other_folder_or_file> will use value_A;
>> 
>> By walking upwards from security_file_open(), we can finish the logic 
>> in a single LSM hook:
>> 
>>    repeat:
>>        if (dentry have user.policy_A) {
>>            /* make decision based on value */;
>>        } else {
>>            dentry = bpf_dget_parent();
>>            goto repeat;
>>        }
>> 
>> Does this make sense? Or maybe I misunderstood the suggestion?
> 
> Imho, what you're doing belongs into inode_permission() not into
> security_file_open(). That's already too late and it's somewhat clear
> from the example you're using that you're essentially doing permission
> checking during path lookup.

I am not sure I follow the suggestion to implement this with 
security_inode_permission()? Could you please share more details about
this idea?

> Btw, what you're doing is potentially very heavy-handed because you're
> retrieving xattrs for which no VFS cache exists so you might end up
> causing a lot of io.
> 
> Say you have a 10000 deep directory hierarchy and you open a
> file_at_level_10000. With that dget_parent() logic in the worst case you
> end up walking up the whole hierarchy reading xattr values from disk
> 10000 times. You can achieve the same result and cleaner if you do the
> checking in inode_permission() where it belongs and you only cause all
> of that pain once and you abort path lookup correctly.

Yes, we need the BPF program to limit the number of parents to walk. 

> Also, I'm not even sure this is always correct because you're
> retroactively checking what policy to apply based on the xattr value
> walking up the parent chain. But a rename could happen and then the
> ancestor chain you're checking is different from the current chain or
> there's a bunch of mounts along the way. 
> 
> Imho, that dget_parent() thing just encourages very badly written bpf
> LSM programs. That's certainly not an interface we want to expose.
> 
>> Also, we don't have a bpf_get_inode_xattr() yet. I guess we will need
>> it for the security_inode_permission approach. If we agree that's a
> 
> Yes, that's fine.
> 
> You also need to ensure that you're only reading user.* xattrs. I know
> you already do that for bpf_get_file_xattr() but this helper needs the
> same treatment.

Sounds good. bpf_get_inode_xattr() would be a very useful kfunc. Let's
add that. 

> 
> And you need to force a drop-out of RCU path lookup btw because you're
> almost definitely going to block when you check the xattr.

We only allow xattr look up from sleepable context. 

> 
>> better approach, I more than happy to implement it that way. In fact,
>> I think we will eventually need both bpf_get_inode_xattr() and 
>> bpf_get_dentry_xattr().
> 
> I'm not sure about that because it's royally annoying in the first place
> that we have to dentry and inode separately in the xattr handlers
> because LSMs sometimes call them from a location when the dentry and
> inode aren't yet fused together. The dentry is the wrong data structure
> to care about here.

Thanks,
Song