| Message ID | 20220517112748.358295-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2,bpf-next] samples/bpf: check detach prog exist or not in xdp_fwd | expand |
Zhengchao Shao <shaozhengchao@huawei.com> writes: > Before detach the prog, we should check detach prog exist or not. > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > samples/bpf/xdp_fwd_user.c | 52 +++++++++++++++++++++++++++++++------- > 1 file changed, 43 insertions(+), 9 deletions(-) > > diff --git a/samples/bpf/xdp_fwd_user.c b/samples/bpf/xdp_fwd_user.c > index 1828487bae9a..2294486ef10a 100644 > --- a/samples/bpf/xdp_fwd_user.c > +++ b/samples/bpf/xdp_fwd_user.c > @@ -47,17 +47,51 @@ static int do_attach(int idx, int prog_fd, int map_fd, const char *name) > return err; > } > > -static int do_detach(int idx, const char *name) > +static int do_detach(int idx, const char *name, const char *prog_name) two 'name' arguments is a bit confusing; could we rename the parameters to 'ifindex', 'ifname' and 'app_name', then use 'prog_name' for the stack variable below instead of 'namepad'? > { > - int err; > + int err = 1; > + __u32 info_len, curr_prog_id; > + struct bpf_prog_info prog_info = {}; > + int prog_fd; > + char namepad[BPF_OBJ_NAME_LEN]; Reverse x-mas tree, please. > + > + if (bpf_xdp_query_id(idx, xdp_flags, &curr_prog_id)) { > + printf("ERROR: bpf_xdp_query_id failed\n"); strerror(errno) might be nice to add to the error message, so users have an inkling as to why the call is failing (same below). > + return err; > + } > + > + if (!curr_prog_id) { > + printf("ERROR: flags(0x%x) xdp prog is not attached to %s\n", > + xdp_flags, name); > + return err; > + } > > - err = bpf_xdp_detach(idx, xdp_flags, NULL); > - if (err < 0) > - printf("ERROR: failed to detach program from %s\n", name); > + info_len = sizeof(prog_info); > + prog_fd = bpf_prog_get_fd_by_id(curr_prog_id); > + if (prog_fd < 0 && errno == ENOENT) { Why the ENOENT check? This should error out regardless of the errno, no? > + printf("ERROR: bpf_prog_get_fd_by_id failed\n"); strerror(errno) > + return err; > + } > + > + err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len); > + if (err) { > + printf("ERROR: bpf_obj_get_info_by_fd failed\n"); strerror(errno) > + return err; > + } > + snprintf(namepad, sizeof(namepad), "%s_prog", prog_name); If the binary somehow gets renamed, this may overflow and you'll end up without a NULL byte terminating the string. So either check the input length first, or make sure to set the last byte to '\0' after this call... > + > + if (strcmp(prog_info.name, namepad)) { > + printf("ERROR: %s isn't attached to %s\n", prog_name, name); > + } else { > + err = bpf_xdp_detach(idx, xdp_flags, NULL); This call should be including an opts struct with the fd obtained above as the old_prog_fd (so make sure it wasn't swapped out since the check). > + if (err < 0) > + printf("ERROR: failed to detach program from %s\n", > + name); strerror(errno) -Toke
diff --git a/samples/bpf/xdp_fwd_user.c b/samples/bpf/xdp_fwd_user.c index 1828487bae9a..2294486ef10a 100644 --- a/samples/bpf/xdp_fwd_user.c +++ b/samples/bpf/xdp_fwd_user.c @@ -47,17 +47,51 @@ static int do_attach(int idx, int prog_fd, int map_fd, const char *name) return err; } -static int do_detach(int idx, const char *name) +static int do_detach(int idx, const char *name, const char *prog_name) { - int err; + int err = 1; + __u32 info_len, curr_prog_id; + struct bpf_prog_info prog_info = {}; + int prog_fd; + char namepad[BPF_OBJ_NAME_LEN]; + + if (bpf_xdp_query_id(idx, xdp_flags, &curr_prog_id)) { + printf("ERROR: bpf_xdp_query_id failed\n"); + return err; + } + + if (!curr_prog_id) { + printf("ERROR: flags(0x%x) xdp prog is not attached to %s\n", + xdp_flags, name); + return err; + } - err = bpf_xdp_detach(idx, xdp_flags, NULL); - if (err < 0) - printf("ERROR: failed to detach program from %s\n", name); + info_len = sizeof(prog_info); + prog_fd = bpf_prog_get_fd_by_id(curr_prog_id); + if (prog_fd < 0 && errno == ENOENT) { + printf("ERROR: bpf_prog_get_fd_by_id failed\n"); + return err; + } + + err = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &info_len); + if (err) { + printf("ERROR: bpf_obj_get_info_by_fd failed\n"); + return err; + } + snprintf(namepad, sizeof(namepad), "%s_prog", prog_name); + + if (strcmp(prog_info.name, namepad)) { + printf("ERROR: %s isn't attached to %s\n", prog_name, name); + } else { + err = bpf_xdp_detach(idx, xdp_flags, NULL); + if (err < 0) + printf("ERROR: failed to detach program from %s\n", + name); + /* TODO: Remember to cleanup map, when adding use of shared map + * bpf_map_delete_elem((map_fd, &idx); + */ + } - /* TODO: Remember to cleanup map, when adding use of shared map - * bpf_map_delete_elem((map_fd, &idx); - */ return err; } @@ -169,7 +203,7 @@ int main(int argc, char **argv) return 1; } if (!attach) { - err = do_detach(idx, argv[i]); + err = do_detach(idx, argv[i], prog_name); if (err) ret = err; } else {
Before detach the prog, we should check detach prog exist or not. Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- samples/bpf/xdp_fwd_user.c | 52 +++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 9 deletions(-)