linux-kernel - Re: [PATCH bpf-next v2 1/4] bpf/crib: Introduce task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <AM6PR03MB584873E3A7333047E28E25B199532@AM6PR03MB5848.eurprd03.prod.outlook.com>
Date: Wed, 6 Nov 2024 21:05:48 +0000
From: Juntong Deng <juntong.deng@...look.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com,
 andrii@...nel.org, martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org,
 yonghong.song@...ux.dev, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, memxor@...il.com, snorcht@...il.com,
 bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 1/4] bpf/crib: Introduce task_file open-coded
 iterator kfuncs

On 2024/11/6 20:02, Andrii Nakryiko wrote:

> On Fri, Nov 1, 2024 at 1:22 PM Juntong Deng <juntong.deng@...look.com> wrote:

>> Yes, users can access file->f_op, but there seems to be no way for
>> users to get references to struct file_operations for the various file
>> types? For example, how does a user get a reference to socket_file_ops?
> 
> See [0]. Libbpf will find it for the BPF program from kallsyms.
> 
>    [0] https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/progs/test_ksyms.c#L13-L18
> 

Thanks for telling me this method.

Yes, then we don't need bpf_get_file_ops_type().

>>
>> Yes, I agree that it is feasible.
>>
>> But there is a question here, should we expose the internal state
>> structure of the iterator (If we want to embed) ?
>>
>> I guess that we need two versions of data structures struct bpf_iter_xxx
>> and struct bpf_iter_xxx_kern is for the purpose of encapsulation?
> 
> Yes, that's what we do for iterator state structure, and you should do
> that as well. bpf_iter_xxx one will be opaque (see other examples, we
> literally add `u64 __opaque[N];` there).
> 
> But this bpf_iter_task_file_item will be sort of internal API that is
> returned from bpf_iter_task_file_next(). So you'll have something like
> 
> struct bpf_iter_task_file {
>      .... additional state ...
>      struct bpf_iter_task_file_item item;
> };
> 
> then you have
> 
> struct bpf_iter_task_file_item bpf_iter_task_file_next(struct
> bpf_iter_task_file *it)
> {
>      struct bpf_iter_task_file_kern *kit = (void *)it;
> 
>      ...
>      kit->item.task = <sometask>;
>      kit->item.file = <file>; /* and so on */
> 
>      return &kit->item;
> }
> 
>>
>> With two versions of the data structure, users can only manipulate
>> the iterator using the iterator kfuncs, avoiding users from directly
>> accessing the internal state.
>>
>> After we decide to return struct bpf_iter_task_file_item, these members
>> will not be able to change and users can directly access/change the
>> internal state of the iterator.
> 
> Yes, you have to carefully set up bpf_iter_task_file_item, but you
> could expand it in the future without breaking any old users, because
> you only return it by pointer and with BPF CO-RE all the field shifts
> will be correctly handled.
> 

You are right, in the next patch series version I will use
struct bpf_iter_task_file_item.

Could you please also give some feedback on
"2. Where should we put CRIB related kfuncs?"
in the discussion section of the v3 patch series
cover letter [0]?

[0]: 
https://lore.kernel.org/bpf/AM6PR03MB58488FD29EB0D0B89D52AABB99532@AM6PR03MB5848.eurprd03.prod.outlook.com/T/#t

Then I can fix all the things in the v4 patch series.

Many thanks.