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 AA0ACC3DA4A for ; Thu, 22 Aug 2024 17:52:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3CDA880044; Thu, 22 Aug 2024 13:52:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 37C376B036A; Thu, 22 Aug 2024 13:52:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1F6BE80044; Thu, 22 Aug 2024 13:52:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 00D066B0368 for ; Thu, 22 Aug 2024 13:52:10 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 8E05A41978 for ; Thu, 22 Aug 2024 17:52:10 +0000 (UTC) X-FDA: 82480625220.27.8D0AD78 Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by imf15.hostedemail.com (Postfix) with ESMTP id 96E73A0009 for ; Thu, 22 Aug 2024 17:52:08 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=OXmGxsmC; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724349111; a=rsa-sha256; cv=none; b=IcdI485pmgNSaxPwsIkFOIXzOE7lGIYLReRKqnK+Y6T13oXF0EsBXmkbM2SSRJ7GNax6NN Ft/OLNsEJIkFwLXvrVdD8jFnK37ifh3m1PYMoOAWuhymerUT+8DzlVW0EFhoCdrPPuLH/4 KVEAWvN6XFDNNu7SzsLA+8MrQdzitVU= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=OXmGxsmC; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724349111; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=TSMdvcvti+RWCtYMd+FEZT5Ru/ACmw+rmYf/bGB23eI=; b=wUqwDyrJ6Zy/4rzbTiyup9sfRGGHxeDb3WaAtjC8yiYo9tuSeduXiC6OaNRK4b8/Q14ZTl Czb3CAnVPGK36zDJ3CMWB22UX54CMW6wcwkLvj4X1p2q/dg7K6x1HnBNr/pFc7l02/QjX5 jOXbquOEDjGdHNZzRghR5vOUeMaUHE0= Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-2d3c08541cdso879065a91.2 for ; Thu, 22 Aug 2024 10:52:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724349127; x=1724953927; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=TSMdvcvti+RWCtYMd+FEZT5Ru/ACmw+rmYf/bGB23eI=; b=OXmGxsmCxBP9Ohy8SFV1NN03hDgmBy2JV8Jp/P9rHx+Dp+oaA2/y4PzeUlbyhswWxK 4jVna83ciZECbdA4T5XkIWVM4MAvvF58UD0exVkP5jWb+uOz1mHURAdIoG5cltmYiIYg YL0yKx2pp8Yp1OWp7t0tu0UFokm+amBZr91mmTrVu0h1pxVttKi65ICygHFoRfCrtPKo GTLdVgFZGHS0Gax9WQbxrpnsStkYyTttFz8YNMMKK0jnJdaj6Vm8DXD30XeZzHwCUHiJ PUPTRpiODAHTBa3YCnK3LRohILkUP3s5ty9pnjuHj3bJpbTtVw21AW7lqoxg9ynfFWUw woTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724349127; x=1724953927; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=TSMdvcvti+RWCtYMd+FEZT5Ru/ACmw+rmYf/bGB23eI=; b=N7daP88EYQy3GHKn41SinSCbYFbxVCFnNmAiKhnntlOYJKr/PH2GNja5PF1JNJ5iPP 9cAQ6r350p2oftYCt5NY98Usxlcyg4foyA7BrNNKIW7GiC6K+Ehk8qKVvz/ff6CRr2K5 IeHRPmyAZRt0/tOB8wslhRdYHjk05DSuM/xyjeI0zJPwlQsV23hb5khb4ljNJ5VHYm0j WkSJsLTQWQD4zQr7+9Lfno4BAmy7vAgitxwlU7PMXLUvqQcmv6JKIjZmOLviSbS0RnAA bg6aj4moXAl2UIX5mvAOI3IdfKLdGWLw1V2YDPQ04hi1gFaoHWBPgmb+4Nt/4HFRG9ps gqvg== X-Forwarded-Encrypted: i=1; AJvYcCX9rRlwrH4WJqbdbZqQzdew6ShPz8PBq0r102ALTpJDhrD4Ia2HPMHhxYAyq76GwiuZIaYjBmLPpg==@kvack.org X-Gm-Message-State: AOJu0YyphWP9CXPNkuykl07+vlXw7GBEtvoAdlqlOrFgOrkL/Ttnoum9 75GkZ9nMX7zjC07kxApYzxzFCq4EmqltoDV+6Rv8y5rJcd3V9kWhsS4+cmaaBH/X8hCP8T6a0o4 Zdo8V0G+jJGEFUG/fNu6ZjBTwWc4= X-Google-Smtp-Source: AGHT+IFbNA+AYoJwPKkug+4J4dkzuWlv61LePE1OdqmnnD6GiBA+gHzMQMKHafS8xgKroO9gkSs966LHnR+mMgns4Bo= X-Received: by 2002:a17:90a:9a4:b0:2c4:ee14:94a2 with SMTP id 98e67ed59e1d1-2d5e9ec9953mr7556078a91.27.1724349127031; Thu, 22 Aug 2024 10:52:07 -0700 (PDT) MIME-Version: 1.0 References: <20240813042917.506057-1-andrii@kernel.org> <20240813042917.506057-5-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Thu, 22 Aug 2024 10:51:54 -0700 Message-ID: Subject: Re: [PATCH v3 04/13] uprobes: travers uprobe's consumer list locklessly under SRCU protection To: Jiri Olsa Cc: Andrii Nakryiko , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 96E73A0009 X-Rspamd-Server: rspam01 X-Stat-Signature: mn31t16d37g4oicf4k8mrhwbqyusby7m X-HE-Tag: 1724349128-285258 X-HE-Meta: U2FsdGVkX19V2zr5vfQdVHdw3lEzBAif9Y6nlzNvt+KuMazx3JEr3tDBSS1KXojIwMNgbmlkk12zlHZCYJvYO91RpPn3ZOyiIKkdhxEm+GLDdDvA3FrPQVFN5TyR4hXm6sg1uHUan5utubRr1QSi1iLEr9DlQUr1xUCtqth/SI7j0s0Zc0Vel1jUTozsoa3R21Suw7XimvgIBMzULOINkypJGzbIHSLe6nS93vxTnmzwpKdrp6MQFK4frqSaCaLBepWpq06cVoKDAvFhqramFxaDQzVo+G7fBcPelFfyNhHNL0uPZfLouk7M4I480PJyjeQF7eU06X3w1br7wRYCPvErjGtECcAS0kqUR1VQ2cUw+YQcv5LPRCIhoXhhSSwbmuKk1qxJ+0Clv40MTKXpd6+4DySS2744/GTUk7+8JcyOazASGSJeDuiIqxF4Gpxr5S1h+bTrnQZx3akLzv3JoxB6NKzltezDanZ6g7xh0iXpcmNpHzyOaOwCPvSVfIN0pJwkEz5Ix37Hmr6OrvWM3Wpr0nSLNo2m2K3FsIVB6cSjjJFyvWy462JQXhsBPXrtZXJkSTaQ1CmXErNz+FWNfEH7fE296pP3c8HU3yG55j+ICY9ohu6tG/8l+9la8Wmh9yVsIIoACzjdsruQo8ynzamRIOuwfhit9JfYIzH/gTe/bwiUH/GlZYaVV1rLsL+u7u7gc4R7jBGUgj27KfbbiFVbgoGD3GOO16a2wcyjnMsynSY8tCHf6xEzeTMrw3QQjGPTLb5C9wByiPVJurQetrqPXttspVWG7+0cIiVQnbHYWtoE6djxMRjgB1h8os4pS/PRN/4XKvdMOT4QXqQgihRMqXe0zYLt49BD2Kc3PEVQJGMpXvDMtt/O1RO/MovE5XTZN6BDN0f30PH6Mzlk6CLbo+u86vvVLWjyZ8B9mGQTlTt1aLDQGeAOrz7Y81hO3nW+vuCjgKfQaZYJLCM Vbgks4UH Njs/er401XLugycVchzI+rTHA5h7HjEDUUip7iJrS/UORFmx7Ye48PxDq/922tofhBBVQ/rrnT6NlcP7fLXkOkyRsTs/Eu/5w408oI3ePUou8Y5zKPxE3PSaL0wuljAnc4SnZ1RooefsEmZq/MUcwS29v1OzlDePhNWMV0GndTev2/9iQxnF/2lVzZUKpSfxaeiqLevUwNarKD6hFKf2lITJGbOM0aj2v1vG+FYzwaMqoCUSGIYHtkZOqE8DvVz+fGvZoAD+mS2wcHlIMBDsBVZOXH39u/G7yxMs13DAstzuuApUJSC40g2iyuvHkpTRkWUWFegvgDbV23USLJD9UPKzeQ2Kfgaw5hY5l X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, 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 Thu, Aug 22, 2024 at 10:35=E2=80=AFAM Jiri Olsa wro= te: > > On Thu, Aug 22, 2024 at 09:59:29AM -0700, Andrii Nakryiko wrote: > > On Thu, Aug 22, 2024 at 7:22=E2=80=AFAM Jiri Olsa = wrote: > > > > > > On Mon, Aug 12, 2024 at 09:29:08PM -0700, Andrii Nakryiko wrote: > > > > > > SNIP > > > > > > > @@ -1125,18 +1103,31 @@ void uprobe_unregister(struct uprobe *uprob= e, struct uprobe_consumer *uc) > > > > int err; > > > > > > > > down_write(&uprobe->register_rwsem); > > > > - if (WARN_ON(!consumer_del(uprobe, uc))) { > > > > - err =3D -ENOENT; > > > > - } else { > > > > - err =3D register_for_each_vma(uprobe, NULL); > > > > - /* TODO : cant unregister? schedule a worker thread *= / > > > > - if (unlikely(err)) > > > > - uprobe_warn(current, "unregister, leaking upr= obe"); > > > > - } > > > > + > > > > + list_del_rcu(&uc->cons_node); > > > > > > hi, > > > I'm using this patchset as base for my changes and stumbled on this t= oday, > > > I'm probably missing something, but should we keep the 'uprobe->consu= mer_rwsem' > > > lock around the list_del_rcu? > > > > > > > Note that original code also didn't take consumer_rwsem, but rather > > kept register_rwsem (which we still use). > > humm, consumer_del took consumer_rwsem, right? > Ah, it was inside consume_del(), sorry, my bad. I can add nested consumer_rwsem back, but what I mentioned earlier, regiser_rwsem is sort of interchangeable and sufficient enough for working with consumer list, it seems. There are a bunch of places where we iterated this list without holding consumer_rwsem lock and that doesn't break anything. Also, consumer_add() and consumer_del() are always called with register_rwsem, so that consumer_rwsem isn't necessary. We also have prepare_uprobe() holding consumer_rwsem and there is a comment about abuse of that rwsem and suggestion to move it to registration, I never completely understood that. But prepare_uprobe() doesn't seem to modify consumers list at all. And the one remaining use of consumer_rwsem is filter_chain(), which for handler_chain() will be also called under register_rwsem, if purely lockless traversal is not enough. There are two other calls to filter_chain() that are not protected by register_rwsem, so just because of those two maybe we should keep consumer_rwsem, but so far all the stress testing never caught any problem. > jirka > > > > > There is a bit of mix of using register_rwsem and consumer_rwsem for > > working with consumer list. Code hints at this as being undesirable > > and "temporary", but you know, it's not broken :) > > > > Anyways, my point is that we didn't change the behavior, this should > > be fine. That _rcu() in list_del_rcu() is not about lockless > > modification of the list, but rather modification in such a way as to > > keep lockless RCU-protected *readers* correct. It just does some more > > memory barrier/release operations more carefully. > > > > > jirka > > > > > > > > > > + err =3D 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); > > > > +