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

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