| Message ID | 20210215154638.4627-3-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce bpf_link in libbpf's xsk | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: jonathan.lemon@gmail.com yhs@fb.com bjorn@kernel.org john.fastabend@gmail.com kpsingh@kernel.org songliubraving@fb.com kafai@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Maciej Fijalkowski wrote: > xsk_lookup_bpf_maps, based on prog_fd, looks whether current prog has a > reference to XSKMAP. BPF prog can include insns that work on various BPF > maps and this is covered by iterating through map_ids. > > The bpf_map_info that is passed to bpf_obj_get_info_by_fd for filling > needs to be cleared at each iteration, so that it doesn't any outdated > fields and that is currently missing in the function of interest. > > To fix that, zero-init map_info via memset before each > bpf_obj_get_info_by_fd call. > > Also, since the area of this code is touched, in general strcmp is > considered harmful, so let's convert it to strncmp and provide the > length of the array name that we're looking for. > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- This is a bugfix independent of the link bits correct? Would be best to send to bpf then. > tools/lib/bpf/xsk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 5911868efa43..fb259c0bba93 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -616,6 +616,7 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > __u32 i, *map_ids, num_maps, prog_len = sizeof(struct bpf_prog_info); > __u32 map_len = sizeof(struct bpf_map_info); > struct bpf_prog_info prog_info = {}; > + const char *map_name = "xsks_map"; > struct xsk_ctx *ctx = xsk->ctx; > struct bpf_map_info map_info; > int fd, err; > @@ -645,13 +646,14 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > if (fd < 0) > continue; > > + memset(&map_info, 0, map_len); > err = bpf_obj_get_info_by_fd(fd, &map_info, &map_len); > if (err) { > close(fd); > continue; > } > > - if (!strcmp(map_info.name, "xsks_map")) { > + if (!strncmp(map_info.name, map_name, strlen(map_name))) { > ctx->xsks_map_fd = fd; > continue; Also just looking at this how is above not buggy? Should be a break instead of continue? If we match another "xsks_map" here won't we stomp on xsks_map_fd and leak a file descriptor? > } > -- > 2.20.1 >
On Mon, Feb 15, 2021 at 12:33:37PM -0800, John Fastabend wrote: > Maciej Fijalkowski wrote: > > xsk_lookup_bpf_maps, based on prog_fd, looks whether current prog has a > > reference to XSKMAP. BPF prog can include insns that work on various BPF > > maps and this is covered by iterating through map_ids. > > > > The bpf_map_info that is passed to bpf_obj_get_info_by_fd for filling > > needs to be cleared at each iteration, so that it doesn't any outdated > > fields and that is currently missing in the function of interest. > > > > To fix that, zero-init map_info via memset before each > > bpf_obj_get_info_by_fd call. > > > > Also, since the area of this code is touched, in general strcmp is > > considered harmful, so let's convert it to strncmp and provide the > > length of the array name that we're looking for. > > > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > --- > > This is a bugfix independent of the link bits correct? Would be best > to send to bpf then. Right, I can pull this out of the series. > > > tools/lib/bpf/xsk.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > > index 5911868efa43..fb259c0bba93 100644 > > --- a/tools/lib/bpf/xsk.c > > +++ b/tools/lib/bpf/xsk.c > > @@ -616,6 +616,7 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > > __u32 i, *map_ids, num_maps, prog_len = sizeof(struct bpf_prog_info); > > __u32 map_len = sizeof(struct bpf_map_info); > > struct bpf_prog_info prog_info = {}; > > + const char *map_name = "xsks_map"; > > struct xsk_ctx *ctx = xsk->ctx; > > struct bpf_map_info map_info; > > int fd, err; > > @@ -645,13 +646,14 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > > if (fd < 0) > > continue; > > > > + memset(&map_info, 0, map_len); > > err = bpf_obj_get_info_by_fd(fd, &map_info, &map_len); > > if (err) { > > close(fd); > > continue; > > } > > > > - if (!strcmp(map_info.name, "xsks_map")) { > > + if (!strncmp(map_info.name, map_name, strlen(map_name))) { > > ctx->xsks_map_fd = fd; > > continue; > > Also just looking at this how is above not buggy? Should be a break instead > of continue? If we match another "xsks_map" here won't we stomp on xsks_map_fd > and leak a file descriptor? Will fix! > > > } > > -- > > 2.20.1 > > > >
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 5911868efa43..fb259c0bba93 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -616,6 +616,7 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) __u32 i, *map_ids, num_maps, prog_len = sizeof(struct bpf_prog_info); __u32 map_len = sizeof(struct bpf_map_info); struct bpf_prog_info prog_info = {}; + const char *map_name = "xsks_map"; struct xsk_ctx *ctx = xsk->ctx; struct bpf_map_info map_info; int fd, err; @@ -645,13 +646,14 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) if (fd < 0) continue; + memset(&map_info, 0, map_len); err = bpf_obj_get_info_by_fd(fd, &map_info, &map_len); if (err) { close(fd); continue; } - if (!strcmp(map_info.name, "xsks_map")) { + if (!strncmp(map_info.name, map_name, strlen(map_name))) { ctx->xsks_map_fd = fd; continue; }
xsk_lookup_bpf_maps, based on prog_fd, looks whether current prog has a reference to XSKMAP. BPF prog can include insns that work on various BPF maps and this is covered by iterating through map_ids. The bpf_map_info that is passed to bpf_obj_get_info_by_fd for filling needs to be cleared at each iteration, so that it doesn't any outdated fields and that is currently missing in the function of interest. To fix that, zero-init map_info via memset before each bpf_obj_get_info_by_fd call. Also, since the area of this code is touched, in general strcmp is considered harmful, so let's convert it to strncmp and provide the length of the array name that we're looking for. Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- tools/lib/bpf/xsk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)