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 X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 321E2C742C2 for ; Fri, 12 Jul 2019 14:07:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 08221208E4 for ; Fri, 12 Jul 2019 14:07:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1562940455; bh=l2jxhcxb3H5zHl1HLHLMhC6OaCKayvurXGn4TEVXXoE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=0Pqn7h9/N7OHGd3kVN8KAn/o3p4BTgXgqvAyq432hGxoD0uCeG/yFAXlpX49/1jzX NhPCgDIwuscKm8bDJKo0xTG2d/p/AvR2XHPOYQmNGrOdLN1PN/F6TwDY6YNo8ubgyp ZvCDa4xoCeQBSUWGWwUZ1BvpooHF31g6N03bKZlM= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726867AbfGLOHe (ORCPT ); Fri, 12 Jul 2019 10:07:34 -0400 Received: from mail.kernel.org ([198.145.29.99]:48538 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726466AbfGLOHd (ORCPT ); Fri, 12 Jul 2019 10:07:33 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 13EEB206B8; Fri, 12 Jul 2019 14:07:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1562940452; bh=l2jxhcxb3H5zHl1HLHLMhC6OaCKayvurXGn4TEVXXoE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=A/oAdNTdeI+UFzMuaoy4DvzeHOzCWLaMqM4Z0jDa0fCTZdnnVMmVzbxFV54ca2D35 keNuDhJQelKhNkBrLYHxdMcUwGrOPDYBJxPTDuvPjVhJfRHoIeifkRu+IK8/R2Lkff ht6sVwffhqeUmPUg9qiRYQW91VgWhz3KGlahak+0= Date: Fri, 12 Jul 2019 16:07:29 +0200 From: "gregkh@linuxfoundation.org" To: Xiaoming Ni Cc: Vasily Averin , "adobriyan@gmail.com" , "akpm@linux-foundation.org" , "anna.schumaker@netapp.com" , "arjan@linux.intel.com" , "bfields@fieldses.org" , "chuck.lever@oracle.com" , "davem@davemloft.net" , "jlayton@kernel.org" , "luto@kernel.org" , "mingo@kernel.org" , "Nadia.Derbey@bull.net" , "paulmck@linux.vnet.ibm.com" , "semen.protsenko@linaro.org" , "stable@kernel.org" , "stern@rowland.harvard.edu" , "tglx@linutronix.de" , "torvalds@linux-foundation.org" , "trond.myklebust@hammerspace.com" , "viresh.kumar@linaro.org" , "Huangjianhui (Alex)" , Dailei , "linux-kernel@vger.kernel.org" , "linux-nfs@vger.kernel.org" , "netdev@vger.kernel.org" Subject: Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration Message-ID: <20190712140729.GA11583@kroah.com> References: <1562728147-30251-1-git-send-email-nixiaoming@huawei.com> <8ee6f763-ccce-ab58-3d96-21f5e1622916@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8ee6f763-ccce-ab58-3d96-21f5e1622916@huawei.com> User-Agent: Mutt/1.12.1 (2019-06-15) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote: > On 2019/7/11 21:57, Vasily Averin wrote: > > On 7/11/19 4:55 AM, Nixiaoming wrote: > >> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote: > >>> On 7/10/19 6:09 AM, Xiaoming Ni wrote: > >>>> Registering the same notifier to a hook repeatedly can cause the hook > >>>> list to form a ring or lose other members of the list. > >>> > >>> I think is not enough to _prevent_ 2nd register attempt, > >>> it's enough to detect just attempt and generate warning to mark host in bad state. > >>> > >> > >> Duplicate registration is prevented in my patch, not just "mark host in bad state" > >> > >> Duplicate registration is checked and exited in notifier_chain_cond_register() > >> > >> Duplicate registration was checked in notifier_chain_register() but only > >> the alarm was triggered without exiting. added by commit 831246570d34692e > >> ("kernel/notifier.c: double register detection") > >> > >> My patch is like a combination of 831246570d34692e and notifier_chain_cond_register(), > >> which triggers an alarm and exits when a duplicate registration is detected. > >> > >>> Unexpected 2nd register of the same hook most likely will lead to 2nd unregister, > >>> and it can lead to host crash in any time: > >>> you can unregister notifier on first attempt it can be too early, it can be still in use. > >>> on the other hand you can never call 2nd unregister at all. > >> > >> Since the member was not added to the linked list at the time of the second registration, > >> no linked list ring was formed. > >> The member is released on the first unregistration and -ENOENT on the second unregistration. > >> After patching, the fault has been alleviated > > > > You are wrong here. > > 2nd notifier's registration is a pure bug, this should never happen. > > If you know the way to reproduce this situation -- you need to fix it. > > > > 2nd registration can happen in 2 cases: > > 1) missed rollback, when someone forget to call unregister after successfull registration, > > and then tried to call register again. It can lead to crash for example when according module will be unloaded. > > 2) some subsystem is registered twice, for example from different namespaces. > > in this case unregister called during sybsystem cleanup in first namespace will incorrectly remove notifier used > > in second namespace, it also can lead to unexpacted behaviour. > > > So in these two cases, is it more reasonable to trigger BUG() directly when checking for duplicate registration ? > But why does current notifier_chain_register() just trigger WARN() without exiting ? > notifier_chain_cond_register() direct exit without triggering WARN() ? It should recover from this, if it can be detected. The main point is that not all apis have to be this "robust" when used within the kernel as we do allow for the callers to know what they are doing :) If this does not cause any additional problems or slow downs, it's probably fine to add. thanks, greg k-h