> 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. Thanks Roberto
