On Tue, Jul 07, 2026 at 07:33:44AM +0000, [email protected] wrote:
> 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.
I will fix it.
> > + \
> > + 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.
Sure, I will fix it.
> > + 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?
Okay, I will fix it.
> > + 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
Suggestion?
> evaluate t only once before passing it to sdt_might_sleep_start_timeout()
> and __dma_fence_wait_any_timeout(), similar to the other macros?
Sure.
Byungchul
> > + sdt_might_sleep_end(); \
> > + __ret; \
> > +})
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=28
