netdev - Re: [PATCH bpf-next v2 12/16] selftests: xsk: remove cleanup at end of program

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ8uoz28+uM9OMq0JKi2bOXR0F9E66LFuYahnTMUaxnRJDtRMQ@mail.gmail.com>
Date:   Thu, 19 Aug 2021 12:02:33 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Ciara Loftus <ciara.loftus@...el.com>,
        bpf <bpf@...r.kernel.org>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH bpf-next v2 12/16] selftests: xsk: remove cleanup at end
 of program

On Thu, Aug 19, 2021 at 11:43 AM Maciej Fijalkowski
<maciej.fijalkowski@...el.com> wrote:
>
> On Tue, Aug 17, 2021 at 11:27:25AM +0200, Magnus Karlsson wrote:
> > From: Magnus Karlsson <magnus.karlsson@...el.com>
> >
> > Remove the cleanup right before the program/process exits as this will
> > trigger the cleanup without us having to write or maintain any
> > code. The application is not a library, so let us benefit from that.
>
> Not a fan of that, I'd rather keep things tidy, but you're right that
> dropping this logic won't hurt us.

My strategy here was that all functions should clean up themselves and
be tidy, the exception to that being the main function as if it
exists, the program is gone and libc will clean up the allocations for
us. Maybe a bit pragmatic, but I do prefer less code in this case. On
the other hand,
I do not have any strong objections to keeping the code. Just think it
is unnecessary. But if we hide the allocations in a function, then I
would need to deallocate them later for it to look clean (according to
the above logic). Maybe that will improve readabilty. Will try it out.

/Magnus

> >
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > ---
> >  tools/testing/selftests/bpf/xdpxceiver.c | 29 +++++-------------------
> >  1 file changed, 6 insertions(+), 23 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
> > index 8ff24472ef1e..c1bb03e0ca07 100644
> > --- a/tools/testing/selftests/bpf/xdpxceiver.c
> > +++ b/tools/testing/selftests/bpf/xdpxceiver.c
> > @@ -1041,28 +1041,24 @@ static void run_pkt_test(int mode, int type)
> >  int main(int argc, char **argv)
> >  {
> >       struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY };
> > -     bool failure = false;
> >       int i, j;
> >
> >       if (setrlimit(RLIMIT_MEMLOCK, &_rlim))
> >               exit_with_error(errno);
> >
> > -     for (int i = 0; i < MAX_INTERFACES; i++) {
> > +     for (i = 0; i < MAX_INTERFACES; i++) {
> >               ifdict[i] = malloc(sizeof(struct ifobject));
> >               if (!ifdict[i])
> >                       exit_with_error(errno);
> >
> >               ifdict[i]->ifdict_index = i;
> >               ifdict[i]->xsk_arr = calloc(2, sizeof(struct xsk_socket_info *));
> > -             if (!ifdict[i]->xsk_arr) {
> > -                     failure = true;
> > -                     goto cleanup;
> > -             }
> > +             if (!ifdict[i]->xsk_arr)
> > +                     exit_with_error(errno);
> > +
> >               ifdict[i]->umem_arr = calloc(2, sizeof(struct xsk_umem_info *));
> > -             if (!ifdict[i]->umem_arr) {
> > -                     failure = true;
> > -                     goto cleanup;
> > -             }
> > +             if (!ifdict[i]->umem_arr)
> > +                     exit_with_error(errno);
> >       }
> >
> >       setlocale(LC_ALL, "");
> > @@ -1081,19 +1077,6 @@ int main(int argc, char **argv)
> >               }
> >       }
> >
> > -cleanup:
> > -     for (int i = 0; i < MAX_INTERFACES; i++) {
> > -             if (ifdict[i]->ns_fd != -1)
> > -                     close(ifdict[i]->ns_fd);
> > -             free(ifdict[i]->xsk_arr);
> > -             free(ifdict[i]->umem_arr);
> > -             free(ifdict[i]);
> > -     }
> > -
> > -     if (failure)
> > -             exit_with_error(errno);
> > -
> >       ksft_exit_pass();
> > -
> >       return 0;
> >  }
> > --
> > 2.29.0
> >