[PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Robert Mader]

The message of commit 504e2b4ab97a ("dma-buf/udmabuf: skip redundant cpu sync to
fix cacheline EEXIST warning") says:

> The CPU sync at map/unmap time is also redundant for udmabuf:
> begin_cpu_udmabuf() and end_cpu_udmabuf() already perform explicit
> cache synchronization via dma_sync_sgtable_for_cpu/device() when CPU
> access is requested through the dma-buf interface.

This, however, does not apply to the first time begin_cpu_udmabuf() is
called on an udmabuf, in which case the implementation previously relied on
get_sg_table() to perform the cache synchronisation.

Ensure to call dma_sync_sgtable_for_cpu() in that case as well.

Fixes: 504e2b4ab97a ("dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning")
Signed-off-by: Robert Mader <robert.mader@collabora.com>
---
 drivers/dma-buf/udmabuf.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index bced421c0d65..08f57bc1294d 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -224,21 +224,22 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
 {
 	struct udmabuf *ubuf = buf->priv;
 	struct device *dev = ubuf->device->this_device;
-	int ret = 0;
 
 	if (!ubuf->sg) {
 		ubuf->sg = get_sg_table(dev, buf, direction);
 		if (IS_ERR(ubuf->sg)) {
+			int ret;
+
 			ret = PTR_ERR(ubuf->sg);
 			ubuf->sg = NULL;
+			return ret;
 		} else {
 			ubuf->sg_dir = direction;
 		}
-	} else {
-		dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);
 	}
 
-	return ret;
+	dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);
+	return 0;
 }
 
 static int end_cpu_udmabuf(struct dma_buf *buf,
-- 
2.54.0

Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unprotected concurrent accesses to `ubuf->sg` allow memory and DMA mapping leaks, and expose a TOCTOU race condition that can trigger a kernel panic via an invalid pointer dereference.
- [High] Mismatch between the initial DMA mapping direction and the cache synchronization direction for subsequent CPU accesses violates the DMA API contract.
--

commit aa38d7b2c22ba17dc9edaf6a59e4f9a1d76a592b
Author: Robert Mader <robert.mader@collabora.com>

udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

The message of commit 504e2b4ab97a ("dma-buf/udmabuf: skip redundant cpu sync to
fix cacheline EEXIST warning") says:

> The CPU sync at map/unmap time is also redundant for udmabuf:
> begin_cpu_udmabuf() and end_cpu_udmabuf() already perform explicit
> cache synchronization via dma_sync_sgtable_for_cpu/device() when CPU
> access is requested through the dma-buf interface.

This, however, does not apply to the first time begin_cpu_udmabuf() is
called on an udmabuf, in which case the implementation previously relied on
get_sg_table() to perform the cache synchronisation.

Ensure to call dma_sync_sgtable_for_cpu() in that case as well.

> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index bced421c0d658..08f57bc1294df 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -224,21 +224,22 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
>  {
>  	struct udmabuf *ubuf = buf->priv;
>  	struct device *dev = ubuf->device->this_device;
> -	int ret = 0;
>  
>  	if (!ubuf->sg) {
>  		ubuf->sg = get_sg_table(dev, buf, direction);
>  		if (IS_ERR(ubuf->sg)) {
> +			int ret;
> +
>  			ret = PTR_ERR(ubuf->sg);
>  			ubuf->sg = NULL;
> +			return ret;
>  		} else {
>  			ubuf->sg_dir = direction;
>  		}
> -	} else {
> -		dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this lack of synchronization allow
concurrent threads to see a transient error pointer?

If multiple threads invoke the sync ioctl concurrently, they can race
on the !ubuf->sg check.  If the first thread fails to allocate the
sg_table, it temporarily assigns an error pointer to ubuf->sg before
setting it back to null:

Thread 1:
	if (!ubuf->sg) {
		ubuf->sg = get_sg_table(dev, buf, direction);

Thread 2 could observe ubuf->sg as an error pointer, bypass the null
check, and pass the error pointer to dma_sync_sgtable_for_cpu() further
down, triggering a kernel panic when dereferenced.

Alternatively, if both threads see !ubuf->sg as true, they could both
allocate sg_table structures, leaking one and its dma mapping.

>  
> -	return ret;
> +	dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);

[Severity: High]
This is a pre-existing issue, but does this allow syncing with a different
direction than the original mapping?

If a user issues a sync ioctl with one direction to establish the
sg_table mapping, and then issues a subsequent sync with a different
direction, dma_sync_sgtable_for_cpu() and dma_sync_sgtable_for_device()
will be called using the new direction.

Since the dma mapping rules require sync and map directions to exactly
match unless the mapping is bidirectional, could this lead to incorrect
cache maintenance operations?

> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627105725.9083-1-robert.mader@collabora.com?part=1

Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Mikhail Gavrilov]

On Sat, Jun 27, 2026 at 3:58 PM Robert Mader <robert.mader@collabora.com> wrote:
>
> The message of commit 504e2b4ab97a ("dma-buf/udmabuf: skip redundant cpu sync to
> fix cacheline EEXIST warning") says:
>
> > The CPU sync at map/unmap time is also redundant for udmabuf:
> > begin_cpu_udmabuf() and end_cpu_udmabuf() already perform explicit
> > cache synchronization via dma_sync_sgtable_for_cpu/device() when CPU
> > access is requested through the dma-buf interface.
>
> This, however, does not apply to the first time begin_cpu_udmabuf() is
> called on an udmabuf, in which case the implementation previously relied on
> get_sg_table() to perform the cache synchronisation.
>
> Ensure to call dma_sync_sgtable_for_cpu() in that case as well.
>
> Fixes: 504e2b4ab97a ("dma-buf/udmabuf: skip redundant cpu sync to fix cacheline EEXIST warning")
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> ---
>  drivers/dma-buf/udmabuf.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index bced421c0d65..08f57bc1294d 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -224,21 +224,22 @@ static int begin_cpu_udmabuf(struct dma_buf *buf,
>  {
>         struct udmabuf *ubuf = buf->priv;
>         struct device *dev = ubuf->device->this_device;
> -       int ret = 0;
>
>         if (!ubuf->sg) {
>                 ubuf->sg = get_sg_table(dev, buf, direction);
>                 if (IS_ERR(ubuf->sg)) {
> +                       int ret;
> +
>                         ret = PTR_ERR(ubuf->sg);
>                         ubuf->sg = NULL;
> +                       return ret;
>                 } else {
>                         ubuf->sg_dir = direction;
>                 }
> -       } else {
> -               dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);
>         }
>
> -       return ret;
> +       dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction);
> +       return 0;
>  }
>
>  static int end_cpu_udmabuf(struct dma_buf *buf,
> --
> 2.54.0
>

Hi Robert,

Thanks for catching this. You're right -- with DMA_ATTR_SKIP_CPU_SYNC
in get_sg_table(), the first begin_cpu_udmabuf() call no longer gets
the implicit CPU sync that the dma_map path used to provide, and the
explicit sync in the old else-branch only ran when ubuf->sg already
existed.

The fix correctly moves dma_sync_sgtable_for_cpu() so it runs in both
cases. The logic looks right to me.

This wouldn't reproduce on x86 since DMA is cache-coherent there
(dma_sync_* is a no-op), which is why I missed it -- it only matters
on non-coherent architectures. My apologies for the regression.

Reviewed-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

-- 
Thanks,
Mikhail

Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Robert Mader]

Hi Mikhail,

On 27.06.26 13:11, Mikhail Gavrilov wrote:
> Hi Robert,
>
> Thanks for catching this. You're right -- with DMA_ATTR_SKIP_CPU_SYNC
> in get_sg_table(), the first begin_cpu_udmabuf() call no longer gets
> the implicit CPU sync that the dma_map path used to provide, and the
> explicit sync in the old else-branch only ran when ubuf->sg already
> existed.
>
> The fix correctly moves dma_sync_sgtable_for_cpu() so it runs in both
> cases. The logic looks right to me.
>
> This wouldn't reproduce on x86 since DMA is cache-coherent there
> (dma_sync_* is a no-op), which is why I missed it -- it only matters
> on non-coherent architectures. My apologies for the regression.
>
> Reviewed-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

thanks for the quick review! And no worries, I was very happy about your 
patch, which is why I looked deeper into it in the first place.

Given the patch fixes a newly introduced regression (and is very 
simple), do you think there's a chance to still get it into 7.1 - or 
would it need to go via the usual 7.2/7.3 release schedule + backport?

Best regards

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Mikhail Gavrilov]

On Mon, Jun 29, 2026 at 3:57 PM Robert Mader <robert.mader@collabora.com> wrote:
>
> Hi Mikhail,
>
> thanks for the quick review! And no worries, I was very happy about your
> patch, which is why I looked deeper into it in the first place.
>
> Given the patch fixes a newly introduced regression (and is very
> simple), do you think there's a chance to still get it into 7.1 - or
> would it need to go via the usual 7.2/7.3 release schedule + backport?

Hi Robert,

The timing depends on Vivek and the drm-misc maintainers, not me,
but here's the situation as I understand it:

The original commit 504e2b4ab97a only landed in 7.2 (it went into
drm-misc-next, not drm-misc-fixes). So 7.1 was never affected by
the regression -- there's nothing to fix there.

For 7.2: since your patch fixes a regression introduced in the same
cycle, it would ideally go through drm-misc-fixes to land in 7.2
before release, avoiding a broken commit in the final tree. That's
the cleanest outcome.

Adding Vivek -- he pushed the original patch and would know whether
drm-misc-fixes is the right route at this point in the cycle.

-- 
Thanks,
Mikhail

Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Robert Mader]

On 29.06.26 13:48, Mikhail Gavrilov wrote:
> On Mon, Jun 29, 2026 at 3:57 PM Robert Mader <robert.mader@collabora.com> wrote:
>> Hi Mikhail,
>>
>> thanks for the quick review! And no worries, I was very happy about your
>> patch, which is why I looked deeper into it in the first place.
>>
>> Given the patch fixes a newly introduced regression (and is very
>> simple), do you think there's a chance to still get it into 7.1 - or
>> would it need to go via the usual 7.2/7.3 release schedule + backport?
> Hi Robert,
>
> The timing depends on Vivek and the drm-misc maintainers, not me,
> but here's the situation as I understand it:
>
> The original commit 504e2b4ab97a only landed in 7.2 (it went into
> drm-misc-next, not drm-misc-fixes). So 7.1 was never affected by
> the regression -- there's nothing to fix there.
>
> For 7.2: since your patch fixes a regression introduced in the same
> cycle, it would ideally go through drm-misc-fixes to land in 7.2
> before release, avoiding a broken commit in the final tree. That's
> the cleanest outcome.
>
> Adding Vivek -- he pushed the original patch and would know whether
> drm-misc-fixes is the right route at this point in the cycle.
Hi Mikhail, I of course meant 7.2, not 7.1, sorry! And agreed, that 
would be ideal / perfect.

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Robert Mader]

