From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrey Ignatov <rdna@fb.com>,
alexei.starovoitov@gmail.com, oss-drivers@netronome.com,
netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse
Date: Tue, 10 Jul 2018 13:58:13 -0700 [thread overview]
Message-ID: <20180710135813.65ad0d7b@cakuba.lan> (raw)
In-Reply-To: <8389fcac-0e05-71c2-707f-447260b8bf91@iogearbox.net>
On Tue, 10 Jul 2018 20:16:46 +0200, Daniel Borkmann wrote:
> On 07/10/2018 08:09 PM, Jakub Kicinski wrote:
> > On Mon, 9 Jul 2018 21:23:20 -0700, Andrey Ignatov wrote:
> >> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 19:49 -0700]:
> >>> On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:
> >>>> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> >>>>> + map->fd = dup(fd);
> >>>>
> >>>> Unfortunately, new descriptor created by dup(2) will not have O_CLOEXEC set, in
> >>>> contrast to original fd returned by kernel on map creation.
> >>>>
> >>>> libbpf has other interface shortcomings where it comes up. E.g. struct
> >>>> bpf_object owns all descriptors it contains (progs, maps) and closes them in
> >>>> bpf_object__close(). if one wants to open/load ELF, then close it but
> >>>> keep, say, prog fd to attach it to cgroup some time later, then fd
> >>>> should be duplicated as well to get a new one not owned by bpf_object.
> >>>>
> >>>> Currently I use this workaround to avoid time when new fd doesn't have
> >>>> O_CLOEXEC:
> >>>>
> >>>> int new_prog_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> >>>> if (new_prog_fd < 0 ||
> >>>> dup3(bpf_program__fd(prog), new_prog_fd, O_CLOEXEC) == -1) {
> >>>> /* .. handle error .. */
> >>>> close(new_prog_fd);
> >>>> }
> >>>> /* .. use new_prog_fd with O_CLOEXEC set */
> >>>>
> >>>> Not sure how to simplify it. dup2() has same problem with regard to
> >>>> O_CLOEXEC.
> >>>>
> >>>> Use-case: standalone server application that uses libbpf and does
> >>>> fork()/execve() a lot.
> >>>
> >>> Good point! I have no better ideas. Although being slightly paranoid
> >>> I would perhaps use "/" instead of "/dev/null"? Shouldn't matter?
> >>
> >> No strong preferences, important thing is to create fd with O_CLOEXEC
> >> set somehow.
> >>
> >> Is it safer to use "/" than "/dev/null"? (trying to understand if I
> >> should change my code as well)
> >
> > IDK :) Could there be a crazy scenario when someone runs chroot or a
> > very broken system without /dev/null? / should always be there?
> >
> > Thanks for all the other reviews, I will update the code accordingly!
Oh no, dup3() is under _GNU_SOURCE as well. libbpf kind of depends
on XSI-compliant strerror_r() semantics, I think I'll have to move the
strerror_r()-dependent code out of the libbpf.c file :(
> Ok, I've tossed the v2 series from patchwork and waiting for your respin
> in that case.
Yes, thank you!
next prev parent reply other threads:[~2018-07-10 21:30 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-09 17:59 [PATCH bpf-next v2 00/12] tools: bpf: extend bpftool prog load Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 01/12] selftests/bpf: remove duplicated word from test offloads Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 02/12] selftests/bpf: add Error: prefix in check_extack helper Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 03/12] tools: bpftool: refactor argument parsing for prog load Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 04/12] tools: bpftool: add support for loading programs for offload Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 05/12] tools: libbpf: expose the prog type guessing from section name logic Jakub Kicinski
2018-07-10 5:10 ` Andrey Ignatov
2018-07-09 17:59 ` [PATCH bpf-next v2 06/12] tools: bpftool: allow users to specify program type for prog load Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 07/12] tools: libbpf: recognize offload neutral maps Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 08/12] tools: libbpf: add extended attributes version of bpf_object__open() Jakub Kicinski
2018-07-10 5:39 ` Andrey Ignatov
2018-07-09 17:59 ` [PATCH bpf-next v2 09/12] tools: bpftool: reimplement bpf_prog_load() for prog load Jakub Kicinski
2018-07-09 19:40 ` Alexei Starovoitov
2018-07-09 17:59 ` [PATCH bpf-next v2 10/12] tools: bpf: make use of reallocarray Jakub Kicinski
2018-07-09 17:59 ` [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse Jakub Kicinski
2018-07-09 20:22 ` Andrey Ignatov
2018-07-10 2:49 ` Jakub Kicinski
2018-07-10 4:23 ` Andrey Ignatov
2018-07-10 18:09 ` Jakub Kicinski
2018-07-10 18:16 ` Daniel Borkmann
2018-07-10 20:58 ` Jakub Kicinski [this message]
2018-07-09 17:59 ` [PATCH bpf-next v2 12/12] tools: bpftool: allow reuse of maps with bpftool prog load Jakub Kicinski
2018-07-09 19:48 ` Alexei Starovoitov
2018-07-10 1:38 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180710135813.65ad0d7b@cakuba.lan \
--to=jakub.kicinski@netronome.com \
--cc=alexei.starovoitov@gmail.com \
--cc=daniel@iogearbox.net \
--cc=netdev@vger.kernel.org \
--cc=oss-drivers@netronome.com \
--cc=rdna@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox