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 <[email protected]>
Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices")
Cc: [email protected]
---
Changes in v2:
* vb2_dma_contig_clear_max_seg_size to empty static inline
* Added cc: stable and fixes tag
.../common/videobuf2/videobuf2-dma-contig.c | 20 ++-----------------
include/media/videobuf2-dma-contig.h | 2 +-
2 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d3a3ee5b597b..f4b4a7c135eb 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);
@@ -737,21 +736,6 @@ int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
}
EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
-/*
- * vb2_dma_contig_clear_max_seg_size() - release resources for DMA parameters
- * @dev: device for configuring DMA parameters
- *
- * This function releases resources allocated to configure DMA parameters
- * (see vb2_dma_contig_set_max_seg_size() function). It should be called from
- * device drivers on driver remove.
- */
-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);
-
MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
MODULE_AUTHOR("Pawel Osciak <[email protected]>");
MODULE_LICENSE("GPL");
diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
index 5604818d137e..5be313cbf7d7 100644
--- a/include/media/videobuf2-dma-contig.h
+++ b/include/media/videobuf2-dma-contig.h
@@ -25,7 +25,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
}
int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size);
-void vb2_dma_contig_clear_max_seg_size(struct device *dev);
+static inline void vb2_dma_contig_clear_max_seg_size(struct device *dev) { }
extern const struct vb2_mem_ops vb2_dma_contig_memops;
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 27.05.2020 10:23, 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 <[email protected]>
> Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices")
> Cc: [email protected]
Acked-by: Marek Szyprowski <[email protected]>
> ---
>
> Changes in v2:
> * vb2_dma_contig_clear_max_seg_size to empty static inline
> * Added cc: stable and fixes tag
>
> .../common/videobuf2/videobuf2-dma-contig.c | 20 ++-----------------
> include/media/videobuf2-dma-contig.h | 2 +-
> 2 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index d3a3ee5b597b..f4b4a7c135eb 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);
> @@ -737,21 +736,6 @@ int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
> }
> EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
>
> -/*
> - * vb2_dma_contig_clear_max_seg_size() - release resources for DMA parameters
> - * @dev: device for configuring DMA parameters
> - *
> - * This function releases resources allocated to configure DMA parameters
> - * (see vb2_dma_contig_set_max_seg_size() function). It should be called from
> - * device drivers on driver remove.
> - */
> -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);
> -
> MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
> MODULE_AUTHOR("Pawel Osciak <[email protected]>");
> MODULE_LICENSE("GPL");
> diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
> index 5604818d137e..5be313cbf7d7 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -25,7 +25,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
> }
>
> int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size);
> -void vb2_dma_contig_clear_max_seg_size(struct device *dev);
> +static inline void vb2_dma_contig_clear_max_seg_size(struct device *dev) { }
>
> extern const struct vb2_mem_ops vb2_dma_contig_memops;
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Wed, 27 May 2020 at 10:23, Tomi Valkeinen <[email protected]> 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 <[email protected]>
> Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices")
> Cc: [email protected]
Thanks for fixing this!
However, as I tried to point out in v1, don't you need to care about
drivers/media/platform/s5p-mfc/s5p_mfc.c, which allocates its own type
of struct device (non-platform). No?
Kind regards
Uffe
> ---
>
> Changes in v2:
> * vb2_dma_contig_clear_max_seg_size to empty static inline
> * Added cc: stable and fixes tag
>
> .../common/videobuf2/videobuf2-dma-contig.c | 20 ++-----------------
> include/media/videobuf2-dma-contig.h | 2 +-
> 2 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index d3a3ee5b597b..f4b4a7c135eb 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);
> @@ -737,21 +736,6 @@ int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
> }
> EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
>
> -/*
> - * vb2_dma_contig_clear_max_seg_size() - release resources for DMA parameters
> - * @dev: device for configuring DMA parameters
> - *
> - * This function releases resources allocated to configure DMA parameters
> - * (see vb2_dma_contig_set_max_seg_size() function). It should be called from
> - * device drivers on driver remove.
> - */
> -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);
> -
> MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
> MODULE_AUTHOR("Pawel Osciak <[email protected]>");
> MODULE_LICENSE("GPL");
> diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
> index 5604818d137e..5be313cbf7d7 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -25,7 +25,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
> }
>
> int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size);
> -void vb2_dma_contig_clear_max_seg_size(struct device *dev);
> +static inline void vb2_dma_contig_clear_max_seg_size(struct device *dev) { }
>
> extern const struct vb2_mem_ops vb2_dma_contig_memops;
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
Hi Ulf,
On 28.05.2020 11:14, Ulf Hansson wrote:
> On Wed, 27 May 2020 at 10:23, Tomi Valkeinen <[email protected]> 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 <[email protected]>
>> Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices")
>> Cc: [email protected]
> Thanks for fixing this!
>
> However, as I tried to point out in v1, don't you need to care about
> drivers/media/platform/s5p-mfc/s5p_mfc.c, which allocates its own type
> of struct device (non-platform). No?
I will send a patch for the S5P MFC driver separately. It is not so
critical, because the mentioned 2port memory case is not used on any
board with the default exynos_defconfig from mainline. On Exynos4-based
boards it is only used when IOMMU is disabled. It is mainly for
S5PV210/S5PC110 boards, which are still not fully functional after
conversion to device-tree.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On 28/05/2020 12:14, Ulf Hansson wrote:
> On Wed, 27 May 2020 at 10:23, Tomi Valkeinen <[email protected]> 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 <[email protected]>
>> Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices")
>> Cc: [email protected]
>
> Thanks for fixing this!
>
> However, as I tried to point out in v1, don't you need to care about
> drivers/media/platform/s5p-mfc/s5p_mfc.c, which allocates its own type
> of struct device (non-platform). No?
Oh my bad. I thought Marek posted a patch for it, but now that I look, Marek's patch was for
ExynosDRM. Somehow I managed to mix up that with the s5p in my head.
I'll try to find time to look at s5p too, but if anyone gets there first, feel free to fix it.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Thu, 28 May 2020 at 11:24, Marek Szyprowski <[email protected]> wrote:
>
> Hi Ulf,
>
> On 28.05.2020 11:14, Ulf Hansson wrote:
> > On Wed, 27 May 2020 at 10:23, Tomi Valkeinen <[email protected]> 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 <[email protected]>
> >> Fixes: 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices")
> >> Cc: [email protected]
> > Thanks for fixing this!
> >
> > However, as I tried to point out in v1, don't you need to care about
> > drivers/media/platform/s5p-mfc/s5p_mfc.c, which allocates its own type
> > of struct device (non-platform). No?
>
> I will send a patch for the S5P MFC driver separately. It is not so
> critical, because the mentioned 2port memory case is not used on any
> board with the default exynos_defconfig from mainline. On Exynos4-based
> boards it is only used when IOMMU is disabled. It is mainly for
> S5PV210/S5PC110 boards, which are still not fully functional after
> conversion to device-tree.
Okay, makes sense to me. Feel free to add:
Reviewed-by: Ulf Hansson <[email protected]>
For the s5p_mfc issue, here's how I would have done it (just pick the
code if you want).
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;
}
Kind regards
Uffe