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.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 0B0D6C4363C for ; Thu, 8 Oct 2020 06:42:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B9B272087D for ; Thu, 8 Oct 2020 06:42:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728092AbgJHGmn (ORCPT ); Thu, 8 Oct 2020 02:42:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40848 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726013AbgJHGmn (ORCPT ); Thu, 8 Oct 2020 02:42:43 -0400 Received: from sipsolutions.net (s3.sipsolutions.net [IPv6:2a01:4f8:191:4433::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 38567C061755 for ; Wed, 7 Oct 2020 23:42:43 -0700 (PDT) Received: by sipsolutions.net with esmtpsa (TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.94) (envelope-from ) id 1kQPdP-001Rfm-6L; Thu, 08 Oct 2020 08:42:39 +0200 Message-ID: <1545e7c77675c9a0574a7582ee5f0c969c01419e.camel@sipsolutions.net> Subject: Re: [PATCH v2 04/11] drivers/base/devcoredump: convert devcd_count to counter_atomic32 From: Johannes Berg To: Kees Cook Cc: Shuah Khan , gregkh@linuxfoundation.org, rafael@kernel.org, linux-kernel@vger.kernel.org Date: Thu, 08 Oct 2020 08:42:22 +0200 In-Reply-To: <202010071334.8298F3FA7@keescook> (sfid-20201007_224326_651899_546CB035) References: <462fd514dfe2afbb8faa1dea4cdb4b0e75d8e8da.1602011710.git.skhan@linuxfoundation.org> <202010071114.EE9A2A47C@keescook> <202010071334.8298F3FA7@keescook> (sfid-20201007_224326_651899_546CB035) Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5 (3.36.5-1.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2020-10-07 at 13:43 -0700, Kees Cook wrote: > > > > I actually wonder if this should use refcount_t just because it is > > > > designed to be an alway-unique value. It is hard to imagine ever causing > > > > this to overflow, but why not let it be protected? > > > > > > > > > > This is one of the cases where devcd_count doesn't guard lifetimes, > > > however if it ever overflows, refcount_t is a better choice. > > > > > > If we decide refcount_t is a better choice, I can drop this patch > > > and send refcount_t conversion patch instead. > > > > > > Greg! Any thoughts on refcount_t for this being a better choice? > > > > I'm not Greg, but ... there's a 5 minute timeout. So in order to cause a > > clash you'd have to manage to overflow the counter within a 5 minute > > interval, otherwise you can actually reuse the numbers starting again > > from 0 without any ill effect. > > That's not true as far as I can see: there's no reset in here. It's a > global heap variable with function-level visibility (note the "static"), > so it is only ever initialized once: Yes, obviously it is a static variable. You'll note that I also never claimed anything regarding reset. What I said was two things (perhaps with too many words :-) ): 1) each value that we derive from this ever-incrementing (modulo 2^32) variable only get used for a limited amount of time (max. 5 minutes) 2) if you manage to overflow within 5 minutes, then the following device_add() will just fail and nothing else/bad will happen Therefore, there's no problem with wrapping, and IMHO it'd be *better* than saturating because (1) means that the wrapping almost certainly doesn't matter, and (2) means that even if you do manage to wrap and cause a "clash" (what I wrote in the text you quoted) this is entirely harmless. OTOH, if you saturate, then - again under the premise of actually getting there, however unlikely it may be - you are afterwards *always* hitting (2), regardless of (1), which seems counter-productive given that (1) means that (2) almost certainly won't happen. IOW, I disagree with you, and think that counter_atomic_32 is more appropriate here than refcount_t. johannes