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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6F0EEC433F5 for ; Fri, 31 Dec 2021 18:58:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ge+7sa4UyxkpGxoMnBW85dLinA0ko8dFAuZPVi9uOto=; b=1q61vTQ+Aizfwc mFBaCF5xFzRZYJGlFoqUowoQbz2YgjODrsX+Uyry5YGbnONL8HDhb0e3p8ve5w0784yxWx9Hk4MiJ zOOOMMTLHNaCxBhtkCnKvOr+O8hIDmqFdaXxq94LE/K3xX2jC//wTxWjpMLSVNzp0ql/3KORvEKyM GYAbXXUDTTShb2hxwNOz5iS5++azW1/3T6EH0yRmja0JBTUxShHTlZHQb3ftzsQurvpkAVLZ3zccV 7KuM9OalJtgNNpDU/lVqaQvB4fxJc+Eg+05ediao+WCs8DMvvcWIi9aX6EzYV7aCPAtaDjOgoftkM zIurW4mAER6QqhHPKL+Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n3N4c-006T3A-18; Fri, 31 Dec 2021 18:56:18 +0000 Received: from mail-wr1-x42d.google.com ([2a00:1450:4864:20::42d]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n3N4Y-006T2h-JJ; Fri, 31 Dec 2021 18:56:16 +0000 Received: by mail-wr1-x42d.google.com with SMTP id i22so57222597wrb.13; Fri, 31 Dec 2021 10:56:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=XRfpr7i3YGNRNX+sqa8uo1YTDTIBpi+zjFAQkEMU4QY=; b=k6gdIM6pyiOdhwcd7Yddxv5Od0x2vC3baD9lr+K7rDR+ZqYhitE4PWr8n4+ZhhP36+ PB68qWV5bmsWOkmK/h6NIMffgSY4Hi9BnHa39LxI56+RwgIf1+vpnSuSAfitXuRoQyQq 7XPxIj+PV+SvkGNpsYgbT5qwXj4BS9X7I1b0OsnWlz3hF5SM4ka1IOwbG7GQrhkduFid u7FtaD/MguCCYOI4TPafF+s+nBvMtglgVZA4Nofex3vED+z5UR7rNGw0n2N4QLEnTDKj wNdaowr1U3lzn5nNOCelXU0H4CmDFTtnTeKndFsNqMPssWQMP0TLsvyuP0RBdUUTg5ca aqVw== 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:user-agent; bh=XRfpr7i3YGNRNX+sqa8uo1YTDTIBpi+zjFAQkEMU4QY=; b=mzZVH8pKq9ljPlbvCamApAFHY/lIc6TFcBcj7F+CkF8tO99cxBxBDUY8TVYBPItT75 87JVbE+ZLuydSDQNXHsW2Q9Sp4EF24yb7suhAblMQNyr5uqSApDxtI1OUNdawLnI663d Av1iC/OsvBnjvsDXPfmSX2YNdmZcD3e5ezRfWG2S8Sq5nIiSts58Pnv2Zd+OgIHOdu3e B5dmWwB224asKwiYrXWo7TssbYut3eqI1nIV2EhOD6ggXFyp89Q4lEAFCCtoW6+npoXz KHyTdcb4ONMsWscoVZW/ImWyw6QkrcYh3zu7qFff4OQlnsH9/gu3+KNuJXGw4/TBIlZg YA/A== X-Gm-Message-State: AOAM533i03Ed2x4t3Qj5NHRIICddfzOF9OsYah5Ib5P5RlL8spQ8WpRX kuJ0Hn4EsaUVK+tHP3ex4A3EyOrA2uicfa7+ X-Google-Smtp-Source: ABdhPJw3U7gOLz7Dj+wHwrpDAZe/nshH5EUBFQgXENLjhVcz1NdMBQjc5fmkNxL3xmhlZ6wZPByQZw== X-Received: by 2002:a05:6000:2aa:: with SMTP id l10mr30698378wry.518.1640976972309; Fri, 31 Dec 2021 10:56:12 -0800 (PST) Received: from pswork (i5E86B4D2.versanet.de. [94.134.180.210]) by smtp.gmail.com with ESMTPSA id g198sm30659446wme.23.2021.12.31.10.56.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 31 Dec 2021 10:56:11 -0800 (PST) Date: Fri, 31 Dec 2021 19:56:11 +0100 From: Padmanabha Srinivasaiah To: Stefan Wahren Cc: linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, nsaenz@kernel.org, Gaston Gonzalez , Ojaswin Mujoo , Arnd Bergmann , Phil Elwell , bcm-kernel-feedback-list@broadcom.com Subject: Re: [PATCH v2] staging: vc04_services: Fix RCU dereference check Message-ID: <20211231185611.GA4463@pswork> References: <20211230133430.GA10256@pswork> <20211230145415.11962-1-treasure4paddy@gmail.com> <1f52892e-ca86-33e4-25da-eb0b40f45b04@i2se.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1f52892e-ca86-33e4-25da-eb0b40f45b04@i2se.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211231_105614_688324_38D2241A X-CRM114-Status: GOOD ( 24.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Dec 30, 2021 at 10:39:58PM +0100, Stefan Wahren wrote: > Hi Padmanabha, > > Am 30.12.21 um 15:54 schrieb Padmanabha Srinivasaiah: > > In service_callback path RCU dereferenced pointer struct vchiq_service > > need to be accessed inside rcu read-critical section. > > > > Accessing same with rcu_read_[lock/unlock] fixes the issue. > > > > [ 32.201659] ============================= > > [ 32.201664] WARNING: suspicious RCU usage > > [ 32.201670] 5.15.11-rt24-v8+ #3 Not tainted > > [ 32.201680] ----------------------------- > > [ 32.201685] drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h:529 suspicious rcu_dereference_check() usage! > > [ 32.201695] > > [ 32.201695] other info that might help us debug this: > > [ 32.201695] > > [ 32.201700] > > [ 32.201700] rcu_scheduler_active = 2, debug_locks = 1 > > [ 32.201708] no locks held by vchiq-slot/0/98. > > [ 32.201715] > > [ 32.201715] stack backtrace: > > [ 32.201723] CPU: 1 PID: 98 Comm: vchiq-slot/0 Not tainted 5.15.11-rt24-v8+ #3 > > [ 32.201733] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT) > > [ 32.201739] Call trace: > > [ 32.201742] dump_backtrace+0x0/0x1b8 > > [ 32.201772] show_stack+0x20/0x30 > > [ 32.201784] dump_stack_lvl+0x8c/0xb8 > > [ 32.201799] dump_stack+0x18/0x34 > > [ 32.201808] lockdep_rcu_suspicious+0xe4/0xf8 > > [ 32.201817] service_callback+0x124/0x400 > > [ 32.201830] slot_handler_func+0xf60/0x1e20 > > [ 32.201839] kthread+0x19c/0x1a8 > > [ 32.201849] ret_from_fork+0x10/0x20 > > > > Signed-off-by: Padmanabha Srinivasaiah > > --- > > Changes in v2: > > RCU dereferenced pointer need to be accessed inside rcu > > read-side critical section. > > > > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > index 6759a6261500..8ddd400ab2c3 100644 > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > @@ -1053,24 +1053,30 @@ service_callback(enum vchiq_reason reason, struct vchiq_header *header, > > struct vchiq_service *service; > > struct vchiq_instance *instance; > > bool skip_completion = false; > > + unsigned int localport; > > > > DEBUG_INITIALISE(g_state.local); > > > > DEBUG_TRACE(SERVICE_CALLBACK_LINE); > > > > + rcu_read_lock(); > > service = handle_to_service(handle); > > - if (WARN_ON(!service)) > > + if (WARN_ON(!service)) { > > + rcu_read_unlock(); > > return VCHIQ_SUCCESS; > > + } > > > > user_service = (struct user_service *)service->base.userdata; > > user_service is part of the service struct and it's modification below > in this function is protected by a spinlock ( msg_queue_spinlock ). So i > would expected that all read accesses to user_service before the > spinlock are protected by RCU. After applying this patch there would be > still the check for "user_service->is_vchi" unprotected. But i'm not > sure about this. > Thank you stefan for identfying it. Yes, userdata/user_service can be potentially released after graceperiod. Also here pointer is used around different synchronization mechanism, taking an extra reference will keep semantics simpler and will not prolong the graceperiod. Will regenerate the patch again. > Best regards > > > instance = user_service->instance; > > + localport = service->localport; > > + rcu_read_unlock(); > > > > if (!instance || instance->closing) > > return VCHIQ_SUCCESS; > > > > vchiq_log_trace(vchiq_arm_log_level, > > "%s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx", > > - __func__, (unsigned long)user_service, service->localport, > > + __func__, (unsigned long)user_service, (int)localport, > > user_service->userdata, reason, (unsigned long)header, > > (unsigned long)instance, (unsigned long)bulk_userdata); > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel