Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp6185246imu; Mon, 21 Jan 2019 04:44:19 -0800 (PST) X-Google-Smtp-Source: ALg8bN5BeuVvi+jWuUdxP1blsCOL5+WzQDgwQ7or3oYxrh7BTkYmdbH95Q3Vlt0slemfnnfGnnlB X-Received: by 2002:a62:509b:: with SMTP id g27mr29833801pfj.48.1548074659004; Mon, 21 Jan 2019 04:44:19 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1548074658; cv=none; d=google.com; s=arc-20160816; b=yX5zcT2Te9jUUp2D9xvn1TRfF6v5w69JioPGLqO3rRBH6OHMOicn/qdyUHntt1iyf/ zrXctbQz1k5SOHLdoc/LUvLmFSzzOh0S0j22UPdvogCWW7FLegriTsGHJlfMv42R1hNB 0WXoAd13JbFGmQwGBscTGPCLrXhvAckP7RV7O8a+OWjXWV4x1weTNh5bc31lukrhVg9I RJD0B8BGoYOh4xclYhxJZTYg4rE36zAZjyIWpLMOFROpaYrD7QV+x4cn5UUsNMfJOBZt FALGR+FqT5354nDRcXVgy/fjciBzqLNiNECdVN0sgmvwfjjnZitO4OWxnDe2v0g8/F0f ClUg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=qLCAMNY8ip350mbuKSMyKuBtoSJ499QiUIVKiUAfvRU=; b=aJyW1qRL+MLh+spOeN2aYRCbNHSaDX66StI8E3qhjxG2kw/RpH4AaTbcJ6suBurFvH ekUZkv3SmNkPG2Lns+Ms7yG7HDytJjpFLdU0PvLvuxE8P+nWydz8VwczDn4lVA7b+y6X cXi3G2t5N+oPXNE2SiZDJx4DNreHDse9zhq2oJTshlevMDp016jTv6FRqkuWAE9tZ0NX jbYsGdQAlg5iwPRhnhVsDEUr8MKh02jtvuIjZJmWnr1H+pM1qQHisBCz+1ECyUkOoB5I xOnV4po5ibPr109ZsV7Jqb21NGGgPeQKkA0z7u/7hiQ8RErITT8HUay4viEp94Atxrto vzFQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=aMEO1IhS; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b34si12888722pld.305.2019.01.21.04.44.03; Mon, 21 Jan 2019 04:44:18 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=aMEO1IhS; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728573AbfAUMm4 (ORCPT + 99 others); Mon, 21 Jan 2019 07:42:56 -0500 Received: from mail-it1-f195.google.com ([209.85.166.195]:55453 "EHLO mail-it1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728163AbfAUMm4 (ORCPT ); Mon, 21 Jan 2019 07:42:56 -0500 Received: by mail-it1-f195.google.com with SMTP id m62so16175914ith.5 for ; Mon, 21 Jan 2019 04:42:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qLCAMNY8ip350mbuKSMyKuBtoSJ499QiUIVKiUAfvRU=; b=aMEO1IhSZvD26ww6BhGKkmBr2Sc4RiOmJuZuX6s5Y5x0sh3SHFJiETs0j4frdA9BGK mwAG1DmARyDcloVIcKDsjg5tAnRhAvgDo5VnzQjU2wLHPuI75DvXfaePl+//8n2NlDRQ ZJBPQyhboiAzNxHp/1p33QFUVWp6p9BtO+CyRbSRm+nCjhbyrLRZvrus3jUhDOFoiAW3 oq6GduNu44WFW3cqirLnjpE/95ub9tukzmcXQi1KE6Sw58F8HVVek1eSjdzhLwndQGTN /dUmyO+KezO2sQ6YKZW8GtP5zD7DsXJBBuU0fiWP9YcWSzFdyw6wC93xMiPHlql0BawZ hGcw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qLCAMNY8ip350mbuKSMyKuBtoSJ499QiUIVKiUAfvRU=; b=ZEiERe+atDuPo38GDRev2aI21OFNwUkBTx4Gu2N36z5muaMAvqNnxK9/O1TPIfYq7k gq7u3Xyk5W0Y5xap+gwGMYhiNtYU4vnrJRiedZH1BrTs6TpUsDXo5f6Fo4O62rrtptrn 24JHvk4xzZ2yjwuH+SxSD9SW5nBObK8YOoVNGjTWfJJWwvuiyG4CWOMRkty+wltD/7Np AnNWVSarbgMqKkNmuFy9HIjnt+rL81UJj7LhMBPTxj9uz1QR7VJBUy7n6dPnXPTY2SlR k1+UfNq1dbApDhdKv6L7KlPUXx8bn0olrD//ZnAf3zebzsQBZ3StMi5wviMgidBqpYXb ffvA== X-Gm-Message-State: AJcUukdvYKH2FhKxjipwopGdc6iPhxO5tFTyXtGlBun53y54ZxV4luud LQPi7mPuVvkZq7d5D2wcrd30N/7591kw/+2005HRVw== X-Received: by 2002:a02:ac8c:: with SMTP id x12mr15856810jan.72.1548074574520; Mon, 21 Jan 2019 04:42:54 -0800 (PST) MIME-Version: 1.0 References: <1547634429-772-1-git-send-email-elena.reshetova@intel.com> <20190121123836.GC47506@lakrids.cambridge.arm.com> In-Reply-To: <20190121123836.GC47506@lakrids.cambridge.arm.com> From: Dmitry Vyukov Date: Mon, 21 Jan 2019 13:42:43 +0100 Message-ID: Subject: Re: [PATCH] kcov: convert kcov.refcount to refcount_t To: Mark Rutland Cc: Elena Reshetova , Andrew Morton , Andrey Ryabinin , Anders Roxell , LKML , Kees Cook , Peter Zijlstra Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 21, 2019 at 1:38 PM Mark Rutland wrote: > > On Wed, Jan 16, 2019 at 12:27:09PM +0200, Elena Reshetova wrote: > > atomic_t variables are currently used to implement reference > > counters with the following properties: > > - counter is initialized to 1 using atomic_set() > > - a resource is freed upon counter reaching zero > > - once counter reaches zero, its further > > increments aren't allowed > > - counter schema uses basic atomic operations > > (set, inc, inc_not_zero, dec_and_test, etc.) > > > > Such atomic variables should be converted to a newly provided > > refcount_t type and API that prevents accidental counter overflows > > and underflows. This is important since overflows and underflows > > can lead to use-after-free situation and be exploitable. > > > > The variable kcov.refcount is used as pure reference counter. > > Convert it to refcount_t and fix up the operations. > > > > **Important note for maintainers: > > > > Some functions from refcount_t API defined in lib/refcount.c > > have different memory ordering guarantees than their atomic > > counterparts. > > The full comparison can be seen in > > https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon > > in state to be merged to the documentation tree. > > Normally the differences should not matter since refcount_t provides > > enough guarantees to satisfy the refcounting use cases, but in > > some rare cases it might matter. > > Please double check that you don't have some undocumented > > memory guarantees for this variable usage. > > > > For the kcov.refcount it might make a difference > > in following places: > > - kcov_put(): decrement in refcount_dec_and_test() only > > provides RELEASE ordering and control dependency on success > > vs. fully ordered atomic counterpart > > > > Suggested-by: Kees Cook > > Reviewed-by: David Windsor > > Reviewed-by: Hans Liljestrand > > Signed-off-by: Elena Reshetova > > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and > something poking kcov? > > Given lib/refcount.c is instrumented, the refcount_*() calls will > recurse back into the kcov code. It looks like that's fine, given these > are only manipulated in setup/teardown paths, but it would be nice to be > sure. A simple program using KCOV is available here: https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-tools/kcov.rst#L42 or here (it's like strace but collects and prints KCOV coverage): https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c > Thanks, > Mark. > > > --- > > kernel/kcov.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/kcov.c b/kernel/kcov.c > > index c2277db..051e86e 100644 > > --- a/kernel/kcov.c > > +++ b/kernel/kcov.c > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > #include > > > > /* Number of 64-bit words written per one comparison: */ > > @@ -44,7 +45,7 @@ struct kcov { > > * - opened file descriptor > > * - task with enabled coverage (we can't unwire it from another task) > > */ > > - atomic_t refcount; > > + refcount_t refcount; > > /* The lock protects mode, size, area and t. */ > > spinlock_t lock; > > enum kcov_mode mode; > > @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch); > > > > static void kcov_get(struct kcov *kcov) > > { > > - atomic_inc(&kcov->refcount); > > + refcount_inc(&kcov->refcount); > > } > > > > static void kcov_put(struct kcov *kcov) > > { > > - if (atomic_dec_and_test(&kcov->refcount)) { > > + if (refcount_dec_and_test(&kcov->refcount)) { > > vfree(kcov->area); > > kfree(kcov); > > } > > @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep) > > if (!kcov) > > return -ENOMEM; > > kcov->mode = KCOV_MODE_DISABLED; > > - atomic_set(&kcov->refcount, 1); > > + refcount_set(&kcov->refcount, 1); > > spin_lock_init(&kcov->lock); > > filep->private_data = kcov; > > return nonseekable_open(inode, filep); > > -- > > 2.7.4 > >