| Message ID | 20220722171836.2852247-3-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Per-operation map permissions | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
| bpf/vmtest-bpf-next-VM_Test-1 | pending | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | pending | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | pending | Logs for Kernel LATEST on z15 with gcc |
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.
> 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. Roberto
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. > Roberto
> 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
On Mon, 2022-08-01 at 10:33 +0000, Roberto Sassu wrote: > > 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. 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. 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
On Thu, Aug 25, 2022 at 5:21 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > On Mon, 2022-08-01 at 10:33 +0000, Roberto Sassu wrote: > > > 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. > > 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 >
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 83c7136c5788..b4311155d535 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3689,7 +3689,7 @@ struct bpf_prog *bpf_prog_get_curr_or_next(u32 *id) return prog; } -#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD prog_id +#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD open_flags struct bpf_prog *bpf_prog_by_id(u32 id) { @@ -4315,7 +4315,7 @@ static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr) return btf_new_fd(attr, uattr); } -#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD btf_id +#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD open_flags static int bpf_btf_get_fd_by_id(const union bpf_attr *attr) { @@ -4733,7 +4733,7 @@ struct bpf_link *bpf_link_get_curr_or_next(u32 *id) return link; } -#define BPF_LINK_GET_FD_BY_ID_LAST_FIELD link_id +#define BPF_LINK_GET_FD_BY_ID_LAST_FIELD open_flags static int bpf_link_get_fd_by_id(const union bpf_attr *attr) {
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. Set open_flags as last field for the remaining bpf_*_get_fd_by_id() functions, so that this information can be taken into account by the bpf() system call. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- kernel/bpf/syscall.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)