Re: [PATCH] workqueue: fix null-ptr-deref on __alloc_workqueue() error

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Aug 16, 2024 at 11:38:31AM +0900, Sergey Senozhatsky wrote:
> Hi Matthew,
> 
> On (24/08/15 16:24), Matthew Brost wrote:
> [..]
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index 8ccbf510880b..5e818eae092d 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -534,7 +534,7 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> >   * @fmt: printf format for the name of the workqueue
> >   * @flags: WQ_* flags (only WQ_FREEZABLE and WQ_MEM_RECLAIM are meaningful)
> >   * @lockdep_map: user-defined lockdep_map
> > - * @args: args for @fmt
> > + * @...: args for @fmt
> >   *
> >   * Same as alloc_ordered_workqueue but with the a user-define lockdep_map.
> >   * Useful for workqueues created with the same purpose and to avoid leaking a
> > @@ -543,20 +543,9 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
> >   * RETURNS:
> >   * Pointer to the allocated workqueue on success, %NULL on failure.
> >   */
> > -__printf(1, 4) static inline struct workqueue_struct *
> > -alloc_ordered_workqueue_lockdep_map(const char *fmt, unsigned int flags,
> > -                                   struct lockdep_map *lockdep_map, ...)
> > -{
> > -       struct workqueue_struct *wq;
> > -       va_list args;
> > -
> > -       va_start(args, lockdep_map);
> > -       wq = alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | flags,
> > -                                        1, lockdep_map, args);
> > -       va_end(args);
> > +#define alloc_ordered_workqueue_lockdep_map(fmt, flags, lockdep_map, args...)  \
> > +       alloc_workqueue_lockdep_map(fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, lockdep_map, ##args)
> > 
> > -       return wq;
> > -}
> >  #endif
> 
> Oh, I haven't checked the workqueue header.  Yes, you are right.
> A macro should work.
> 

To be clear we your change to __alloc_workqueue in workqueue.c AND my
change to a macro in workqueue.h. In both cases a call chain of
multiple va_start / va_end happens which from what I read up on is not
allowed or undefined behavior depending on the compiler / platform
target.

> 
> Tejun, how do you plan to handle this?  Would it be possible to
> drop current series from your tree so that Matthew can send an
> updated version (with all the fixes squashed)?

Tejun, yes let me know how to move forward with this as it is highly
desired for Intel Xe team to get this into 6.12.

Matt