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,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 30696C43142 for ; Tue, 31 Jul 2018 17:10:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E587B20862 for ; Tue, 31 Jul 2018 17:10:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E587B20862 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 S1731760AbeGaSwG (ORCPT ); Tue, 31 Jul 2018 14:52:06 -0400 Received: from Chamillionaire.breakpoint.cc ([146.0.238.67]:43618 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727269AbeGaSwG (ORCPT ); Tue, 31 Jul 2018 14:52:06 -0400 Received: from fw by Chamillionaire.breakpoint.cc with local (Exim 4.89) (envelope-from ) id 1fkY9l-0004Cl-I8; Tue, 31 Jul 2018 19:09:57 +0200 Date: Tue, 31 Jul 2018 19:09:57 +0200 From: Florian Westphal To: Andrey Ryabinin Cc: Theodore Ts'o , Jan Kara , linux-ext4@vger.kernel.org, Greg Kroah-Hartman , Pablo Neira Ayuso , Jozsef Kadlecsik , Florian Westphal , "David S. Miller" , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, Gerrit Renker , dccp@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Eric Dumazet , Alexey Kuznetsov , Hideaki YOSHIFUJI , Ursula Braun , linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, Dmitry Vyukov , Christoph Lameter , Andrew Morton , "linux-mm@kvack.org" , Andrey Konovalov , Linus Torvalds Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation) Message-ID: <20180731170957.o4vhopmzgedpo5sh@breakpoint.cc> References: 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 Andrey Ryabinin wrote: > Guys, it seems that we have a lot of code using SLAB_TYPESAFE_BY_RCU cache without constructor. > I think it's nearly impossible to use that combination without having bugs. > It's either you don't really need the SLAB_TYPESAFE_BY_RCU, or you need to have a constructor in kmem_cache. > > Could you guys, please, verify your code if it's really need SLAB_TYPSAFE or constructor? > > E.g. the netlink code look extremely suspicious: > > /* > * Do not use kmem_cache_zalloc(), as this cache uses > * SLAB_TYPESAFE_BY_RCU. > */ > ct = kmem_cache_alloc(nf_conntrack_cachep, gfp); > if (ct == NULL) > goto out; > > spin_lock_init(&ct->lock); > > If nf_conntrack_cachep objects really used in rcu typesafe manner, than 'ct' returned by kmem_cache_alloc might still be > in use by another cpu. So we just reinitialize spin_lock used by someone else? That would be a bug, nf_conn objects are reference counted. spinlock can only be used after object had its refcount incremented. lookup operation on nf_conn object: 1. compare keys 2. attempt to obtain refcount (using _not_zero version) 3. compare keys again after refcount was obtained if any of that fails, nf_conn candidate is skipped. 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: Tue, 31 Jul 2018 19:09:57 +0200 Message-ID: <20180731170957.o4vhopmzgedpo5sh@breakpoint.cc> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Theodore Ts'o , Jan Kara , linux-ext4@vger.kernel.org, Greg Kroah-Hartman , Pablo Neira Ayuso , Jozsef Kadlecsik , Florian Westphal , "David S. Miller" , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, Gerrit Renker , dccp@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Eric Dumazet , Alexey Kuznetsov , Hideaki YOSHIFUJI Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Andrey Ryabinin wrote: > Guys, it seems that we have a lot of code using SLAB_TYPESAFE_BY_RCU cache without constructor. > I think it's nearly impossible to use that combination without having bugs. > It's either you don't really need the SLAB_TYPESAFE_BY_RCU, or you need to have a constructor in kmem_cache. > > Could you guys, please, verify your code if it's really need SLAB_TYPSAFE or constructor? > > E.g. the netlink code look extremely suspicious: > > /* > * Do not use kmem_cache_zalloc(), as this cache uses > * SLAB_TYPESAFE_BY_RCU. > */ > ct = kmem_cache_alloc(nf_conntrack_cachep, gfp); > if (ct == NULL) > goto out; > > spin_lock_init(&ct->lock); > > If nf_conntrack_cachep objects really used in rcu typesafe manner, than 'ct' returned by kmem_cache_alloc might still be > in use by another cpu. So we just reinitialize spin_lock used by someone else? That would be a bug, nf_conn objects are reference counted. spinlock can only be used after object had its refcount incremented. lookup operation on nf_conn object: 1. compare keys 2. attempt to obtain refcount (using _not_zero version) 3. compare keys again after refcount was obtained if any of that fails, nf_conn candidate is skipped. 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: Tue, 31 Jul 2018 19:09:57 +0200 Message-ID: <20180731170957.o4vhopmzgedpo5sh@breakpoint.cc> References: 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: Andrey Ryabinin Cc: Theodore Ts'o , Jan Kara , linux-ext4@vger.kernel.org, Greg Kroah-Hartman , Pablo Neira Ayuso , Jozsef Kadlecsik , Florian Westphal , "David S. Miller" , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, Gerrit Renker , dccp@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Eric Dumazet , Alexey Kuznetsov , Hideaki YOSHIFUJI List-Id: dri-devel@lists.freedesktop.org Andrey Ryabinin wrote: > Guys, it seems that we have a lot of code using SLAB_TYPESAFE_BY_RCU cache without constructor. > I think it's nearly impossible to use that combination without having bugs. > It's either you don't really need the SLAB_TYPESAFE_BY_RCU, or you need to have a constructor in kmem_cache. > > Could you guys, please, verify your code if it's really need SLAB_TYPSAFE or constructor? > > E.g. the netlink code look extremely suspicious: > > /* > * Do not use kmem_cache_zalloc(), as this cache uses > * SLAB_TYPESAFE_BY_RCU. > */ > ct = kmem_cache_alloc(nf_conntrack_cachep, gfp); > if (ct == NULL) > goto out; > > spin_lock_init(&ct->lock); > > If nf_conntrack_cachep objects really used in rcu typesafe manner, than 'ct' returned by kmem_cache_alloc might still be > in use by another cpu. So we just reinitialize spin_lock used by someone else? That would be a bug, nf_conn objects are reference counted. spinlock can only be used after object had its refcount incremented. lookup operation on nf_conn object: 1. compare keys 2. attempt to obtain refcount (using _not_zero version) 3. compare keys again after refcount was obtained if any of that fails, nf_conn candidate is skipped. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Westphal Date: Tue, 31 Jul 2018 17:09:57 +0000 Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implement Message-Id: <20180731170957.o4vhopmzgedpo5sh@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 Andrey Ryabinin wrote: > Guys, it seems that we have a lot of code using SLAB_TYPESAFE_BY_RCU cache without constructor. > I think it's nearly impossible to use that combination without having bugs. > It's either you don't really need the SLAB_TYPESAFE_BY_RCU, or you need to have a constructor in kmem_cache. > > Could you guys, please, verify your code if it's really need SLAB_TYPSAFE or constructor? > > E.g. the netlink code look extremely suspicious: > > /* > * Do not use kmem_cache_zalloc(), as this cache uses > * SLAB_TYPESAFE_BY_RCU. > */ > ct = kmem_cache_alloc(nf_conntrack_cachep, gfp); > if (ct = NULL) > goto out; > > spin_lock_init(&ct->lock); > > If nf_conntrack_cachep objects really used in rcu typesafe manner, than 'ct' returned by kmem_cache_alloc might still be > in use by another cpu. So we just reinitialize spin_lock used by someone else? That would be a bug, nf_conn objects are reference counted. spinlock can only be used after object had its refcount incremented. lookup operation on nf_conn object: 1. compare keys 2. attempt to obtain refcount (using _not_zero version) 3. compare keys again after refcount was obtained if any of that fails, nf_conn candidate is skipped.