Message ID | 20200520131558.23009-1-tomi.valkeinen@ti.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Hans Verkuil |
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 1jbOWj-00FO18-Gn; Wed, 20 May 2020 13:12:55 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726804AbgETNQY (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 20 May 2020 09:16:24 -0400 Received: from lelv0142.ext.ti.com ([198.47.23.249]:51004 "EHLO lelv0142.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726436AbgETNQY (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 20 May 2020 09:16:24 -0400 Received: from fllv0035.itg.ti.com ([10.64.41.0]) by lelv0142.ext.ti.com (8.15.2/8.15.2) with ESMTP id 04KDGIPK075879; Wed, 20 May 2020 08:16:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1589980578; bh=g5mxtIyMDGJIfjMohatgyirw9qrdm5M7LriXPEsGKOs=; h=From:To:CC:Subject:Date; b=Rsc7egtCVTnZ9rJ3AC75KqbJbnXiFZortrfw8p98+O/9/rJfiEL+gPjjKKLLZiXwF JO8Ts9qbYTbLxlZOUdipIsJNH2Lrk+M+sBd53BrZSFD1519giTdG+BIuS3+ATSMbp8 LGkFjXBPKgKFpnJZh3FxGILUcx2pa1/hI6WdlMyE= Received: from DLEE102.ent.ti.com (dlee102.ent.ti.com [157.170.170.32]) by fllv0035.itg.ti.com (8.15.2/8.15.2) with ESMTP id 04KDGIOC094815; Wed, 20 May 2020 08:16:18 -0500 Received: from DLEE111.ent.ti.com (157.170.170.22) by DLEE102.ent.ti.com (157.170.170.32) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1979.3; Wed, 20 May 2020 08:16:17 -0500 Received: from fllv0039.itg.ti.com (10.64.41.19) by DLEE111.ent.ti.com (157.170.170.22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1979.3 via Frontend Transport; Wed, 20 May 2020 08:16:17 -0500 Received: from deskari.lan (ileax41-snat.itg.ti.com [10.172.224.153]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 04KDGGnl086139; Wed, 20 May 2020 08:16:16 -0500 From: Tomi Valkeinen <tomi.valkeinen@ti.com> To: Linux Media Mailing List <linux-media@vger.kernel.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Marek Szyprowski <m.szyprowski@samsung.com>, Ulf Hansson <ulf.hansson@linaro.org> CC: LKML <linux-kernel@vger.kernel.org>, Tomi Valkeinen <tomi.valkeinen@ti.com> Subject: [PATCH] media: videobuf2-dma-contig: fix bad kfree in vb2_dma_contig_clear_max_seg_size Date: Wed, 20 May 2020 16:15:58 +0300 Message-ID: <20200520131558.23009-1-tomi.valkeinen@ti.com> X-Mailer: git-send-email 2.17.1 MIME-Version: 1.0 Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 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 autolearn=ham autolearn_force=no |
Series |
media: videobuf2-dma-contig: fix bad kfree in vb2_dma_contig_clear_max_seg_size
|
|
Commit Message
Tomi Valkeinen
May 20, 2020, 1:15 p.m. UTC
Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform:
Initialize dma_parms for platform devices") in v5.7-rc5 causes
vb2_dma_contig_clear_max_seg_size() to kfree memory that was not
allocated by vb2_dma_contig_set_max_seg_size().
The assumption in vb2_dma_contig_set_max_seg_size() seems to be that
dev->dma_parms is always NULL when the driver is probed, and the case
where dev->dma_parms has bee initialized by someone else than the driver
(by calling vb2_dma_contig_set_max_seg_size) will cause a failure.
All the current users of these functions are platform devices, which now
always have dma_parms set by the driver core. To fix the issue for v5.7,
make vb2_dma_contig_set_max_seg_size() return an error if dma_parms is
NULL to be on the safe side, and remove the kfree code from
vb2_dma_contig_clear_max_seg_size().
For v5.8 we should remove the two functions and move the
dma_set_max_seg_size() calls into the drivers.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
Note: I have only fully tested this on linux-next, as the capture driver
I use doesn't support unloading modules in v5.7.
drivers/media/common/videobuf2/videobuf2-dma-contig.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
Comments
Hi Tomi, On 20.05.2020 15:15, Tomi Valkeinen wrote: > Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform: > Initialize dma_parms for platform devices") in v5.7-rc5 causes > vb2_dma_contig_clear_max_seg_size() to kfree memory that was not > allocated by vb2_dma_contig_set_max_seg_size(). > > The assumption in vb2_dma_contig_set_max_seg_size() seems to be that > dev->dma_parms is always NULL when the driver is probed, and the case > where dev->dma_parms has bee initialized by someone else than the driver > (by calling vb2_dma_contig_set_max_seg_size) will cause a failure. > > All the current users of these functions are platform devices, which now > always have dma_parms set by the driver core. To fix the issue for v5.7, > make vb2_dma_contig_set_max_seg_size() return an error if dma_parms is > NULL to be on the safe side, and remove the kfree code from > vb2_dma_contig_clear_max_seg_size(). > > For v5.8 we should remove the two functions and move the > dma_set_max_seg_size() calls into the drivers. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > > Note: I have only fully tested this on linux-next, as the capture driver > I use doesn't support unloading modules in v5.7. > > drivers/media/common/videobuf2/videobuf2-dma-contig.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > index d3a3ee5b597b..24f80b62ef94 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > @@ -726,9 +726,8 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); > int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size) > { > if (!dev->dma_parms) { > - dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); > - if (!dev->dma_parms) > - return -ENOMEM; > + dev_err(dev, "Failed to set max_seg_size: dma_parms is NULL\n"); > + return -ENODEV; > } > if (dma_get_max_seg_size(dev) < size) > return dma_set_max_seg_size(dev, size); > @@ -747,8 +746,6 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size); > */ > void vb2_dma_contig_clear_max_seg_size(struct device *dev) > { > - kfree(dev->dma_parms); > - dev->dma_parms = NULL; > } > EXPORT_SYMBOL_GPL(vb2_dma_contig_clear_max_seg_size); We don't need to export empty function imho. It can be made an empty static inline in include/media/videobuf2-dma-contig.h. Best regards
On Wed, 20 May 2020 at 15:16, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > Commit 9495b7e92f716ab2bd6814fab5e97ab4a39adfdd ("driver core: platform: > Initialize dma_parms for platform devices") in v5.7-rc5 causes > vb2_dma_contig_clear_max_seg_size() to kfree memory that was not > allocated by vb2_dma_contig_set_max_seg_size(). > > The assumption in vb2_dma_contig_set_max_seg_size() seems to be that > dev->dma_parms is always NULL when the driver is probed, and the case > where dev->dma_parms has bee initialized by someone else than the driver > (by calling vb2_dma_contig_set_max_seg_size) will cause a failure. > > All the current users of these functions are platform devices, which now > always have dma_parms set by the driver core. To fix the issue for v5.7, > make vb2_dma_contig_set_max_seg_size() return an error if dma_parms is > NULL to be on the safe side, and remove the kfree code from > vb2_dma_contig_clear_max_seg_size(). Not entirely true I believe, see more below. > > For v5.8 we should remove the two functions and move the > dma_set_max_seg_size() calls into the drivers. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > > Note: I have only fully tested this on linux-next, as the capture driver > I use doesn't support unloading modules in v5.7. > > drivers/media/common/videobuf2/videobuf2-dma-contig.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > index d3a3ee5b597b..24f80b62ef94 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > @@ -726,9 +726,8 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); > int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size) > { > if (!dev->dma_parms) { > - dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); > - if (!dev->dma_parms) > - return -ENOMEM; > + dev_err(dev, "Failed to set max_seg_size: dma_parms is NULL\n"); > + return -ENODEV; > } > if (dma_get_max_seg_size(dev) < size) > return dma_set_max_seg_size(dev, size); > @@ -747,8 +746,6 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size); > */ > void vb2_dma_contig_clear_max_seg_size(struct device *dev) > { > - kfree(dev->dma_parms); > - dev->dma_parms = NULL; > } > EXPORT_SYMBOL_GPL(vb2_dma_contig_clear_max_seg_size); I think you need something along the lines of this as well: diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c index 5c2a23b953a4..7acf2a03812d 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c @@ -1070,6 +1070,7 @@ static const struct v4l2_file_operations s5p_mfc_fops = { /* DMA memory related helper functions */ static void s5p_mfc_memdev_release(struct device *dev) { + kfree(dev->dma_parms); of_reserved_mem_device_release(dev); } @@ -1090,6 +1091,10 @@ static struct device *s5p_mfc_alloc_memdev(struct device *dev, child->dma_mask = dev->dma_mask; child->release = s5p_mfc_memdev_release; + child->dma_parms = kzalloc(sizeof(*child->dma_parms), GFP_KERNEL); + if (!child->dma_parms) + goto err; + /* * The memdevs are not proper OF platform devices, so in order for them * to be treated as valid DMA masters we need a bit of a hack to force @@ -1105,6 +1110,7 @@ static struct device *s5p_mfc_alloc_memdev(struct device *dev, device_del(child); } +err: put_device(child); return NULL; } Also, please tag the patch for stable. Kind regards Uffe
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index d3a3ee5b597b..24f80b62ef94 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -726,9 +726,8 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size) { if (!dev->dma_parms) { - dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); - if (!dev->dma_parms) - return -ENOMEM; + dev_err(dev, "Failed to set max_seg_size: dma_parms is NULL\n"); + return -ENODEV; } if (dma_get_max_seg_size(dev) < size) return dma_set_max_seg_size(dev, size); @@ -747,8 +746,6 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size); */ void vb2_dma_contig_clear_max_seg_size(struct device *dev) { - kfree(dev->dma_parms); - dev->dma_parms = NULL; } EXPORT_SYMBOL_GPL(vb2_dma_contig_clear_max_seg_size);