From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH net] inetpeer: fix uninit-value in inet_getpeer Date: Mon, 9 Apr 2018 08:08:29 -0700 Message-ID: References: <20180409134327.22367-1-edumazet@google.com> <20180409.105839.445341256424332032.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, eric.dumazet@gmail.com To: David Miller , edumazet@google.com Return-path: Received: from mail-qt0-f195.google.com ([209.85.216.195]:39374 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751688AbeDIPId (ORCPT ); Mon, 9 Apr 2018 11:08:33 -0400 Received: by mail-qt0-f195.google.com with SMTP id v11so9529914qtj.6 for ; Mon, 09 Apr 2018 08:08:32 -0700 (PDT) In-Reply-To: <20180409.105839.445341256424332032.davem@davemloft.net> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 04/09/2018 07:58 AM, David Miller wrote: > From: Eric Dumazet > Date: Mon, 9 Apr 2018 06:43:27 -0700 > >> syzbot/KMSAN reported that p->dtime was read while it was >> not yet initialized in : >> >> delta = (__u32)jiffies - p->dtime; >> if (delta < ttl || !refcount_dec_if_one(&p->refcnt)) >> gc_stack[i] = NULL; >> >> This is a false positive, because the inetpeer wont be erased >> from rb-tree if the refcount_dec_if_one(&p->refcnt) does not >> succeed. And this wont happen before first inet_putpeer() call >> for this inetpeer has been done, and ->dtime field is written >> exactly before the refcount_dec_and_test(&p->refcnt). >> >> The KMSAN report was : > ... >> Signed-off-by: Eric Dumazet >> Reported-by: syzbot > > Applied, but it looks like we are just adding assignments simply > to placate these reports when the tools and facilities cannot > see through the logic properly. > To be fair, this is because the check on ->dtime should be done a second time after the refcount_dec_if_one(&p->refcnt) It is a tiny race, and we do not really care given nature of inetpeer cache, best effort, and DDOS candidate anyway. If we purge one entry too soon, this is not a big deal. I believe tool is fine.