From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 743A4C28CF6 for ; Wed, 1 Aug 2018 11:41:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2E9FA20862 for ; Wed, 1 Aug 2018 11:41:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E9FA20862 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=strlen.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388891AbeHAN0r (ORCPT ); Wed, 1 Aug 2018 09:26:47 -0400 Received: from Chamillionaire.breakpoint.cc ([146.0.238.67]:47042 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387610AbeHAN0q (ORCPT ); Wed, 1 Aug 2018 09:26:46 -0400 Received: from fw by Chamillionaire.breakpoint.cc with local (Exim 4.89) (envelope-from ) id 1fkpUm-0001QR-7g; Wed, 01 Aug 2018 13:40:48 +0200 Date: Wed, 1 Aug 2018 13:40:48 +0200 From: Florian Westphal To: Dmitry Vyukov Cc: Florian Westphal , Linus Torvalds , Christoph Lameter , Andrey Ryabinin , Theodore Ts'o , Jan Kara , linux-ext4@vger.kernel.org, Greg Kroah-Hartman , Pablo Neira Ayuso , Jozsef Kadlecsik , David Miller , NetFilter , coreteam@netfilter.org, Network Development , Gerrit Renker , dccp@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Dave Airlie , intel-gfx , DRI , Eric Dumazet , Alexey Kuznetsov , Hideaki YOSHIFUJI , Ursula Braun , linux-s390 , Linux Kernel Mailing List , Andrew Morton , linux-mm , Andrey Konovalov Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation) Message-ID: <20180801114048.ufkr7zwmir7ps3vl@breakpoint.cc> References: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> <20180801103537.d36t3snzulyuge7g@breakpoint.cc> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dmitry Vyukov wrote: > On Wed, Aug 1, 2018 at 12:35 PM, Florian Westphal wrote: > > Dmitry Vyukov wrote: > >> Still can't grasp all details. > >> There is state that we read without taking ct->ct_general.use ref > >> first, namely ct->state and what's used by nf_ct_key_equal. > >> So let's say the entry we want to find is in the list, but > >> ____nf_conntrack_find finds a wrong entry earlier because all state it > >> looks at is random garbage, so it returns the wrong entry to > >> __nf_conntrack_find_get. > > > > If an entry can be found, it can't be random garbage. > > We never link entries into global table until state has been set up. > > But... we don't hold a reference to the entry. So say it's in the > table with valid state, now ____nf_conntrack_find discovers it, now > the entry is removed and reused a dozen of times will all associated > state reinitialization. And nf_ct_key_equal looks at it concurrently > and decides if it's the entry we are looking for or now. I think > unless we hold a ref to the entry, it's state needs to be considered > random garbage for correctness reasoning. It checks if it might be the entry we're looking for. If this was complete random garbage, scheme would not work, as then we could have entry that isn't in table, has nonzero refcount, but has its confirmed bit set. I don't see how that would be possible, any reallocation makes sure ct->status has CONFIRMED bit clear before setting refcount to nonzero value. I think this is the scenario you hint at is: 1. nf_ct_key_equal is true 2. the entry is free'd (or was already free'd) 3. we return NULL to caller due to atomic_inc_not_zero() failure but i fail to see how thats wrong? Sure, we could restart lookup but how would that help? We'd not find the 'candidate' entry again. We might find entry that has been inserted at this very instant but newly allocated entries are only inserted into global table until the skb that created the nf_conn object has made it through the network stack (postrouting for fowarded, or input for local delivery). So, the likelyhood of such restart finding another candidate is close to 0, and won't prevent 'insert race' from happening. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Westphal Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation) Date: Wed, 1 Aug 2018 13:40:48 +0200 Message-ID: <20180801114048.ufkr7zwmir7ps3vl@breakpoint.cc> References: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> <20180801103537.d36t3snzulyuge7g@breakpoint.cc> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Westphal , Linus Torvalds , Christoph Lameter , Andrey Ryabinin , Theodore Ts'o , Jan Kara , linux-ext4@vger.kernel.org, Greg Kroah-Hartman , Pablo Neira Ayuso , Jozsef Kadlecsik , David Miller , NetFilter , coreteam@netfilter.org, Network Development , Gerrit Renker , dccp@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Dave Airlie , To: Dmitry Vyukov Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Dmitry Vyukov wrote: > On Wed, Aug 1, 2018 at 12:35 PM, Florian Westphal wrote: > > Dmitry Vyukov wrote: > >> Still can't grasp all details. > >> There is state that we read without taking ct->ct_general.use ref > >> first, namely ct->state and what's used by nf_ct_key_equal. > >> So let's say the entry we want to find is in the list, but > >> ____nf_conntrack_find finds a wrong entry earlier because all state it > >> looks at is random garbage, so it returns the wrong entry to > >> __nf_conntrack_find_get. > > > > If an entry can be found, it can't be random garbage. > > We never link entries into global table until state has been set up. > > But... we don't hold a reference to the entry. So say it's in the > table with valid state, now ____nf_conntrack_find discovers it, now > the entry is removed and reused a dozen of times will all associated > state reinitialization. And nf_ct_key_equal looks at it concurrently > and decides if it's the entry we are looking for or now. I think > unless we hold a ref to the entry, it's state needs to be considered > random garbage for correctness reasoning. It checks if it might be the entry we're looking for. If this was complete random garbage, scheme would not work, as then we could have entry that isn't in table, has nonzero refcount, but has its confirmed bit set. I don't see how that would be possible, any reallocation makes sure ct->status has CONFIRMED bit clear before setting refcount to nonzero value. I think this is the scenario you hint at is: 1. nf_ct_key_equal is true 2. the entry is free'd (or was already free'd) 3. we return NULL to caller due to atomic_inc_not_zero() failure but i fail to see how thats wrong? Sure, we could restart lookup but how would that help? We'd not find the 'candidate' entry again. We might find entry that has been inserted at this very instant but newly allocated entries are only inserted into global table until the skb that created the nf_conn object has made it through the network stack (postrouting for fowarded, or input for local delivery). So, the likelyhood of such restart finding another candidate is close to 0, and won't prevent 'insert race' from happening. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Westphal Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation) Date: Wed, 1 Aug 2018 13:40:48 +0200 Message-ID: <20180801114048.ufkr7zwmir7ps3vl@breakpoint.cc> References: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> <20180801103537.d36t3snzulyuge7g@breakpoint.cc> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Dmitry Vyukov Cc: Florian Westphal , Linus Torvalds , Christoph Lameter , Andrey Ryabinin , Theodore Ts'o , Jan Kara , linux-ext4@vger.kernel.org, Greg Kroah-Hartman , Pablo Neira Ayuso , Jozsef Kadlecsik , David Miller , NetFilter , coreteam@netfilter.org, Network Development , Gerrit Renker , dccp@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Dave Airlie List-Id: dri-devel@lists.freedesktop.org Dmitry Vyukov wrote: > On Wed, Aug 1, 2018 at 12:35 PM, Florian Westphal wrote: > > Dmitry Vyukov wrote: > >> Still can't grasp all details. > >> There is state that we read without taking ct->ct_general.use ref > >> first, namely ct->state and what's used by nf_ct_key_equal. > >> So let's say the entry we want to find is in the list, but > >> ____nf_conntrack_find finds a wrong entry earlier because all state it > >> looks at is random garbage, so it returns the wrong entry to > >> __nf_conntrack_find_get. > > > > If an entry can be found, it can't be random garbage. > > We never link entries into global table until state has been set up. > > But... we don't hold a reference to the entry. So say it's in the > table with valid state, now ____nf_conntrack_find discovers it, now > the entry is removed and reused a dozen of times will all associated > state reinitialization. And nf_ct_key_equal looks at it concurrently > and decides if it's the entry we are looking for or now. I think > unless we hold a ref to the entry, it's state needs to be considered > random garbage for correctness reasoning. It checks if it might be the entry we're looking for. If this was complete random garbage, scheme would not work, as then we could have entry that isn't in table, has nonzero refcount, but has its confirmed bit set. I don't see how that would be possible, any reallocation makes sure ct->status has CONFIRMED bit clear before setting refcount to nonzero value. I think this is the scenario you hint at is: 1. nf_ct_key_equal is true 2. the entry is free'd (or was already free'd) 3. we return NULL to caller due to atomic_inc_not_zero() failure but i fail to see how thats wrong? Sure, we could restart lookup but how would that help? We'd not find the 'candidate' entry again. We might find entry that has been inserted at this very instant but newly allocated entries are only inserted into global table until the skb that created the nf_conn object has made it through the network stack (postrouting for fowarded, or input for local delivery). So, the likelyhood of such restart finding another candidate is close to 0, and won't prevent 'insert race' from happening. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Westphal Date: Wed, 01 Aug 2018 11:40:48 +0000 Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implement Message-Id: <20180801114048.ufkr7zwmir7ps3vl@breakpoint.cc> List-Id: References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: dccp@vger.kernel.org Dmitry Vyukov wrote: > On Wed, Aug 1, 2018 at 12:35 PM, Florian Westphal wrote: > > Dmitry Vyukov wrote: > >> Still can't grasp all details. > >> There is state that we read without taking ct->ct_general.use ref > >> first, namely ct->state and what's used by nf_ct_key_equal. > >> So let's say the entry we want to find is in the list, but > >> ____nf_conntrack_find finds a wrong entry earlier because all state it > >> looks at is random garbage, so it returns the wrong entry to > >> __nf_conntrack_find_get. > > > > If an entry can be found, it can't be random garbage. > > We never link entries into global table until state has been set up. > > But... we don't hold a reference to the entry. So say it's in the > table with valid state, now ____nf_conntrack_find discovers it, now > the entry is removed and reused a dozen of times will all associated > state reinitialization. And nf_ct_key_equal looks at it concurrently > and decides if it's the entry we are looking for or now. I think > unless we hold a ref to the entry, it's state needs to be considered > random garbage for correctness reasoning. It checks if it might be the entry we're looking for. If this was complete random garbage, scheme would not work, as then we could have entry that isn't in table, has nonzero refcount, but has its confirmed bit set. I don't see how that would be possible, any reallocation makes sure ct->status has CONFIRMED bit clear before setting refcount to nonzero value. I think this is the scenario you hint at is: 1. nf_ct_key_equal is true 2. the entry is free'd (or was already free'd) 3. we return NULL to caller due to atomic_inc_not_zero() failure but i fail to see how thats wrong? Sure, we could restart lookup but how would that help? We'd not find the 'candidate' entry again. We might find entry that has been inserted at this very instant but newly allocated entries are only inserted into global table until the skb that created the nf_conn object has made it through the network stack (postrouting for fowarded, or input for local delivery). So, the likelyhood of such restart finding another candidate is close to 0, and won't prevent 'insert race' from happening.