From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case Date: Thu, 5 Jul 2018 07:41:29 -0700 Message-ID: References: <20180630134148.6395.64795.stgit@john-Precision-Tower-5810> <20180630135131.6395.86522.stgit@john-Precision-Tower-5810> <2a083803-c830-b911-b8fd-d4dd0f560b56@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, kafai@fb.com To: Daniel Borkmann , ast@kernel.org Return-path: Received: from mail-it0-f68.google.com ([209.85.214.68]:36344 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753800AbeGEOll (ORCPT ); Thu, 5 Jul 2018 10:41:41 -0400 Received: by mail-it0-f68.google.com with SMTP id j185-v6so12394662ite.1 for ; Thu, 05 Jul 2018 07:41:41 -0700 (PDT) In-Reply-To: <2a083803-c830-b911-b8fd-d4dd0f560b56@iogearbox.net> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 07/03/2018 07:40 AM, Daniel Borkmann wrote: > On 06/30/2018 03:51 PM, John Fastabend wrote: >> The current code, in the error path of sock_hash_ctx_update_elem, >> checks if the sock has a psock in the user data and if so decrements >> the reference count of the psock. However, if the error happens early >> in the error path we may have never incremented the psock reference >> count and if the psock exists because the sock is in another map then >> we may inadvertently decrement the reference count. >> >> Fix this by making the error path only call smap_release_sock if the >> error happens after the increment. >> >> Reported-by: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com >> Fixes: 81110384441a ("bpf: sockmap, add hash map support") >> Signed-off-by: John Fastabend >> --- [...] >> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops, >> if (err) >> goto err; >> >> - /* bpf_map_update_elem() can be called in_irq() */ >> + psock = smap_psock_sk(sock); >> + if (unlikely(!psock)) { >> + err = -EINVAL; >> + goto err; >> + } > > Is an error even possible at this point? If __sock_map_ctx_update_elem() succeeds, > we either allocated and linked a new psock to the sock or we inc'ed the existing > one's refcount. From my reading it seems we should always succeed the subsequent > smap_psock_sk(). If we would have failed here in between it would mean we'd have > a refcount imbalance somewhere? > Its not possible will replace with a comment. Thanks.