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=-1.0 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,T_DKIMWL_WL_MED 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 E3C33C43142 for ; Tue, 31 Jul 2018 17:36:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 97E52208A4 for ; Tue, 31 Jul 2018 17:36:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=amazonses.com header.i=@amazonses.com header.b="S6ySXpXv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 97E52208A4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux.com 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 S1731427AbeGaTR6 (ORCPT ); Tue, 31 Jul 2018 15:17:58 -0400 Received: from a9-92.smtp-out.amazonses.com ([54.240.9.92]:43154 "EHLO a9-92.smtp-out.amazonses.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728327AbeGaTR6 (ORCPT ); Tue, 31 Jul 2018 15:17:58 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=ug7nbtf4gccmlpwj322ax3p6ow6yfsug; d=amazonses.com; t=1533058596; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References:MIME-Version:Content-Type:Feedback-ID; bh=90HqWVTWMOjsJBf0Cfr1/WHlmUJGQ1TYHIcGVJupAFo=; b=S6ySXpXvndgph7eN1SPyT4oAf9N4Hbeq4QzGwi866kIXOXqqJmcFkTl8IQlGfuot HdHIsMhCdcqY8kfWmLbcc5FmCcuqdDI/yq+4F9pEhleQMmAVXKfp/lS2B7sZo/P7e7v /rxdpCFawSdaaq5wIyA6FwY+jeaL6qvVxUKblDPo= Date: Tue, 31 Jul 2018 17:36:36 +0000 From: Christopher Lameter X-X-Sender: cl@nuc-kabylake 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 , 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) In-Reply-To: Message-ID: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> References: User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SES-Outgoing: 2018.07.31-54.240.9.92 Feedback-ID: 1.us-east-1.fQZZZ0Xtj2+TD7V5apTT/NrT6QKuPgzCT/IC7XYgDKI=:AmazonSES Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 31 Jul 2018, 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? ct may still be read by another cpu in a RCU section but the object was freed elsewhere so no other processor may modify the object. The lock must have been released before freeing the slab object and thus the initialization of the spinlock is unnecessary if it was initialized in ctor. If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Lameter Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation) Date: Tue, 31 Jul 2018 17:36:36 +0000 Message-ID: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> 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: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Tue, 31 Jul 2018, 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? ct may still be read by another cpu in a RCU section but the object was freed elsewhere so no other processor may modify the object. The lock must have been released before freeing the slab object and thus the initialization of the spinlock is unnecessary if it was initialized in ctor. If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Lameter Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation) Date: Tue, 31 Jul 2018 17:36:36 +0000 Message-ID: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Return-path: 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 On Tue, 31 Jul 2018, 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? ct may still be read by another cpu in a RCU section but the object was freed elsewhere so no other processor may modify the object. The lock must have been released before freeing the slab object and thus the initialization of the spinlock is unnecessary if it was initialized in ctor. If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Lameter Date: Tue, 31 Jul 2018 17:36:36 +0000 Subject: Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implement Message-Id: <01000164f169bc6b-c73a8353-d7d9-47ec-a782-90aadcb86bfb-000000@email.amazonses.com> 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 On Tue, 31 Jul 2018, 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? ct may still be read by another cpu in a RCU section but the object was freed elsewhere so no other processor may modify the object. The lock must have been released before freeing the slab object and thus the initialization of the spinlock is unnecessary if it was initialized in ctor. If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?