netdev - Re: [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAJ+HfNh_brhM5C1jModyUPibps2ouPcfGfYscavoqBFCLmWj7Q@mail.gmail.com>
Date:   Tue, 30 Mar 2021 19:21:41 +0200
From:   Björn Töpel <bjorn@...nel.org>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Ciara Loftus <ciara.loftus@...el.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>
Subject: Re: [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure

On Tue, 30 Mar 2021 at 17:08, Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Tue, Mar 30, 2021 at 5:06 AM Ciara Loftus <ciara.loftus@...el.com> wrote:
> >

[...]

> >         if (--ctx->refcount == 0) {
> > -               err = xsk_get_mmap_offsets(umem->fd, &off);
> > -               if (!err) {
> > -                       munmap(ctx->fill->ring - off.fr.desc,
> > -                              off.fr.desc + umem->config.fill_size *
> > -                              sizeof(__u64));
> > -                       munmap(ctx->comp->ring - off.cr.desc,
> > -                              off.cr.desc + umem->config.comp_size *
> > -                              sizeof(__u64));
> > +               if (unmap) {
> > +                       err = xsk_get_mmap_offsets(umem->fd, &off);
> > +                       if (!err) {
> > +                               munmap(ctx->fill->ring - off.fr.desc,
> > +                                      off.fr.desc + umem->config.fill_size *
> > +                               sizeof(__u64));
> > +                               munmap(ctx->comp->ring - off.cr.desc,
> > +                                      off.cr.desc + umem->config.comp_size *
> > +                               sizeof(__u64));
> > +                       }
>
> The whole function increases indent, since it changes anyway
> could you write it as:
> {
> if (--ctx->refcount)
>   return;
> if (!unmap)
>   goto out_free;
> err = xsk_get
> if (err)
>  goto out_free;
> munmap();
> out_free:
> list_del
> free
> }
>

Yes, please try to reduce the nesting, and while at it try to expand
the as much as possible of the munmap arguments to the full 100 chars.


Björn