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=-11.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 D7246C48BDF for ; Thu, 10 Jun 2021 10:11:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C0B1A613BD for ; Thu, 10 Jun 2021 10:11:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230151AbhFJKNt (ORCPT ); Thu, 10 Jun 2021 06:13:49 -0400 Received: from mail.kernel.org ([198.145.29.99]:49768 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230084AbhFJKNr (ORCPT ); Thu, 10 Jun 2021 06:13:47 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id CC7EE61184; Thu, 10 Jun 2021 10:11:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1623319911; bh=Li5h7gAN6SkiNXdFPfRuFav/iBGrnBV+vWpPj8iqHjw=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=VtYEHv4uHH9pqFlj/1pMAVoF2+01G3tSwYM6JWXMMwHJyxzBZmnGy6K/2xTP4ijlG FHcxE114Zyad4EDxVneVSUXi6/uh3n0+stKWPsplHbffMxIb+qeF/Ez9H9dGx3Ubhv jbjDDy018iSOYgquDg7te8npJ6ATZWLOqMYNdWPbktJ60/hMC2dqkxluSM+KK6tZ/E lRcwRfJSLsEvtc2M7hflKh+qLHinaSDfiInA+PuxV9Hi9wgzNMGdHA9ieVp6CzlCwn 1/fePN9W3wbFeyjFbcFR5g+0ChOB5Pk6vmPSLoLk+ZxEo1hUIOkIOqbhanoNVLfk0D b20ahfroBwpUg== From: Felipe Balbi To: Jack Pham Cc: Alexandru Elisei , Greg Kroah-Hartman , p.zabel@pengutronix.de, linux-usb@vger.kernel.org, Linux Kernel Mailing List , arm-mail-list , sanm@codeaurora.org Subject: Re: [BUG] usb: dwc3: Kernel NULL pointer dereference in dwc3_remove() In-Reply-To: <20210607180023.GA23045@jackp-linux.qualcomm.com> References: <87r1hjcvf6.fsf@kernel.org> <70be179c-d36b-de6f-6efc-2888055b1312@arm.com> <8272121c-ac8a-1565-a047-e3a16dcf13b0@arm.com> <877djbc8xq.fsf@kernel.org> <20210603173632.GA25299@jackp-linux.qualcomm.com> <87mts6avnn.fsf@kernel.org> <20210607180023.GA23045@jackp-linux.qualcomm.com> Date: Thu, 10 Jun 2021 13:11:42 +0300 Message-ID: <87sg1q1129.fsf@kernel.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Jack Pham writes: > On Fri, Jun 04, 2021 at 11:20:12AM +0300, Felipe Balbi wrote: >> Jack Pham writes: >> >> >>>> Alexandru Elisei writes: >> >> >>>>> I've been able to bisect the panic and the offending commit is = 568262bf5492 ("usb: >> >> >>>>> dwc3: core: Add shutdown callback for dwc3"). I can provide mor= e diagnostic >> >> >>>>> information if needed and I can help test the fix. >> >> >>>> if you simply revert that commit in HEAD, does the problem reall= y go >> >> >>>> away? >> >> >>> Kernel built from commit 324c92e5e0ee, which is the kernel tip to= day, the panic is >> >> >>> there. Reverting the offending commit, 568262bf5492, makes the pa= nic disappear. >> >> >> Want to send a revert so I can take it now? >> >> > >> >> > I can send a revert, but Felipe was asking Sandeep (the commit auth= or) for a fix, >> >> > so I'll leave it up to Felipe to decide how to proceed. >> >>=20 >> >> I'm okay with a revert. Feel free to add my Acked-by: Felipe Balbi >> >> or it. >> >>=20 >> >> Sandeep, please send a new version that doesn't encounter the same >> >> issue. Make sure to test by reloading the driver in a tight loop for >> >> several iterations. >> > >> > This would probably be tricky to test on other "glue" drivers as the >> > problem appears to be specific only to dwc3_of_simple. It looks like >> > both dwc3_of_simple and the dwc3 core now (due to 568262bf5492) each >> > implement respective .shutdown callbacks. The latter is simply a wrapp= er >> > around dwc3_remove(). And from the panic call stack above we see that >> > dwc3_of_simple_shutdown() calls of_platform_depopulate() which will=20 >> > again call dwc3_remove() resulting in the double remove. >> > >> > So would an alternative approach be to protect against dwc3_remove() >> > getting called multiple times? IMO it'd be a bit messy to have to add >>=20 >> no, I don't think so. That sounds like a workaround. We should be able >> to guarantee that ->remove() doesn't get called twice using the driver >> model properly. > > Completely fair. So then having a .shutdown callback that directly calls > dwc3_remove() is probably not the right thing to do as it completely > bypasses the driver model so if and when the driver core does later > release the device from the driver that's how we end up with the double > remove. yeah, I would agree with that. >> > additional checks there to know if it had already been called. So maybe >> > avoid it altogether--should dwc3_of_simple_shutdown() just skip calling >> > of_platform_depopulate()? >>=20 >> I don't know what the idiomatic is nowadays, but at least early on, we >> had to call depopulate. > > So any suggestions on how to fix the original issue Sandeep was trying > to fix with 568262bf5492? Maybe implement .shutdown in dwc3_qcom and have > it follow what dwc3_of_simple does with of_platform_depopulate()? But > then wouldn't other "glues" want/need to follow suit? I think we can implement shutdown in core, but we need to careful with it. Instead of just blindly calling remove, let's extract the common parts to another internal function that both remove and shutdown call. debugfs removal should not be part of that generic method :-) Anything in that generic method should, probably, be idempotent. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQFFBAEBCAAvFiEE9DumQ60WEZ09LIErzlfNM9wDzUgFAmDB5V4RHGJhbGJpQGtl cm5lbC5vcmcACgkQzlfNM9wDzUg9Ewf+NproMS9BtlaBnZq0EMVqyl8gHjrytzkc nnc//13H/V63dPFSZUjvUx35eVZj227lpFaX8+kjBO45BrhnzrBwIvE3xryPYS5S pvE9PZHMwCr+Ic4lt0g+hx8+bp4pePULCLzZnCnfL3BH4KWg9NOg2k4/DgHfmPC3 GRu/MqivsR30ErtiM91ILkS144AIYVlxtQEIz9ghbHM4srMaMgW5MPegMO1BFZwz PVOKcSZ30FJkK733eX1Kxz55gv50wBsCgeZorsaoFP/ruMeCWBe5thkyBJfYi34Y MKkzst3/zGDomIkN6ReZbfJFXMw3xB3Wkzx9231riSWS4sonSN0L3g== =/AIV -----END PGP SIGNATURE----- --=-=-=-- 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=-9.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham 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 7D708C47094 for ; Thu, 10 Jun 2021 10:13:49 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 46CF2613BD for ; Thu, 10 Jun 2021 10:13:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 46CF2613BD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: MIME-Version:Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=FfoYORqBzx7b3Zjiy22MCeGTgXPRLoCksSjHNGTh8D0=; b=KEc2Zutw4fpH8zyE7yzfJEgKiT Eny6ayCDXLlImJL0ovg1XZC6pGSWSbYfLmTbUgMIlopthFoBJ72AcIR2EFTC/OpbZ0XE0wPlZ/eI6 +Yfu6FLR5dri9JoP6oh3JQridU4HWV9E40QPMcBYkdKkxEgPIAxEdeRKV2EgWsBhzjm7gyWWorxx2 GjC8kEyirZaN8cGOz79j+aIYB4HGBnm5Uxu1lC2fgV9qj/4Yt1cewDJs0PyPnqLyS8Fwn9J0uR+AU MHbSNSAfo8BQzovzgw6A043agpPiROIhQ6Ejl6RBRnAUD6NskVBuo1nGLLmeYj9ibNuUdaXka/QSD ZAHLnWuA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lrHfH-000OkX-Vt; Thu, 10 Jun 2021 10:11:56 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lrHfE-000Ojv-Pg for linux-arm-kernel@lists.infradead.org; Thu, 10 Jun 2021 10:11:54 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id CC7EE61184; Thu, 10 Jun 2021 10:11:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1623319911; bh=Li5h7gAN6SkiNXdFPfRuFav/iBGrnBV+vWpPj8iqHjw=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=VtYEHv4uHH9pqFlj/1pMAVoF2+01G3tSwYM6JWXMMwHJyxzBZmnGy6K/2xTP4ijlG FHcxE114Zyad4EDxVneVSUXi6/uh3n0+stKWPsplHbffMxIb+qeF/Ez9H9dGx3Ubhv jbjDDy018iSOYgquDg7te8npJ6ATZWLOqMYNdWPbktJ60/hMC2dqkxluSM+KK6tZ/E lRcwRfJSLsEvtc2M7hflKh+qLHinaSDfiInA+PuxV9Hi9wgzNMGdHA9ieVp6CzlCwn 1/fePN9W3wbFeyjFbcFR5g+0ChOB5Pk6vmPSLoLk+ZxEo1hUIOkIOqbhanoNVLfk0D b20ahfroBwpUg== From: Felipe Balbi To: Jack Pham Cc: Alexandru Elisei , Greg Kroah-Hartman , p.zabel@pengutronix.de, linux-usb@vger.kernel.org, Linux Kernel Mailing List , arm-mail-list , sanm@codeaurora.org Subject: Re: [BUG] usb: dwc3: Kernel NULL pointer dereference in dwc3_remove() In-Reply-To: <20210607180023.GA23045@jackp-linux.qualcomm.com> References: <87r1hjcvf6.fsf@kernel.org> <70be179c-d36b-de6f-6efc-2888055b1312@arm.com> <8272121c-ac8a-1565-a047-e3a16dcf13b0@arm.com> <877djbc8xq.fsf@kernel.org> <20210603173632.GA25299@jackp-linux.qualcomm.com> <87mts6avnn.fsf@kernel.org> <20210607180023.GA23045@jackp-linux.qualcomm.com> Date: Thu, 10 Jun 2021 13:11:42 +0300 Message-ID: <87sg1q1129.fsf@kernel.org> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210610_031152_894653_F10A4DE2 X-CRM114-Status: GOOD ( 30.10 ) 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: multipart/mixed; boundary="===============6444540466226811162==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============6444540466226811162== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Jack Pham writes: > On Fri, Jun 04, 2021 at 11:20:12AM +0300, Felipe Balbi wrote: >> Jack Pham writes: >> >> >>>> Alexandru Elisei writes: >> >> >>>>> I've been able to bisect the panic and the offending commit is = 568262bf5492 ("usb: >> >> >>>>> dwc3: core: Add shutdown callback for dwc3"). I can provide mor= e diagnostic >> >> >>>>> information if needed and I can help test the fix. >> >> >>>> if you simply revert that commit in HEAD, does the problem reall= y go >> >> >>>> away? >> >> >>> Kernel built from commit 324c92e5e0ee, which is the kernel tip to= day, the panic is >> >> >>> there. Reverting the offending commit, 568262bf5492, makes the pa= nic disappear. >> >> >> Want to send a revert so I can take it now? >> >> > >> >> > I can send a revert, but Felipe was asking Sandeep (the commit auth= or) for a fix, >> >> > so I'll leave it up to Felipe to decide how to proceed. >> >>=20 >> >> I'm okay with a revert. Feel free to add my Acked-by: Felipe Balbi >> >> or it. >> >>=20 >> >> Sandeep, please send a new version that doesn't encounter the same >> >> issue. Make sure to test by reloading the driver in a tight loop for >> >> several iterations. >> > >> > This would probably be tricky to test on other "glue" drivers as the >> > problem appears to be specific only to dwc3_of_simple. It looks like >> > both dwc3_of_simple and the dwc3 core now (due to 568262bf5492) each >> > implement respective .shutdown callbacks. The latter is simply a wrapp= er >> > around dwc3_remove(). And from the panic call stack above we see that >> > dwc3_of_simple_shutdown() calls of_platform_depopulate() which will=20 >> > again call dwc3_remove() resulting in the double remove. >> > >> > So would an alternative approach be to protect against dwc3_remove() >> > getting called multiple times? IMO it'd be a bit messy to have to add >>=20 >> no, I don't think so. That sounds like a workaround. We should be able >> to guarantee that ->remove() doesn't get called twice using the driver >> model properly. > > Completely fair. So then having a .shutdown callback that directly calls > dwc3_remove() is probably not the right thing to do as it completely > bypasses the driver model so if and when the driver core does later > release the device from the driver that's how we end up with the double > remove. yeah, I would agree with that. >> > additional checks there to know if it had already been called. So maybe >> > avoid it altogether--should dwc3_of_simple_shutdown() just skip calling >> > of_platform_depopulate()? >>=20 >> I don't know what the idiomatic is nowadays, but at least early on, we >> had to call depopulate. > > So any suggestions on how to fix the original issue Sandeep was trying > to fix with 568262bf5492? Maybe implement .shutdown in dwc3_qcom and have > it follow what dwc3_of_simple does with of_platform_depopulate()? But > then wouldn't other "glues" want/need to follow suit? I think we can implement shutdown in core, but we need to careful with it. Instead of just blindly calling remove, let's extract the common parts to another internal function that both remove and shutdown call. debugfs removal should not be part of that generic method :-) Anything in that generic method should, probably, be idempotent. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQFFBAEBCAAvFiEE9DumQ60WEZ09LIErzlfNM9wDzUgFAmDB5V4RHGJhbGJpQGtl cm5lbC5vcmcACgkQzlfNM9wDzUg9Ewf+NproMS9BtlaBnZq0EMVqyl8gHjrytzkc nnc//13H/V63dPFSZUjvUx35eVZj227lpFaX8+kjBO45BrhnzrBwIvE3xryPYS5S pvE9PZHMwCr+Ic4lt0g+hx8+bp4pePULCLzZnCnfL3BH4KWg9NOg2k4/DgHfmPC3 GRu/MqivsR30ErtiM91ILkS144AIYVlxtQEIz9ghbHM4srMaMgW5MPegMO1BFZwz PVOKcSZ30FJkK733eX1Kxz55gv50wBsCgeZorsaoFP/ruMeCWBe5thkyBJfYi34Y MKkzst3/zGDomIkN6ReZbfJFXMw3xB3Wkzx9231riSWS4sonSN0L3g== =/AIV -----END PGP SIGNATURE----- --=-=-=-- --===============6444540466226811162== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============6444540466226811162==--