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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CC0BDC3DA4A for ; Thu, 22 Aug 2024 14:22:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 271B16B0119; Thu, 22 Aug 2024 10:22:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 221246B0176; Thu, 22 Aug 2024 10:22:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 073186B0280; Thu, 22 Aug 2024 10:22:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id D6F366B027C for ; Thu, 22 Aug 2024 10:22:57 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 7232D1A077F for ; Thu, 22 Aug 2024 14:22:57 +0000 (UTC) X-FDA: 82480097994.22.1D0334C Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by imf01.hostedemail.com (Postfix) with ESMTP id 7BE804000E for ; Thu, 22 Aug 2024 14:22:55 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Rq7pHYEZ; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf01.hostedemail.com: domain of olsajiri@gmail.com designates 209.85.218.46 as permitted sender) smtp.mailfrom=olsajiri@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724336558; a=rsa-sha256; cv=none; b=aXSVwDsmV+rPCXyD4o5CgdxlcbL+KZ8ytRwLLkWW8Sk6heOU3LB5N558yufZj3jIsO2XnK XkYOWIP1lVg97VbU/A/BlPFB2GVVfbSngapESR6EBSnqnAHrIhk55GWjyzDixvlgTD0o4T q47DUSW5Z2lJGHoW3RcFmv+sXhFMjIY= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Rq7pHYEZ; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf01.hostedemail.com: domain of olsajiri@gmail.com designates 209.85.218.46 as permitted sender) smtp.mailfrom=olsajiri@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724336558; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=hvFk4KiFQ4jknRyXdpWSIplvO8xMF83nYSG+PUhv9V4=; b=n9zR5nXtYUsgNW41JwA6JqXMKm9UAjSzcjU38wzOa2egW7oH1xJGkGwFlrq0WCd6BjF8IZ Fib4mq/qZMlLYANEdOPniVrsHAa4YcWmsqzaJSO4Is1bD9+uOwsEKeM9RO9K+U689XuAZG uMeYC9R77OZ3rPDtHjfBNNWvzl3yfEY= Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-a86910caf9cso129317166b.1 for ; Thu, 22 Aug 2024 07:22:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724336574; x=1724941374; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:from:to:cc:subject:date:message-id:reply-to; bh=hvFk4KiFQ4jknRyXdpWSIplvO8xMF83nYSG+PUhv9V4=; b=Rq7pHYEZDYxCYPNbfeoz3QlH3RKU2DHKhAtAUQMKqwmiiZrMxKmcqhSMYaBa3lWk9c yeMG23qHEk5uP+8WJNygUj4yYqA43DpfwZZ0bBVc4ayG6uVZXLslSqqpRcsoNZK7WiTC GrnFH/tnF/+0Q5L+DFoF0NZPNN6rX+07xF2x84m25tfA/lNcplhHEJDUwU+PMkghmhNO JUS++7R4EEqSgBA1HCZomCyuKrUjApphWXc9SUJuBUWmDZJFcWlq/HX/taH0+rh90d8V kSmO3LtSTf0L5aEv64HN5ROxvEbGfeQW4jZE55r5SbwUqRnOhwFCeShEWAH1CJL04vnt 9A0Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724336574; x=1724941374; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=hvFk4KiFQ4jknRyXdpWSIplvO8xMF83nYSG+PUhv9V4=; b=F8xzLxCRCVyH8QWst2TL3buHm1N/nvhmmuEE8XbiIrKfmhgtN65sHLLxC9i5cqwQJy FbZpeQ+Y65h3HIyaEnHwr4BbTC7lsGQvqtWNw/43cPe7EkdusqwOK8K2LpIIkklrCB3o /wlPOs8CjY3jrsKFLbkpowSb9xie3P3SuBCiszutpfjI0dhtxEfslowSOcUu144f/L52 zLMtTArrjZGC3axd7fF/KdGCczmoXlm6UwZPhMkVQsR6wXFfHjWrCiBfZH1gI5RZbBtl 8iIbIhErbbKw34EUCI9R4FTF+5gBNYsPvvpkrPYoMLPtCtXXrI+nuPLPi+4Iq7L7cm/r qh0g== X-Forwarded-Encrypted: i=1; AJvYcCX6PuMxSFEmDl9i1bzpUzqHetArh3k75ewIluyS5yJAvKnMIim6hpBripZUVOY3eejGVqsRjlncJQ==@kvack.org X-Gm-Message-State: AOJu0YwDSsMwgmCH+iCP+OVtwVu3QNm1gXIFJbU0YnR9ggKdHC0YUL12 iCMyRHzBb0g+66sgzVBoiveLq4ua5JW0i3HKJVLK6bA32MfcSpIA X-Google-Smtp-Source: AGHT+IF0hmAt6tkNqeAkfXeiDKDltNUNbNaOv2hcoNDEkEIxY1sF68Dk0oiuClfQ1GBKVbEcaxalUA== X-Received: by 2002:a17:906:6a0e:b0:a80:f646:c9c4 with SMTP id a640c23a62f3a-a868a5aaf28mr301279766b.1.1724336573366; Thu, 22 Aug 2024 07:22:53 -0700 (PDT) Received: from krava (2001-1ae9-1c2-4c00-726e-c10f-8833-ff22.ip6.tmcz.cz. [2001:1ae9:1c2:4c00:726e:c10f:8833:ff22]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a868f48621esm127706966b.176.2024.08.22.07.22.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Aug 2024 07:22:52 -0700 (PDT) From: Jiri Olsa X-Google-Original-From: Jiri Olsa Date: Thu, 22 Aug 2024 16:22:51 +0200 To: Andrii Nakryiko Cc: linux-trace-kernel@vger.kernel.org, peterz@infradead.org, oleg@redhat.com, rostedt@goodmis.org, mhiramat@kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org, paulmck@kernel.org, willy@infradead.org, surenb@google.com, akpm@linux-foundation.org, linux-mm@kvack.org Subject: Re: [PATCH v3 04/13] uprobes: travers uprobe's consumer list locklessly under SRCU protection Message-ID: References: <20240813042917.506057-1-andrii@kernel.org> <20240813042917.506057-5-andrii@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240813042917.506057-5-andrii@kernel.org> X-Rspam-User: X-Rspamd-Queue-Id: 7BE804000E X-Rspamd-Server: rspam01 X-Stat-Signature: 3yg5tof3shkhpre45r38qtjg37h635g4 X-HE-Tag: 1724336575-11881 X-HE-Meta: U2FsdGVkX1+3A13dRYHugvDsSRV2PfgQpbLPoLIX+uoIX+OlkXjDkjOVnOaJ1z+qks49BHfZpzxOmJkwgWPCD1JXYnOIckdBrNRkwlsMvcU/hVpAqM6D/HJd+0uUOpabWkLTr8KttwqoDvJ2FWhZnTa5YNSh/ovGR+DWwLzrUWZyBIXAcRUNzGxZmmmK0kWAx8F/nsjC5IOZPoAtc733Z50nXAaTD2kyiVRba7Z0IAiDMPZ7iFLccqBf+ykF1Qsi9nspGL4wYNNIOOEjLLe+TLs78NzndNf0Jc4SU37ESduIp8JDQlNIcW/EVM9bxA26xpzzpcIe3M+VA0WuRpnasBoAsd06qCcMzR5oGO7I1/x9e/2wa49w/HAuqnAOfynqkEDDe91OqDK6WxvRHMW+u7bJZ8eLOZ7xRsENv9x5dFg1t935sZuXebeLOaAdw0LHUByYg1sefZ9Sgcb54vMfUHzImlie+bgWNIPw5PpDvF2T+x3jf9MFj38oOXqtiHSvHVs3SScD37qCQ8Gr47HACgslbfvtlSEFJVHnlY9HHgHqDYtXX51tH0euzSZThzHNAxdrh5YIFmkZuloTeTJGJAGjrFQMMm0b8B5VKgJNNDq/zCl8O51aLXmeVQ04gbvMnbDB2qFC5uNQApz3GxiiG85C7KZo0jRxjOu4gzSu8Ow/L/Ruyt9Q2FulvvSoEcAZUc3HVq8mcqw2EXILlOhN4S2QMLjjXTAGI5ItqUnUQdQHzeXmNXA4LHgPswa2cqgCbFN2K/pmxWTiBh6pxjLGr2R9J8D3pyXDNM7jMeY5ioWU/Q2U5uiyRDA1FWzTR6SD/jSUy+a/ULOpE8OagnBahCkaMDZeIp1Yp1h+HTOfpgiOdsxAXy1vVrbvhxAGGpWD8XatpoGsmBnLRyBgZ5w8WUG+Jgs8X296Ev3loWrwzrFhbARV9lsuM6a6V48WeKKzhCZxRQQsbqORQ7OFgUu WyCiYE7I gkcZBhTLoCvx8k5MbFWOGgSF3Qrlvwd6auxLMkFqheYSjP9t30Y24OUC9FpqD4c0i18fH/x4NqYw8qdI1/3r2ztUCc+JRakWqNzsb/NkLadZij9OPmdSTd9CZxsmumi2MK2uMjL/uOD5iJkRS5QVKczstUGdc6L+e+3A0XnFCHRhZ1Sq7RQRA1+1aZ/10aL3Sc0nUmJqsBZ6/s4VNAMAGKy7Z+yZIRao+DxF1+z9jopeyXOKL86JWQoSE05oHbqyL+/V/a6Zrm1lOymtzJU3CXpOzkZshj6F6lSRxBf7vnmTvWbpXbiyW1kPQ0N9gQ+9z/CsksAK9QP2cuSvxqXv4L/rHXNbxubYJNmCH X-Bogosity: Ham, tests=bogofilter, spamicity=0.007180, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Mon, Aug 12, 2024 at 09:29:08PM -0700, Andrii Nakryiko wrote: SNIP > @@ -1125,18 +1103,31 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) > int err; > > down_write(&uprobe->register_rwsem); > - if (WARN_ON(!consumer_del(uprobe, uc))) { > - err = -ENOENT; > - } else { > - err = register_for_each_vma(uprobe, NULL); > - /* TODO : cant unregister? schedule a worker thread */ > - if (unlikely(err)) > - uprobe_warn(current, "unregister, leaking uprobe"); > - } > + > + list_del_rcu(&uc->cons_node); hi, I'm using this patchset as base for my changes and stumbled on this today, I'm probably missing something, but should we keep the 'uprobe->consumer_rwsem' lock around the list_del_rcu? jirka > + err = register_for_each_vma(uprobe, NULL); > + > up_write(&uprobe->register_rwsem); > > - if (!err) > - put_uprobe(uprobe); > + /* TODO : cant unregister? schedule a worker thread */ > + if (unlikely(err)) { > + uprobe_warn(current, "unregister, leaking uprobe"); > + goto out_sync; > + } > + > + put_uprobe(uprobe); > + > +out_sync: > + /* > + * Now that handler_chain() and handle_uretprobe_chain() iterate over > + * uprobe->consumers list under RCU protection without holding > + * uprobe->register_rwsem, we need to wait for RCU grace period to > + * make sure that we can't call into just unregistered > + * uprobe_consumer's callbacks anymore. If we don't do that, fast and > + * unlucky enough caller can free consumer's memory and cause > + * handler_chain() or handle_uretprobe_chain() to do an use-after-free. > + */ > + synchronize_srcu(&uprobes_srcu); > } > EXPORT_SYMBOL_GPL(uprobe_unregister); > > @@ -1214,13 +1205,20 @@ EXPORT_SYMBOL_GPL(uprobe_register); > int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool add) > { > struct uprobe_consumer *con; > - int ret = -ENOENT; > + int ret = -ENOENT, srcu_idx; > > down_write(&uprobe->register_rwsem); > - for (con = uprobe->consumers; con && con != uc ; con = con->next) > - ; > - if (con) > - ret = register_for_each_vma(uprobe, add ? uc : NULL); > + > + srcu_idx = srcu_read_lock(&uprobes_srcu); > + list_for_each_entry_srcu(con, &uprobe->consumers, cons_node, > + srcu_read_lock_held(&uprobes_srcu)) { > + if (con == uc) { > + ret = register_for_each_vma(uprobe, add ? uc : NULL); > + break; > + } > + } > + srcu_read_unlock(&uprobes_srcu, srcu_idx); > + > up_write(&uprobe->register_rwsem); > > return ret; > @@ -2085,10 +2083,12 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs) > struct uprobe_consumer *uc; > int remove = UPROBE_HANDLER_REMOVE; > bool need_prep = false; /* prepare return uprobe, when needed */ > + bool has_consumers = false; > > - down_read(&uprobe->register_rwsem); > current->utask->auprobe = &uprobe->arch; > - for (uc = uprobe->consumers; uc; uc = uc->next) { > + > + list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node, > + srcu_read_lock_held(&uprobes_srcu)) { > int rc = 0; > > if (uc->handler) { > @@ -2101,17 +2101,24 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs) > need_prep = true; > > remove &= rc; > + has_consumers = true; > } > current->utask->auprobe = NULL; > > if (need_prep && !remove) > prepare_uretprobe(uprobe, regs); /* put bp at return */ > > - if (remove && uprobe->consumers) { > - WARN_ON(!uprobe_is_active(uprobe)); > - unapply_uprobe(uprobe, current->mm); > + if (remove && has_consumers) { > + down_read(&uprobe->register_rwsem); > + > + /* re-check that removal is still required, this time under lock */ > + if (!filter_chain(uprobe, current->mm)) { > + WARN_ON(!uprobe_is_active(uprobe)); > + unapply_uprobe(uprobe, current->mm); > + } > + > + up_read(&uprobe->register_rwsem); > } > - up_read(&uprobe->register_rwsem); > } > > static void > @@ -2119,13 +2126,15 @@ handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs) > { > struct uprobe *uprobe = ri->uprobe; > struct uprobe_consumer *uc; > + int srcu_idx; > > - down_read(&uprobe->register_rwsem); > - for (uc = uprobe->consumers; uc; uc = uc->next) { > + srcu_idx = srcu_read_lock(&uprobes_srcu); > + list_for_each_entry_srcu(uc, &uprobe->consumers, cons_node, > + srcu_read_lock_held(&uprobes_srcu)) { > if (uc->ret_handler) > uc->ret_handler(uc, ri->func, regs); > } > - up_read(&uprobe->register_rwsem); > + srcu_read_unlock(&uprobes_srcu, srcu_idx); > } > > static struct return_instance *find_next_ret_chain(struct return_instance *ri) > -- > 2.43.5 >