Hi, sorry for the noise - adding more udmabuf maintainers in case Vivek 
doesn't have time to look into it atm, just to ensure people are aware.

Gerd, Sumit, Christian: this is a simple fix for a minor regression in 
the upcoming 7.2 release - it would therefor be great if anyone could 
pick it to the fixes branch :)

Thanks and best regards,

Robert

On 29.06.26 13:58, Robert Mader wrote:
> On 29.06.26 13:48, Mikhail Gavrilov wrote:
>> On Mon, Jun 29, 2026 at 3:57 PM Robert Mader 
>> <robert.mader@collabora.com> wrote:
>>> Hi Mikhail,
>>>
>>> thanks for the quick review! And no worries, I was very happy about 
>>> your
>>> patch, which is why I looked deeper into it in the first place.
>>>
>>> Given the patch fixes a newly introduced regression (and is very
>>> simple), do you think there's a chance to still get it into 7.1 - or
>>> would it need to go via the usual 7.2/7.3 release schedule + backport?
>> Hi Robert,
>>
>> The timing depends on Vivek and the drm-misc maintainers, not me,
>> but here's the situation as I understand it:
>>
>> The original commit 504e2b4ab97a only landed in 7.2 (it went into
>> drm-misc-next, not drm-misc-fixes). So 7.1 was never affected by
>> the regression -- there's nothing to fix there.
>>
>> For 7.2: since your patch fixes a regression introduced in the same
>> cycle, it would ideally go through drm-misc-fixes to land in 7.2
>> before release, avoiding a broken commit in the final tree. That's
>> the cleanest outcome.
>>
>> Adding Vivek -- he pushed the original patch and would know whether
>> drm-misc-fixes is the right route at this point in the cycle.
> Hi Mikhail, I of course meant 7.2, not 7.1, sorry! And agreed, that 
> would be ideal / perfect.
>
-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

