netdev - Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse

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: <8389fcac-0e05-71c2-707f-447260b8bf91@iogearbox.net>
Date:   Tue, 10 Jul 2018 20:16:46 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrey Ignatov <rdna@...com>
Cc:     alexei.starovoitov@...il.com, oss-drivers@...ronome.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse

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@...ronome.com> [Mon, 2018-07-09 19:49 -0700]:
>>> On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:  
>>>> Jakub Kicinski <jakub.kicinski@...ronome.com> [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!

Ok, I've tossed the v2 series from patchwork and waiting for your respin
in that case.

Thanks,
Daniel