From mboxrd@z Thu Jan 1 00:00:00 1970 From: Prashant Bhole Subject: Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported Date: Thu, 20 Sep 2018 11:34:30 +0900 Message-ID: References: <20180919075143.9308-1-bhole_prashant_q7@lab.ntt.co.jp> <20180919075143.9308-3-bhole_prashant_q7@lab.ntt.co.jp> <20180919151422.tysdcku2hxaq37xw@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Alexei Starovoitov , Daniel Borkmann , Jakub Kicinski , Quentin Monnet , "David S . Miller" , netdev@vger.kernel.org To: Mauricio Vasquez , Alexei Starovoitov Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:52568 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726000AbeITIQN (ORCPT ); Thu, 20 Sep 2018 04:16:13 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 9/20/2018 3:40 AM, Mauricio Vasquez wrote: > > > On 09/19/2018 10:14 AM, Alexei Starovoitov wrote: >> On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote: >>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below >>> map types: >>> - BPF_MAP_TYPE_PROG_ARRAY >>> - BPF_MAP_TYPE_STACK_TRACE >>> - BPF_MAP_TYPE_XSKMAP >>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH >>> >>> Signed-off-by: Prashant Bhole >>> --- >>>   kernel/bpf/arraymap.c | 2 +- >>>   kernel/bpf/sockmap.c  | 2 +- >>>   kernel/bpf/stackmap.c | 2 +- >>>   kernel/bpf/xskmap.c   | 2 +- >>>   4 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c >>> index dded84cbe814..24583da9ffd1 100644 >>> --- a/kernel/bpf/arraymap.c >>> +++ b/kernel/bpf/arraymap.c >>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map) >>>   static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key) >>>   { >>> -    return NULL; >>> +    return ERR_PTR(-EOPNOTSUPP); >>>   } >> conceptually the set looks good to me. >> Please add a test to test_verifier.c to make sure >> that these lookup helpers cannot be called from BPF program. >> Otherwise this diff may cause crashes. >> >> > I think we can remove all these stub functions that return EOPNOTSUPP or > EINVAL and return the error in syscall.c if ops->map_[update, delete, > lookup, ...] is null. > This will require to extend (and test) the verifier to guarantee that > those helpers are not called in wrong maps, for example > map_delete_element in array like maps. > > Would it make sense? Thanks for review and suggestion. I had thought about this way too (except adding restrictions in the verifier). There is no strong reason for choosing current implementation. I thought there must be some reason that those methods are implemented and just returning NULL. Also there are no NULL checks for map_lookup_elem stub. So I decided to simply change the return value. If some more people agree with removing stub function idea, I will do it. -Prashant