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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0BA29C433EF for ; Mon, 10 Jan 2022 13:46:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234009AbiAJNqM (ORCPT ); Mon, 10 Jan 2022 08:46:12 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52316 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234710AbiAJNp5 (ORCPT ); Mon, 10 Jan 2022 08:45:57 -0500 Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A9F8EC061751 for ; Mon, 10 Jan 2022 05:45:51 -0800 (PST) Received: by mail-ed1-x529.google.com with SMTP id u25so53827843edf.1 for ; Mon, 10 Jan 2022 05:45:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=bHeTRojrHT97CzcJwwp8ETZD3FH9KBheVLaCf3e+Bz0=; b=mam0LsLi8LoWTGNIdMOetg75e2XJQGJ0mNUoJa9FG0TO6dRWdqOWRNLdf+K1w2HCvn ekYd1httCsa1T8n7aCJEd2iRaN9JPVswWNi4V4+8QXBahGH9CmKwtF2fOYG5qCCzLeE+ MG5PXOzkIGYFwWUVzNWAB2+Dl6nQHonD2kk2/xKDWsDE20PchZ8sFeNsreLfHgz8BtcZ oVS/WCHrk7F2C/U0h5gntBVEI6yQZUI/Uwj0PVyMu/QaOlmYwR1GJZff4he0ywtp6D32 cJ1i3G7EOdxnqUy88mBICgzyC4FU6+rmZ1T3ihyWYr0gp2JMb52qWRbKIZujTXgqTuPK +ufA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=bHeTRojrHT97CzcJwwp8ETZD3FH9KBheVLaCf3e+Bz0=; b=uxpr4qcOjX6I1azjPMvxOi+Z5ex3UpGPL25ggi6txwqvvUC7ZsAFeNK4WJqUkI2wYE Ykt6+Y3ybnhyn2iPQupfLMATkT04HtMz4HNBJvqFzS7x2Jak+YjYR6vBj9qCBUojjb/Z 5aRSK/qekqTGAkFh+RI18Y+I9pf7H57vpVwmBfj/4r5SNBH209nBK/irv8mHqfbNOnT9 QXblqpCkN+eq55ED1yrZB0EADJIhYktNKck6bO50FHJIJNsriW06L/Q2bUv1f1WWcrBB p4Ok6YzEomE8P0LIOK5a5a2E4CEipMcqXfWF8zUMIXzrf/DsqHXO+z2rPft5BCt1rzWT 5z/g== X-Gm-Message-State: AOAM530YvDgHVaCg7E+xcTwpP+2sKrE/zSumkZBZFYRUKFCqUhBI6LAn FSF14utfHpR+OmLBJ4YH3BfjYg== X-Google-Smtp-Source: ABdhPJxeRE0a4BzWgsaDYzwOeQO6iPCeZjoODWDm48/LJAdqsxT5P++dvckwm+iL3KZk5c4Bb65k1Q== X-Received: by 2002:a17:907:960d:: with SMTP id gb13mr3041620ejc.572.1641822350222; Mon, 10 Jan 2022 05:45:50 -0800 (PST) Received: from localhost ([57.190.1.3]) by smtp.gmail.com with ESMTPSA id l18sm2464463ejf.7.2022.01.10.05.45.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Jan 2022 05:45:49 -0800 (PST) Date: Mon, 10 Jan 2022 14:45:41 +0100 From: Johannes Weiner To: Linus Torvalds Cc: Eric Biggers , Tejun Heo , Zefan Li , Peter Zijlstra , Juri Lelli , Vincent Guittot , Ingo Molnar , Hillf Danton , syzbot , linux-fsdevel , Linux Kernel Mailing List , syzkaller-bugs , Linux-MM , Suren Baghdasaryan Subject: Re: psi_trigger_poll() is completely broken Message-ID: References: <000000000000e8f8f505d0e479a5@google.com> <20211211015620.1793-1-hdanton@sina.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Wed, Jan 05, 2022 at 11:13:30AM -0800, Linus Torvalds wrote: > On Wed, Jan 5, 2022 at 11:07 AM Linus Torvalds > wrote: > > > > Whoever came up with that stupid "replace existing trigger with a > > write()" model should feel bad. It's garbage, and it's actively buggy > > in multiple ways. > > What are the users? Can we make the rule for -EBUSY simply be that you > can _install_ a trigger, but you can't replace an existing one (except > with NULL, when you close it). Apologies for the delay, I'm traveling right now. The primary user of the poll interface is still Android userspace OOM killing. I'm CCing Suren who is the most familiar with this usecase. Suren, the way the refcounting is written right now assumes that poll_wait() is the actual blocking wait. That's not true, it just queues the waiter and saves &t->event_wait, and the *caller* of psi_trigger_poll() continues to interact with it afterwards. If at all possible, I would also prefer the simplicity of one trigger setup per fd; if you need a new trigger, close the fd and open again. Can you please take a look if that is workable from the Android side? (I'm going to follow up on the static branch issue Linus pointed out, later this week when I'm back home. I also think we should add Suren as additional psi maintainer since the polling code is a good chunk of the codebase and he shouldn't miss threads like these.) > That would fix the poll() lifetime issue, and would make the > psi_trigger_replace() races fairly easy to fix - just use > > if (cmpxchg(trigger_ptr, NULL, new) != NULL) { > ... free 'new', return -EBUSY .. > > to install the new one, instead of > > rcu_assign_pointer(*trigger_ptr, new); > > or something like that. No locking necessary. > > But I assume people actually end up re-writing triggers, because > people are perverse and have taken advantage of this completely broken > API. > > Linus