lkml.org 
[lkml]   [2020]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v2 bpf] tools: bpftool: Add missing close before bpftool net attach exit
From
Date

在 2020/11/10 12:33, John Fastabend 写道:
> 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>
>> ---
>> v1->v2: use cleanup tag instead of repeated closes
>> tools/bpf/bpftool/net.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
>> index 910e7bac6e9e..1ac7228167e6 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;
>> }
>> }
>>
>> @@ -600,13 +600,15 @@ static int do_attach(int argc, char **argv)
> I think now that return value depends on this err it should be 'if (err)'
> otherwise we risk retunring non-zero error code from do_attach which
> will cause programs to fail.
I agree with you. Thanks.
>> 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;
>
> Alternatively we could add an 'err = 0' here, but above should never
> return a value >0 as far as I can see.
It's true that 'err > 0' doesn't exist currently , but adding 'err = 0'
would make the code clearer. Thanks for your advice.
>> +cleanup:
>> + close(progfd);
>> + return err;
>> }
>>
>> static int do_detach(int argc, char **argv)
>> --
>> 2.17.1
>>
Can it be fixed like this?

--- 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,19 @@ 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;
+       ret = 0;
+cleanup:
+       close(progfd);
+       return err;
 }

>
> .
>

\
 
 \ /
  Last update: 2020-11-10 09:41    [W:0.053 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site