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 11:09:06 -0700 Message-ID: <20180710110906.48df8981@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> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: , , , To: Andrey Ignatov Return-path: Received: from mail-pl0-f68.google.com ([209.85.160.68]:39923 "EHLO mail-pl0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388650AbeGJSfB (ORCPT ); Tue, 10 Jul 2018 14:35:01 -0400 Received: by mail-pl0-f68.google.com with SMTP id s24-v6so7991325plq.6 for ; Tue, 10 Jul 2018 11:34:48 -0700 (PDT) In-Reply-To: <20180710042319.GA77322@rdna-mbp.dhcp.thefacebook.com> Sender: netdev-owner@vger.kernel.org List-ID: 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]: > > > fd of every map is set to -1 in bpf_object__init_maps() that, in turn, is > > > called from __bpf_object__open(): > > > > > > for (i = 0; i < nr_maps; i++) > > > obj->maps[i].fd = -1; > > > > > > Later it will either contain valid fd that is >= 0, or that same -1, what > > > should be enough to identify fd presence. > > > > I thought it to be cleaner to indicate the fd has been pre-set, in case > > things get more complicated in the future and fd >= 0 becomes ambiguous. > > > > But no strong preference, should I change? > > My preference (not strong either) is to avoid a new field whenever it's > possible. Though if you have a use-case that can't be covered by > (fd >= 0) keeping the field is fine as well. Okay, I can change. I think it may be worthwhile keeping the information that the map definition was replaced by something that did not come from the ELF file, but I don't actually have a use for it right now, so we can just add it back later :) > > > > + 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!