RE: [PATCH v1] udmabuf: Ensure to perform cache synchronisation in begin_cpu_udmabuf()

[Kasireddy, Vivek]

> Subject: Re: [PATCH v1] udmabuf: Ensure to perform cache
> synchronisation in begin_cpu_udmabuf()
> 
> Hi, sorry for the noise - adding more udmabuf maintainers in case Vivek
> doesn't have time to look into it atm, just to ensure people are aware.
> 
> Gerd, Sumit, Christian: this is a simple fix for a minor regression in
> the upcoming 7.2 release - it would therefor be great if anyone could
> pick it to the fixes branch :)
Hi Robert,

Your patch LGTM. If no one else picks it up soon, I'll push it to drm-misc-fixes in the
next few days.

Thanks,
Vivek

> 
> Thanks and best regards,
> 
> Robert
> 
> On 29.06.26 13:58, Robert Mader wrote:
> > On 29.06.26 13:48, Mikhail Gavrilov wrote:
> >> On Mon, Jun 29, 2026 at 3:57 PM Robert Mader
> >> <robert.mader@collabora.com> wrote:
> >>> Hi Mikhail,
> >>>
> >>> thanks for the quick review! And no worries, I was very happy about
> >>> your
> >>> patch, which is why I looked deeper into it in the first place.
> >>>
> >>> Given the patch fixes a newly introduced regression (and is very
> >>> simple), do you think there's a chance to still get it into 7.1 - or
> >>> would it need to go via the usual 7.2/7.3 release schedule +
> backport?
> >> Hi Robert,
> >>
> >> The timing depends on Vivek and the drm-misc maintainers, not me,
> >> but here's the situation as I understand it:
> >>
> >> The original commit 504e2b4ab97a only landed in 7.2 (it went into
> >> drm-misc-next, not drm-misc-fixes). So 7.1 was never affected by
> >> the regression -- there's nothing to fix there.
> >>
> >> For 7.2: since your patch fixes a regression introduced in the same
> >> cycle, it would ideally go through drm-misc-fixes to land in 7.2
> >> before release, avoiding a broken commit in the final tree. That's
> >> the cleanest outcome.
> >>
> >> Adding Vivek -- he pushed the original patch and would know
> whether
> >> drm-misc-fixes is the right route at this point in the cycle.
> > Hi Mikhail, I of course meant 7.2, not 7.1, sorry! And agreed, that
> > would be ideal / perfect.
> >
> --
> Robert Mader
> Consultant Software Developer
> 
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718