RE: [RFC][PATCH v3 02/15] bpf: Set open_flags as last bpf_attr field for bpf_*_get_fd_by_id() funcs

[Roberto Sassu <roberto.sassu@huawei.com>]

> From: Andrii Nakryiko [mailto:andrii.nakryiko@gmail.com]
> Sent: Friday, July 29, 2022 8:49 PM
> On Mon, Jul 25, 2022 at 12:10 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > 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