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 511F1C433EF for ; Mon, 4 Jul 2022 14:46:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229896AbiGDOq1 (ORCPT ); Mon, 4 Jul 2022 10:46:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44694 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233644AbiGDOqY (ORCPT ); Mon, 4 Jul 2022 10:46:24 -0400 Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D17C9EE13 for ; Mon, 4 Jul 2022 07:46:21 -0700 (PDT) Received: by mail-ej1-x631.google.com with SMTP id fw3so17105643ejc.10 for ; Mon, 04 Jul 2022 07:46:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=M+qVlZDYaAoCOVBpeXTz8LxMK35K+S3D3doABPbbA6U=; b=O+YOPzG3QC5vCWWZEKLqaxGKZYtynjFnz6aQS6ibqsWEEVI11WzP+tP8h79MvxyvEI VQtFQjNwpI05jrdPqDiyxAQHNM3HU7V8eANnSthLH3fjHpazQEeiDqN2azM+oOMZ8r20 bPXVCzPILpA7aqpUJZRdnrfD2lV/0VHDvK4CrObBHe79Zh2zhBt+2YqG94wstcDMwAqa FSWz/w+dyw54CtjOx1Ox1XnBKF3CIQAfYSmrBxfJPLLOs+k2F0rZd7gG44L+8T8UEJQC fUc38jfZwXld6BQy8XYnBSJWc44bEPnxhJlix/vdLigKWD5MmkdAPjKkZnGomLknYe+m 6BPQ== 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=M+qVlZDYaAoCOVBpeXTz8LxMK35K+S3D3doABPbbA6U=; b=wM6y3dVUT5hbM1GW1zU1hqbYkYIbSX0hRpD5sTMYVii/yK2pg03pTY/rOz5yxWn9O6 RppW865ugDu5rbg9EIX8V1Z9Hlq0hNJ+XPil6LPssZYzJ2XZlrD/7+LP/Vdw5sY5LIHi 15jCY9ZnfHUF8ReJgtuOkI9mpxFczoc6cZ6lfR8MBMXV1hsmK3XJN/lhUWwDagwEsImw 9VEJdHMWdWzAldYrpnzEuDYfxbcwGtphF6yN2obHKjeX+cVJUuL/MDiBlCvT01WgDPHX S0nqYd1kFamR3HoNhWl7ykRaua6NCUSMzsx1diIS587T44RhyYI9vqWQZZc7A3s0Ax5/ YK2Q== X-Gm-Message-State: AJIora89G8l8pp5ziQhAT6wG7gU5e+e1eck6nWXmqinDu5yMO+xLkT8I DGEu4II4DbDhPwGA5US5UmMQ X-Google-Smtp-Source: AGRyM1t89RbesStqt7Tieom5AmKNJLEeLrY0+FlIzuKMV7VyOhtKEjj91xD5EazoVDBn2kcBfXPj4Q== X-Received: by 2002:a17:906:6a11:b0:726:97b8:51e9 with SMTP id qw17-20020a1709066a1100b0072697b851e9mr29599521ejc.115.1656945980259; Mon, 04 Jul 2022 07:46:20 -0700 (PDT) Received: from google.com (64.227.90.34.bc.googleusercontent.com. [34.90.227.64]) by smtp.gmail.com with ESMTPSA id pv1-20020a170907208100b00726abf9cd8esm9904462ejb.125.2022.07.04.07.46.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 04 Jul 2022 07:46:19 -0700 (PDT) Date: Mon, 4 Jul 2022 14:46:15 +0000 From: Wedson Almeida Filho To: Tetsuo Handa Cc: Greg KH , "Rafael J. Wysocki" , Len Brown , Pavel Machek , arnd@arndb.de, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH] char: misc: make misc_open() and misc_register() killable Message-ID: References: <000000000000d9ff3a05bb37069e@google.com> <72e74af9-f1b6-e383-a2c3-6ee8a0aea5e0@I-love.SAKURA.ne.jp> <100f445e-9fa8-4f37-76aa-8359f0008c59@I-love.SAKURA.ne.jp> <01a93294-e323-b9ca-7e95-a33d4b89dc47@I-love.SAKURA.ne.jp> <19598d43-de61-c663-25e8-17b6f5d5ef80@I-love.SAKURA.ne.jp> <951dd1a9-2b48-fb0c-9ee2-aac2b8170c2c@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <951dd1a9-2b48-fb0c-9ee2-aac2b8170c2c@I-love.SAKURA.ne.jp> Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On Mon, Jul 04, 2022 at 11:07:54PM +0900, Tetsuo Handa wrote: > On 2022/07/04 22:57, Wedson Almeida Filho wrote: > > On Mon, Jul 04, 2022 at 10:48:32PM +0900, Tetsuo Handa wrote: > >> On 2022/07/04 21:59, Wedson Almeida Filho wrote: > >>>> @@ -139,6 +139,10 @@ static int misc_open(struct inode *inode, struct file *file) > >>>> > >>>> err = 0; > >>>> replace_fops(file, new_fops); > >>>> + if (iter->unlocked_open && file->f_op->open) { > >>>> + mutex_unlock(&misc_mtx); > >>>> + return file->f_op->open(inode, file); > >>>> + } > >>> > >>> One of the invariants of miscdev is that once misc_deregister() returns, > >>> no new calls to f_op->open() are made. (Although, of course, you can > >>> still have open files but that's a whole different problem.) > >> > >> The point of this change is that file->f_op after mutex_unlock(&misc_mtx) is > >> from new_fops which is guaranteed to hold a ref on "struct file_operations" > >> via new_fops = fops_get("struct miscdevice"->fops). > >> That is, a module ref remains valid even after mutex_unlock(&misc_mtx). > >> > >> And as with major_names_lock case quoted below, this change assumes that > >> misc_deregister() is called from module's __exit function, and fops_get() > >> is preventing the module owning new_fops from calling __exit function. > > > > Your assumption is not sound. misc_deregister() can be (and is) > > legitimately called from other places, for example, a driver's remove() > > callback. In fact, when I grep for misc_deregister(), the second > > instance is such a case. > > OK, the frequency of calling misc_deregister() can be much higher than > unregister_blkdev(), which means that misc_mtx is more prone to trigger > hung task warnings. I'm more inclined to avoid sleeping with misc_mtx held. Tetsuo, I'm sorry if I'm not making myself clear. I'm arguing that your patch is buggy and therefore should not be accepted. Here's one example of an issue that your patch would introduce. In binder init, we have the following error path: err_init_binder_device_failed: hlist_for_each_entry_safe(device, tmp, &binder_devices, hlist) { misc_deregister(&device->miscdev); hlist_del(&device->hlist); kfree(device); } Note that open() for binder touches the `device` pointer. If open() can be called _after_ misc_deregister(), then there is a race condition where open() is touching `device` while this error path is freeing it. Right now it isn't a bug because misc_deregister() guarantees that after it returns, open() won't be called anymore. Your patch breaks this guarantee.