linux-kernel - Re: [PATCH bpf-next] bpf: Support default .validate() and .update() behavior for struct

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f10dd9ba-de75-c2b1-a7e9-fd71bdc2f0fe@linux.dev>
Date:   Mon, 14 Aug 2023 09:55:37 -0700
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     David Vernet <void@...ifault.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com,
        jolsa@...nel.org, linux-kernel@...r.kernel.org,
        kernel-team@...a.com, tj@...nel.org, clm@...a.com,
        thinker.li@...il.com, Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next] bpf: Support default .validate() and .update()
 behavior for struct_ops links

On 8/11/23 4:36 PM, David Vernet wrote:
> I see, thanks for explaining. This is why sched_ext doesn't really work
> with the BPF_F_LINK version of map update. We can't guarantee that a map
> can be updated if we can't succeed in ->reg(), because we can also race
> with e.g. sysrq unloading the scheduler between ->validate() and
> ->reg(). In a sense, it feels like ->reg() in "updateable" struct_ops
> implementations should be void, whereas in other struct_ops
> implementations like scx() it has to be int *. If validate() is meant to
> prevent the scenario you outlined, can you help me understand why we
> still check the return value of ->reg() in bpf_struct_ops_link_create()?
> Or at the very least it seems like we should WARN_ON()?

->regs() can fail if another struct_ops under the same name has already been 
loaded to the subsystem. If another subsystem needs another return value to 
support .update, I believe it can be done if that is blocking scx to support 
"updateable" link.

>> If it needs to validate struct_ops as a while,

There was a typo: as a /whole/.

>>
>> 1. it must be implemented in .validate instead of .reg. Otherwise, it may
>> end up having an unusable map.
> 
> Some clarity on this point (why we check ->reg() on the ->validate()
> path) would help me write this comment more clearly.


hmm... where does it check ->reg() on the ->validate() now?

I was meaning the struct_ops supported subsystem should validate the struct_ops 
map in '.validate' instead of in the '.reg'.

or I misunderstood the question?

> 
>> 2. if the validation is implemented in '.reg' only, the map update behavior
>> will be different between BPF_F_LINK map and the non BPF_F_LINK map.
> 
> Ack, this is good to document regardless.
> 
> I'll send a v3 on Monday with these comments added both to the code, and
> to the commit summary.
> 
> Thanks,
> David