Am 04.04.25 um 10:27 schrieb Dan Carpenter:

On Mon, Mar 31, 2025 at 02:02:44PM +0200, Christian König wrote:

...

Am 31.03.25 um 11:45 schrieb Dan Carpenter:

...

Call dma_fence_put(fence) before returning an error on this error path.

Fixes: 70e67aaec2f4 ("dma-buf/sw_sync: Add fence deadline support") Signed-off-by: Dan Carpenter dan.carpenter@linaro.org

---

drivers/dma-buf/sw_sync.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index f5905d67dedb..b7615c5c6cac 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -438,8 +438,10 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return -EINVAL; pt = dma_fence_to_sync_pt(fence);

• if (!pt)

• if (!pt) {

• dma_fence_put(fence);
  

  return -EINVAL;
• }

Good catch.

I think it would be cleaner if we add an error label and then use "ret = -EINVAL; goto error;" here as well as a few lines below when ret is set to -ENOENT.

This way we can also avoid the ret = 0 in the declaration and let the compiler actually check the lifetime of the assignment.

I had some issues with my email and it silently ate a bunch of outgoing email without saving a single trace of anything I had sent. I see this was one that was eaten.

Yeah, AMD had similar problems with receiving mails at the beginning of the year.

Unwind ladders don't work really well for things where you just take it for a little while and then drop it a few lines later. Such as here you take reference and then drop it or you take a lock and then drop it. Normally, you can add things to anywere in the unwind ladder but if you add an unlock to the ladder than you to add a weird bunny hop if the goto isn't holding the lock. It ends up getting confusing. With that kind of thing, I prefer to do the unlock before the goto.

Yeah, completely agree. This is usually also a good indicator that something should be in a separate function.

But this case doesn't apply here, doesn't it?

I mean the solution you created below has a few more lines of code, but if you ask me that is way more readable.

The -EFAULT doesn't need any cleanup and can perfectly stay separate as far as I can see.

Regards, Christian.

free_c: free(c); goto free_b; <-- bunny hop; unlock: unlock(); free_b: free(b); free_a: free(a);

return ret;

regards, dan carpenter

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index f5905d67dedb..22a808995f10 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -438,15 +438,17 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return -EINVAL; pt = dma_fence_to_sync_pt(fence);

• if (!pt)

• return -EINVAL;
  

• if (!pt) {

• ret = -EINVAL;
  

• goto put_fence;
  

• }

spin_lock_irqsave(fence->lock, flags);

• if (test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) {

• data.deadline_ns = ktime_to_ns(pt->deadline);
  

• } else {

• if (!test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { ret = -ENOENT;

• goto unlock;
  

  }
• data.deadline_ns = ktime_to_ns(pt->deadline); spin_unlock_irqrestore(fence->lock, flags);

dma_fence_put(fence); @@ -458,6 +460,13 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return -EFAULT; return 0;

+unlock:

• spin_unlock_irqrestore(fence->lock, flags);

+put_fence:

• dma_fence_put(fence);
• return ret;

} static long sw_sync_ioctl(struct file *file, unsigned int cmd,