From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse Date: Tue, 10 Jul 2018 13:58:13 -0700 Message-ID: <20180710135813.65ad0d7b@cakuba.lan> References: <20180709175944.32265-1-jakub.kicinski@netronome.com> <20180709175944.32265-12-jakub.kicinski@netronome.com> <20180709202254.GA32302@rdna-mbp.dhcp.thefacebook.com> <20180709194914.003ef519@cakuba.lan> <20180710042319.GA77322@rdna-mbp.dhcp.thefacebook.com> <20180710110906.48df8981@cakuba.lan> <8389fcac-0e05-71c2-707f-447260b8bf91@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Andrey Ignatov , alexei.starovoitov@gmail.com, oss-drivers@netronome.com, netdev@vger.kernel.org To: Daniel Borkmann Return-path: Received: from mail-qt0-f172.google.com ([209.85.216.172]:45234 "EHLO mail-qt0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732205AbeGJVaG (ORCPT ); Tue, 10 Jul 2018 17:30:06 -0400 Received: by mail-qt0-f172.google.com with SMTP id y5-v6so19684829qti.12 for ; Tue, 10 Jul 2018 14:29:08 -0700 (PDT) In-Reply-To: <8389fcac-0e05-71c2-707f-447260b8bf91@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: 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 [Mon, 2018-07-09 19:49 -0700]: > >>> On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote: > >>>> Jakub Kicinski [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!