From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yonghong Song Subject: Re: [PATCH bpf] bpf: fix bpffs non-array map seq_show issue Date: Thu, 9 Aug 2018 08:15:37 -0700 Message-ID: References: <20180809012519.3534824-1-yhs@fb.com> <20180809022530.yprizizentv7frmt@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: , , To: Daniel Borkmann , Alexei Starovoitov Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:47572 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731793AbeHIRlb (ORCPT ); Thu, 9 Aug 2018 13:41:31 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 8/9/18 7:24 AM, Daniel Borkmann wrote: > 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 >>> >>> Acked-by: Alexei Starovoitov > > 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 ... Thanks and will fix the problem and resubmit the patch set to bpf-next. > > # 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 >