On Thu, Aug 25, 2022 at 5:21 AM Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> wrote: > > On Mon, 2022-08-01 at 10:33 +0000, Roberto Sassu wrote: > > > From: Andrii Nakryiko [mailto:andrii.nakryiko@xxxxxxxxx] > > > Sent: Friday, July 29, 2022 8:49 PM > > > On Mon, Jul 25, 2022 at 12:10 AM Roberto Sassu < > > > roberto.sassu@xxxxxxxxxx> > > > wrote: > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > > > > > Sent: Friday, July 22, 2022 7:55 PM > > > > > On Fri, Jul 22, 2022 at 07:18:23PM +0200, Roberto Sassu wrote: > > > > > > The bpf() system call validates the bpf_attr structure > > > > > > received as > > > > > > argument, and considers data until the last field, defined > > > > > > for each > > > > > > operation. The remaing space must be filled with zeros. > > > > > > > > > > > > Currently, for bpf_*_get_fd_by_id() functions except > > > bpf_map_get_fd_by_id() > > > > > > the last field is *_id. Setting open_flags to BPF_F_RDONLY > > > > > > from user space > > > > > > will result in bpf() rejecting the argument. > > > > > > > > > > The kernel is doing the right thing. It should not ignore > > > > > fields. > > > > > > > > Exactly. As Andrii requested to add opts to all > > > > bpf_*_get_fd_by_id() > > > > functions, the last field in the kernel needs to be updated > > > > accordingly. > > > > > > > > > > It's been a while ago so details are hazy. But the idea was that if > > > we > > > add _opts variant for bpf_map_get_fd_by_id() for interface > > > consistency > > > all the other bpf_*_get_fd_by_id() probably should get _opts > > > variant > > > and use the same opts struct. Right now kernel doesn't support > > > specifying flags for non-maps and that's fine. I agree with Alexei > > > that kernel shouldn't just ignore unrecognized field silently. > > > > > > I think we still can add _opts() for all APIs, but user will need > > > to > > > know that non-map variants expect 0 as flags. For now. If we > > > eventually add ability to specify flags for, say, links, then > > > existing > > > API will just work. One can see how this get_fd_by_id() can use > > > read-only flags to return FDs that only support read-only > > > operations > > > on objects (e.g., fetching link info for links, dumping prog > > > instructions for programs), but not modification operations (e.g., > > > updating prog for links, or whatever write operation could be for > > > programs). > > > > > > So I don't think there is contradiction here. We might choose to > > > add > > > bpf_map_get_fd_by_id_opts() only, but we probably still should use > > > common struct name as if all bpf_*_get_fd_by_id_opts() exist. > > > > Ok, understood. > > Hi Andrii > > I'm about to send v4 with the suggestions you made. > > Since now libbpf v1 has been released, how it works for new patches? It > seems there is not a new section in libbpf.map (kernel) new API > functions should be added to. yes, we need to add LIBBPF_1.1.0 now. I might send a small patch today to do that, if not, feel free to add it in your patch set. Whoever lands first wins (unfortunately even if I add an empty section, first feature adding a new function to libbpf API would need to add "global:" and thus conflict with any other patch set adding new API). > > Also, I'm using a custom step in the CI: > > https://github.com/robertosassu/libbpf-ci/commit/7743eb92f81f95355571c07e5b7082a9a2b0bfe0 > > Do you want me to create a new PR before sending the patch set? > > Thanks > > Roberto >
