Re: [PATCH v19 28/40] dept: assign unique dept_key to each distinct dma fence caller

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly introduced statement expression macros for DMA fence waiting evaluate their timeout argument (`t`) multiple times.
- [Medium] Local variable shadowing in the statement expression macros due to the unhygienic `__ret` variable declaration.
--

commit 4d4caad8d173eededcba1493c060f5ddecbd6b70
Author: Byungchul Park <[email protected]>

dept: assign unique dept_key to each distinct dma fence caller

dma fence can be used at various points in the code and it's very hard
to distinguish dma fences between different usages.  Using a single
dept_key for all the dma fences could trigger false positive reports.

> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index d4c92fd350924..3732849a30b7e 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h

[ ... ]

> @@ -370,8 +370,22 @@ bool dma_fence_check_and_signal_locked(struct dma_fence *fence);
>  void dma_fence_signal_locked(struct dma_fence *fence);
>  void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
>  void dma_fence_signal_timestamp_locked(struct dma_fence *fence, ktime_t timestamp);
> -signed long dma_fence_default_wait(struct dma_fence *fence,
> +signed long __dma_fence_default_wait(struct dma_fence *fence,
>  				   bool intr, signed long timeout);
> +
> +/*
> + * Associate every caller with its own dept map.
> + */
> +#define dma_fence_default_wait(f, intr, t)				\
> +({									\
> +	signed long __ret;						\

[Severity: Medium]
Could this local variable declaration lead to shadowing issues? If a caller
passes arguments that reference a variable named __ret in their own scope,
this uninitialized local variable will shadow the caller's variable, causing
undefined behavior.

> +									\
> +	sdt_might_sleep_start_timeout(NULL, t);				\
> +	__ret = __dma_fence_default_wait(f, intr, t);			\

[Severity: Medium]
Is it safe to evaluate the timeout argument t multiple times here?

If a caller passes an expression with side effects, such as timeout--, it
would be evaluated twice, which could lead to incorrect timeout logic.

> +	sdt_might_sleep_end();						\
> +	__ret;								\
> +})
> +

[ ... ]

> @@ -628,12 +642,37 @@ static inline ktime_t dma_fence_timestamp(struct dma_fence *fence)
>  	return fence->timestamp;
>  }
>  
> -signed long dma_fence_wait_timeout(struct dma_fence *,
> +signed long __dma_fence_wait_timeout(struct dma_fence *,
>  				   bool intr, signed long timeout);
> -signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
> +signed long __dma_fence_wait_any_timeout(struct dma_fence **fences,
>  				       uint32_t count,
>  				       bool intr, signed long timeout,
>  				       uint32_t *idx);
> +/*
> + * Associate every caller with its own dept map.
> + */
> +#define dma_fence_wait_timeout(f, intr, t)				\
> +({									\
> +	signed long __ret;						\
> +									\
> +	sdt_might_sleep_start_timeout(NULL, t);				\
> +	__ret = __dma_fence_wait_timeout(f, intr, t);			\

[Severity: Medium]
Does this code also suffer from the same macro hygiene issues with shadowing
__ret and evaluating the t argument twice?

> +	sdt_might_sleep_end();						\
> +	__ret;								\
> +})
> +
> +/*
> + * Associate every caller with its own dept map.
> + */
> +#define dma_fence_wait_any_timeout(fpp, count, intr, t, idx)		\
> +({									\
> +	signed long __ret;						\
> +									\
> +	sdt_might_sleep_start_timeout(NULL, t);				\
> +	__ret = __dma_fence_wait_any_timeout(fpp, count, intr, t, idx);	\

[Severity: Medium]
Would it be safer to use a more unique variable name instead of __ret, and
evaluate t only once before passing it to sdt_might_sleep_start_timeout()
and __dma_fence_wait_any_timeout(), similar to the other macros?

> +	sdt_might_sleep_end();						\
> +	__ret;								\
> +})

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=28