netdev - Re: [PATCH bpf] bpf: fix bpffs non-array map seq

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: <c8c3b728-adf7-77d8-70a9-f6c9684e771d@iogearbox.net>
Date:   Thu, 9 Aug 2018 16:24:06 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yonghong Song <yhs@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     ast@...com, netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH bpf] bpf: fix bpffs non-array map seq_show issue

On 08/09/2018 05:55 AM, Yonghong Song wrote:
> On 8/8/18 7:25 PM, Alexei Starovoitov wrote:
>> On Wed, Aug 08, 2018 at 06:25:19PM -0700, Yonghong Song wrote:
>>> In function map_seq_next() of kernel/bpf/inode.c,
>>> the first key will be the "0" regardless of the map type.
>>> This works for array. But for hash type, if it happens
>>> key "0" is in the map, the bpffs map show will miss
>>> some items if the key "0" is not the first element of
>>> the first bucket.
>>>
>>> This patch fixed the issue by guaranteeing to get
>>> the first element, if the seq_show is just started,
>>> by passing NULL pointer key to map_get_next_key() callback.
>>> This way, no missing elements will occur for
>>> bpffs hash table show even if key "0" is in the map.
> 
> Currently, map_seq_show_elem callback is only implemented
> for arraymap. So the problem actually is not exposed.
> 
> The issue is discovered when I tried to implement
> map_seq_show_elem for hash maps, and I will have followup
> patches for it.
> 
> So this patch probably should apply to bpf-next or
> I can include this patch in my later patch set
> which implements map_seq_show_elem for hash map
> which can demonstrate the problem.
> 
> Please let me know.
> 
>>> Fixes: a26ca7c982cb5 ("bpf: btf: Add pretty print support to the basic arraymap")
>>> Signed-off-by: Yonghong Song <yhs@...com>
>>
>> Acked-by: Alexei Starovoitov <ast@...nel.org>

Given this doesn't affect any current code, I think bpf-next
would be fine.

Anyway, this cannot be used as-is, results in following compile
warning ...

# make -j4 kernel/bpf/
  DESCEND  objtool
  CALL    scripts/checksyscalls.sh
  CC      kernel/bpf/verifier.o
  CC      kernel/bpf/inode.o
kernel/bpf/inode.c: In function ‘map_seq_next’:
kernel/bpf/inode.c:214:1: warning: label ‘done’ defined but not used [-Wunused-label]
 done:
 ^~~~
  AR      kernel/bpf/built-in.a