Message ID | 20210915120240.21572-2-p.yadav@ti.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Sakari Ailus |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1mQTd1-00BEKO-Eq; Wed, 15 Sep 2021 12:03:04 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237525AbhIOMET (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 15 Sep 2021 08:04:19 -0400 Received: from fllv0016.ext.ti.com ([198.47.19.142]:55142 "EHLO fllv0016.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237623AbhIOMEO (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 15 Sep 2021 08:04:14 -0400 Received: from lelv0265.itg.ti.com ([10.180.67.224]) by fllv0016.ext.ti.com (8.15.2/8.15.2) with ESMTP id 18FC2ofe085397; Wed, 15 Sep 2021 07:02:50 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1631707370; bh=zJ/G+uhDKnKmBBBqmYU+RVWsZ4psvl76j+mA50Q1hn4=; h=From:To:CC:Subject:Date:In-Reply-To:References; b=SfRBDjtL47GoCRgRMD1wy/5we2Fz4K6DaNrxpcXDS439lW5vqg1Yddz5/t8jliRW8 OuVgbSD93+KhAJjGj5Dv5kR5CauztAYSgV+Rd08XkNBsDBNON5yizVHhPejJwW1bdJ g+hJfANMSZW7MdjXjN/NEkIrobLLY3IK2Yl1bHlI= Received: from DLEE112.ent.ti.com (dlee112.ent.ti.com [157.170.170.23]) by lelv0265.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 18FC2ovX068123 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 15 Sep 2021 07:02:50 -0500 Received: from DLEE107.ent.ti.com (157.170.170.37) by DLEE112.ent.ti.com (157.170.170.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.14; Wed, 15 Sep 2021 07:02:49 -0500 Received: from lelv0326.itg.ti.com (10.180.67.84) by DLEE107.ent.ti.com (157.170.170.37) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.14 via Frontend Transport; Wed, 15 Sep 2021 07:02:49 -0500 Received: from pratyush-OptiPlex-790.dhcp.ti.com (ileax41-snat.itg.ti.com [10.172.224.153]) by lelv0326.itg.ti.com (8.15.2/8.15.2) with ESMTP id 18FC2fD2019246; Wed, 15 Sep 2021 07:02:46 -0500 From: Pratyush Yadav <p.yadav@ti.com> To: Mauro Carvalho Chehab <mchehab@kernel.org> CC: Pratyush Yadav <p.yadav@ti.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Nikhil Devshatwar <nikhil.nd@ti.com>, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>, Vignesh Raghavendra <vigneshr@ti.com>, Benoit Parrot <bparrot@ti.com>, Maxime Ripard <mripard@kernel.org>, Rob Herring <robh+dt@kernel.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, =?utf-8?q?Niklas_S=C3=B6derlun?= =?utf-8?q?d?= <niklas.soderlund+renesas@ragnatech.se>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-media@vger.kernel.org> Subject: [PATCH v4 01/11] media: cadence: csi2rx: Unregister v4l2 async notifier Date: Wed, 15 Sep 2021 17:32:30 +0530 Message-ID: <20210915120240.21572-2-p.yadav@ti.com> X-Mailer: git-send-email 2.33.0 In-Reply-To: <20210915120240.21572-1-p.yadav@ti.com> References: <20210915120240.21572-1-p.yadav@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -4.8 (----) X-LSpam-Report: No, score=-4.8 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no |
Series |
CSI2RX support on J721E
|
|
Commit Message
Pratyush Yadav
Sept. 15, 2021, 12:02 p.m. UTC
The notifier is added to the global notifier list when registered. When
the module is removed, the struct csi2rx_priv in which the notifier is
embedded, is destroyed. As a result the notifier list has a reference to
a notifier that no longer exists. This causes invalid memory accesses
when the list is iterated over. Similar for when the probe fails.
Unregister and clean up the notifier to avoid this.
Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver")
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
(no changes since v3)
Changes in v3:
- New in v3.
drivers/media/platform/cadence/cdns-csi2rx.c | 3 +++
1 file changed, 3 insertions(+)
Comments
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:30PM +0530, Pratyush Yadav wrote: > The notifier is added to the global notifier list when registered. When > the module is removed, the struct csi2rx_priv in which the notifier is > embedded, is destroyed. As a result the notifier list has a reference to > a notifier that no longer exists. This causes invalid memory accesses > when the list is iterated over. Similar for when the probe fails. > > Unregister and clean up the notifier to avoid this. > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Note that there are other issues in the driver in cleanup paths, in particular a missing v4l2_async_notifier_cleanup() call in csi2rx_parse_dt() when v4l2_async_notifier_add_fwnode_remote_subdev() fails (moving the one from the other error path to an err label would be best), and missing media_entity_cleanup() calls in both the probe error path and the remove handler. Would you like to submit fixes for those ? > --- > > (no changes since v3) > > Changes in v3: > - New in v3. > > drivers/media/platform/cadence/cdns-csi2rx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 7b44ab2b8c9a..d60975f905d6 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -469,6 +469,7 @@ static int csi2rx_probe(struct platform_device *pdev) > return 0; > > err_cleanup: > + v4l2_async_nf_unregister(&csi2rx->notifier); > v4l2_async_nf_cleanup(&csi2rx->notifier); > err_free_priv: > kfree(csi2rx); > @@ -479,6 +480,8 @@ static int csi2rx_remove(struct platform_device *pdev) > { > struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev); > > + v4l2_async_nf_unregister(&csi2rx->notifier); > + v4l2_async_nf_cleanup(&csi2rx->notifier); > v4l2_async_unregister_subdev(&csi2rx->subdev); > kfree(csi2rx); >
On 07/10/21 02:31AM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Wed, Sep 15, 2021 at 05:32:30PM +0530, Pratyush Yadav wrote: > > The notifier is added to the global notifier list when registered. When > > the module is removed, the struct csi2rx_priv in which the notifier is > > embedded, is destroyed. As a result the notifier list has a reference to > > a notifier that no longer exists. This causes invalid memory accesses > > when the list is iterated over. Similar for when the probe fails. > > > > Unregister and clean up the notifier to avoid this. > > > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Note that there are other issues in the driver in cleanup paths, in > particular a missing v4l2_async_notifier_cleanup() call in > csi2rx_parse_dt() when v4l2_async_notifier_add_fwnode_remote_subdev() > fails (moving the one from the other error path to an err label would be > best), and missing media_entity_cleanup() calls in both the probe error > path and the remove handler. Would you like to submit fixes for those ? Sure, will do. > > > --- > > > > (no changes since v3) > > > > Changes in v3: > > - New in v3. > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 7b44ab2b8c9a..d60975f905d6 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -469,6 +469,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > return 0; > > > > err_cleanup: > > + v4l2_async_nf_unregister(&csi2rx->notifier); > > v4l2_async_nf_cleanup(&csi2rx->notifier); > > err_free_priv: > > kfree(csi2rx); > > @@ -479,6 +480,8 @@ static int csi2rx_remove(struct platform_device *pdev) > > { > > struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev); > > > > + v4l2_async_nf_unregister(&csi2rx->notifier); > > + v4l2_async_nf_cleanup(&csi2rx->notifier); > > v4l2_async_unregister_subdev(&csi2rx->subdev); > > kfree(csi2rx); > > > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c index 7b44ab2b8c9a..d60975f905d6 100644 --- a/drivers/media/platform/cadence/cdns-csi2rx.c +++ b/drivers/media/platform/cadence/cdns-csi2rx.c @@ -469,6 +469,7 @@ static int csi2rx_probe(struct platform_device *pdev) return 0; err_cleanup: + v4l2_async_nf_unregister(&csi2rx->notifier); v4l2_async_nf_cleanup(&csi2rx->notifier); err_free_priv: kfree(csi2rx); @@ -479,6 +480,8 @@ static int csi2rx_remove(struct platform_device *pdev) { struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev); + v4l2_async_nf_unregister(&csi2rx->notifier); + v4l2_async_nf_cleanup(&csi2rx->notifier); v4l2_async_unregister_subdev(&csi2rx->subdev); kfree(csi2rx);