Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp1085854imu; Wed, 16 Jan 2019 12:28:52 -0800 (PST) X-Google-Smtp-Source: ALg8bN6Yb6smTz2HMMCOZ1h0nUZ4o4jrhiHXZLlfuPsv5l+L2C5VCWQvOh9mhc6HTovVmubcmD8Y X-Received: by 2002:a17:902:f01:: with SMTP id 1mr11487018ply.143.1547670532567; Wed, 16 Jan 2019 12:28:52 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1547670532; cv=none; d=google.com; s=arc-20160816; b=S4Y6jkDn2/LOU7tG20YyrMs5cJ2QMC2jPnhGD270aQ8cqPeahi9/QjPdfMOzeMkH7u umiTYbN5Li7DKd3v6o58DlJXX3zuB9tVFeNZQnGTGxEj3VyyvFhpThrEDWlO0WnRVB76 afA0qQvkaVIsqwnRRGIvnUHmOYH/5S7veaXOE5MPYYesdKxF8OLHixU+LmHc8YAQT9nN 5bUB6sclzGv22XXwIFg55Gem2zsAgYa7wWvy3EKrCu/+gz7A3rf/jrprDlUWzAuWw93X eR5Tqvf9e1/0wQ46GPvjvNVbvLluRCjDA9iEEc4JyAKmAfUyIg9qQ0pHLYNdze+w7jVs dCtQ== 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=rETGl7o2fGs5+cpdOh+3cwNMQcwXj7xEC2JHUJhKfDA=; b=pQ8+8n93l938uLkwyF+d8VZ7guXYkcOrohR7BzI+tLfZR8t+GMHB+ba0/DrFfO/deV R27sGRlKgW2hALfzrmD/UfkSp6gBo+SXs14B01inplTRZhIy/uT+e2bGsUj7sL47A8Cq DaHVfFhebHa1lEFj/xmXrUrNI54Z/8DtL7n2iQ4xz0uzu50uzjC5h+WsQL9hw+ikAlnj paUHyYAvGCRVw9cAHOjwtp+Pynuute9Zu+2EPGqkYdRzI6EGkl7JBOIDFWJj+FhdNHy+ XLSkrS9SYj4bFCCsr6ETd3ODcs8nw1S+dNMrH30za+tjDcwkU8PIwmvR+9XSo/Ezky+o UauA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=uqmoPKZc; 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 i5si7218145pfo.189.2019.01.16.12.28.36; Wed, 16 Jan 2019 12:28:52 -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=uqmoPKZc; 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 S2392814AbfAPMvq (ORCPT + 99 others); Wed, 16 Jan 2019 07:51:46 -0500 Received: from mail-it1-f193.google.com ([209.85.166.193]:36714 "EHLO mail-it1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2390115AbfAPMvp (ORCPT ); Wed, 16 Jan 2019 07:51:45 -0500 Received: by mail-it1-f193.google.com with SMTP id c9so2529418itj.1 for ; Wed, 16 Jan 2019 04:51:45 -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=rETGl7o2fGs5+cpdOh+3cwNMQcwXj7xEC2JHUJhKfDA=; b=uqmoPKZcnF+a04u8AJ/n3tbnb1CqQVnkmub47jiv85mbmV59JDjmK0ggaAHn7sHQdt B8yFAwgS2WwN6ec2MehsI3QR0P8nNDB2NtVlqplKoki4rvR3zHBXjuiSXmBkfc8qBsAY eezUIcmImTHx7fid+SMag3mI28Qg+bb/MeMjB/X6vZAhtfuwriO6hGtvKP5KU6PDVp3S hAwHuxYVLfiPsyFRyqU40uivnDAWvUW+mwdKYhI+RJzFaRiTTQHD+THMCihfyo2X9yGu DRKzMVaNTU3X4aQyO2BPrLDQav6zz/6wvuHZYVHkVYGGLa1GIvKzESxCj2YeajEpqA+Z Lzvg== 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=rETGl7o2fGs5+cpdOh+3cwNMQcwXj7xEC2JHUJhKfDA=; b=HJMUVAxi3Iw4jsL7E7Y7uo18Dwssj2viUneBOcy9p4vgz5clM32gQuHycsxmnhwwUc ojZWpAbnvVcXzxMFwh34OytqKGp+7gD5olSWgsHRgSrG4bqmdsqom/OEN/9n9Qss0mhn sN1Zt068vWo53GfI4HJNxvavYz2cdBZ1QPOsUTA7pTZMgzL5E7Y6+749DCXWzEM89KUR 08ZhHtg34Rtmt6JCv5GE1dyjgHzk5Zt6DHj+PADqvdOCcfvjfZhTPt21Rbu8Xdu1A+YS k6XjDzvBcU9ccCp9Q/iiByA6wHz4d9l2vchGApSm7hTGJ8nO9n1Xvw3A1lHiqHr8wAki 6HhA== X-Gm-Message-State: AJcUukfReXHtaDJPMWTP/VzqfZAQWRRVvB6HzkpzVVpl1jTaaIT7b5be jYtQxbjhquoxsthciE9QPxqCnSAPexeAN5P7l4GftUXI0mk= X-Received: by 2002:a24:f14d:: with SMTP id q13mr1271642iti.166.1547643104451; Wed, 16 Jan 2019 04:51:44 -0800 (PST) MIME-Version: 1.0 References: <1547634429-772-1-git-send-email-elena.reshetova@intel.com> In-Reply-To: <1547634429-772-1-git-send-email-elena.reshetova@intel.com> From: Dmitry Vyukov Date: Wed, 16 Jan 2019 13:51:33 +0100 Message-ID: Subject: Re: [PATCH] kcov: convert kcov.refcount to refcount_t To: Elena Reshetova Cc: Andrew Morton , Andrey Ryabinin , Anders Roxell , Mark Rutland , 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 Wed, Jan 16, 2019 at 11:27 AM 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 Reviewed-by: Dmitry Vyukov Thanks for improving this. KCOV uses refcounts in a very simple canonical way, so no hidden ordering implied. Am I missing something or refcount_dec_and_test does not in fact provide ACQUIRE ordering? +case 5) - decrement-based RMW ops that return a value +----------------------------------------------------- + +Function changes: + atomic_dec_and_test() --> refcount_dec_and_test() + atomic_sub_and_test() --> refcount_sub_and_test() + no atomic counterpart --> refcount_dec_if_one() + atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var) + +Memory ordering guarantees changes: + fully ordered --> RELEASE ordering + control dependency I think that's against the expected refcount guarantees. When I privatize an atomic_dec_and_test I would expect that not only stores, but also loads act on a quiescent object. But loads can hoist outside of the control dependency. Consider the following example, is it the case that the BUG_ON can still fire? struct X { refcount_t rc; // == 2 int done1, done2; // == 0 }; // thread 1: x->done1 = 1; if (refcount_dec_and_test(&x->rc)) BUG_ON(!x->done2); // thread 2: x->done2 = 1; if (refcount_dec_and_test(&x->rc)) BUG_ON(!x->done1); > Suggested-by: Kees Cook > Reviewed-by: David Windsor > Reviewed-by: Hans Liljestrand > Signed-off-by: Elena Reshetova > --- > 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 >