| Message ID | 20201111135425.56533-1-wanghai38@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3,bpf] tools: bpftool: Add missing close before bpftool net attach exit | 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 |
| netdev/subject_prefix | success | Link |
| 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, 43 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 |
On 11/11/20 2:54 PM, Wang Hai wrote: > progfd is created by prog_parse_fd(), before 'bpftool net attach' exit, > it should be closed. > > Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on interface") > Signed-off-by: Wang Hai <wanghai38@huawei.com> > --- > v2->v3: add 'err = 0' before successful return > v1->v2: use cleanup tag instead of repeated closes > tools/bpf/bpftool/net.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c > index 910e7bac6e9e..f927392271cc 100644 > --- a/tools/bpf/bpftool/net.c > +++ b/tools/bpf/bpftool/net.c > @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv) > > ifindex = net_parse_dev(&argc, &argv); > if (ifindex < 1) { > - close(progfd); > - return -EINVAL; > + err = -EINVAL; > + goto cleanup; > } > > if (argc) { > @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) > overwrite = true; > } else { > p_err("expected 'overwrite', got: '%s'?", *argv); > - close(progfd); > - return -EINVAL; > + err = -EINVAL; > + goto cleanup; > } > } > > @@ -597,16 +597,20 @@ static int do_attach(int argc, char **argv) > err = do_attach_detach_xdp(progfd, attach_type, ifindex, > overwrite); > > - if (err < 0) { > + if (err) { > p_err("interface %s attach failed: %s", > attach_type_strings[attach_type], strerror(-err)); > - return err; > + goto cleanup; > } > > if (json_output) > jsonw_null(json_wtr); > > - return 0; > + err = 0; > + > +cleanup: > + close(progfd); > + return err; > } > > static int do_detach(int argc, char **argv) > LGTM, thanks! Reviewed-By: Michal Rostecki <mrostecki@opensuse.org>
On 11/11/20 2:54 PM, Wang Hai wrote: > progfd is created by prog_parse_fd(), before 'bpftool net attach' exit, > it should be closed. > > Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on interface") > Signed-off-by: Wang Hai <wanghai38@huawei.com> > --- > v2->v3: add 'err = 0' before successful return > v1->v2: use cleanup tag instead of repeated closes > tools/bpf/bpftool/net.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c > index 910e7bac6e9e..f927392271cc 100644 > --- a/tools/bpf/bpftool/net.c > +++ b/tools/bpf/bpftool/net.c > @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv) > > ifindex = net_parse_dev(&argc, &argv); > if (ifindex < 1) { > - close(progfd); > - return -EINVAL; > + err = -EINVAL; > + goto cleanup; > } > > if (argc) { > @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) > overwrite = true; > } else { > p_err("expected 'overwrite', got: '%s'?", *argv); > - close(progfd); > - return -EINVAL; > + err = -EINVAL; > + goto cleanup; > } > } > > @@ -597,16 +597,20 @@ static int do_attach(int argc, char **argv) > err = do_attach_detach_xdp(progfd, attach_type, ifindex, > overwrite); > > - if (err < 0) { > + if (err) { > p_err("interface %s attach failed: %s", > attach_type_strings[attach_type], strerror(-err)); > - return err; > + goto cleanup; > } > > if (json_output) > jsonw_null(json_wtr); > > - return 0; > + err = 0; Why is the 'err = 0' still needed here given we test for err != 0 earlier? Would just remove it, otherwise looks good. > +cleanup: > + close(progfd); > + return err; > } > > static int do_detach(int argc, char **argv) >
On Wed, Nov 11, 2020 at 1:24 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 11/11/20 2:54 PM, Wang Hai wrote: > > progfd is created by prog_parse_fd(), before 'bpftool net attach' exit, > > it should be closed. > > > > Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on interface") > > Signed-off-by: Wang Hai <wanghai38@huawei.com> > > --- > > v2->v3: add 'err = 0' before successful return > > v1->v2: use cleanup tag instead of repeated closes > > tools/bpf/bpftool/net.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c > > index 910e7bac6e9e..f927392271cc 100644 > > --- a/tools/bpf/bpftool/net.c > > +++ b/tools/bpf/bpftool/net.c > > @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv) > > > > ifindex = net_parse_dev(&argc, &argv); > > if (ifindex < 1) { > > - close(progfd); > > - return -EINVAL; > > + err = -EINVAL; > > + goto cleanup; > > } > > > > if (argc) { > > @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) > > overwrite = true; > > } else { > > p_err("expected 'overwrite', got: '%s'?", *argv); > > - close(progfd); > > - return -EINVAL; > > + err = -EINVAL; > > + goto cleanup; > > } > > } > > > > @@ -597,16 +597,20 @@ static int do_attach(int argc, char **argv) > > err = do_attach_detach_xdp(progfd, attach_type, ifindex, > > overwrite); > > > > - if (err < 0) { > > + if (err) { > > p_err("interface %s attach failed: %s", > > attach_type_strings[attach_type], strerror(-err)); > > - return err; > > + goto cleanup; > > } > > > > if (json_output) > > jsonw_null(json_wtr); > > > > - return 0; > > + err = 0; > > Why is the 'err = 0' still needed here given we test for err != 0 earlier? > Would just remove it, otherwise looks good. This patch was already applied. Wang, can you please follow up with another patch to address Daniel's feedback? > > > +cleanup: > > + close(progfd); > > + return err; > > } > > > > static int do_detach(int argc, char **argv) > > >
On Wed, Nov 11, 2020 at 2:37 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Nov 11, 2020 at 1:24 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > On 11/11/20 2:54 PM, Wang Hai wrote: > > > progfd is created by prog_parse_fd(), before 'bpftool net attach' exit, > > > it should be closed. > > > > > > Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on interface") > > > Signed-off-by: Wang Hai <wanghai38@huawei.com> > > > --- > > > v2->v3: add 'err = 0' before successful return > > > v1->v2: use cleanup tag instead of repeated closes > > > tools/bpf/bpftool/net.c | 18 +++++++++++------- > > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c > > > index 910e7bac6e9e..f927392271cc 100644 > > > --- a/tools/bpf/bpftool/net.c > > > +++ b/tools/bpf/bpftool/net.c > > > @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv) > > > > > > ifindex = net_parse_dev(&argc, &argv); > > > if (ifindex < 1) { > > > - close(progfd); > > > - return -EINVAL; > > > + err = -EINVAL; > > > + goto cleanup; > > > } > > > > > > if (argc) { > > > @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) > > > overwrite = true; > > > } else { > > > p_err("expected 'overwrite', got: '%s'?", *argv); > > > - close(progfd); > > > - return -EINVAL; > > > + err = -EINVAL; > > > + goto cleanup; > > > } > > > } > > > > > > @@ -597,16 +597,20 @@ static int do_attach(int argc, char **argv) > > > err = do_attach_detach_xdp(progfd, attach_type, ifindex, > > > overwrite); > > > > > > - if (err < 0) { > > > + if (err) { > > > p_err("interface %s attach failed: %s", > > > attach_type_strings[attach_type], strerror(-err)); > > > - return err; > > > + goto cleanup; > > > } > > > > > > if (json_output) > > > jsonw_null(json_wtr); > > > > > > - return 0; > > > + err = 0; > > > > Why is the 'err = 0' still needed here given we test for err != 0 earlier? > > Would just remove it, otherwise looks good. > > This patch was already applied. Wang, can you please follow up with > another patch to address Daniel's feedback? Actually, the patch hasn't been applied yet, so please just respin, thanks. > > > > > > +cleanup: > > > + close(progfd); > > > + return err; > > > } > > > > > > static int do_detach(int argc, char **argv) > > > > >
diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c index 910e7bac6e9e..f927392271cc 100644 --- a/tools/bpf/bpftool/net.c +++ b/tools/bpf/bpftool/net.c @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv) ifindex = net_parse_dev(&argc, &argv); if (ifindex < 1) { - close(progfd); - return -EINVAL; + err = -EINVAL; + goto cleanup; } if (argc) { @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) overwrite = true; } else { p_err("expected 'overwrite', got: '%s'?", *argv); - close(progfd); - return -EINVAL; + err = -EINVAL; + goto cleanup; } } @@ -597,16 +597,20 @@ static int do_attach(int argc, char **argv) err = do_attach_detach_xdp(progfd, attach_type, ifindex, overwrite); - if (err < 0) { + if (err) { p_err("interface %s attach failed: %s", attach_type_strings[attach_type], strerror(-err)); - return err; + goto cleanup; } if (json_output) jsonw_null(json_wtr); - return 0; + err = 0; + +cleanup: + close(progfd); + return err; } static int do_detach(int argc, char **argv)
progfd is created by prog_parse_fd(), before 'bpftool net attach' exit, it should be closed. Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on interface") Signed-off-by: Wang Hai <wanghai38@huawei.com> --- v2->v3: add 'err = 0' before successful return v1->v2: use cleanup tag instead of repeated closes tools/bpf/bpftool/net.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)