kernel/workqueue.c | 3 +++ 1 file changed, 3 insertions(+)
wq->lockdep_map is set only after __alloc_workqueue()
successfully returns. However, on its error path
__alloc_workqueue() may call destroy_workqueue() which
expects wq->lockdep_map to be already set, which results
in a null-ptr-deref in touch_wq_lockdep_map().
Add a simple NULL-check to touch_wq_lockdep_map().
Oops: general protection fault, probably for non-canonical address
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
RIP: 0010:__lock_acquire+0x81/0x7800
[..]
Call Trace:
<TASK>
? __die_body+0x66/0xb0
? die_addr+0xb2/0xe0
? exc_general_protection+0x300/0x470
? asm_exc_general_protection+0x22/0x30
? __lock_acquire+0x81/0x7800
? mark_lock+0x94/0x330
? __lock_acquire+0x12fd/0x7800
? __lock_acquire+0x3439/0x7800
lock_acquire+0x14c/0x3e0
? __flush_workqueue+0x167/0x13a0
? __init_swait_queue_head+0xaf/0x150
? __flush_workqueue+0x167/0x13a0
__flush_workqueue+0x17d/0x13a0
? __flush_workqueue+0x167/0x13a0
? lock_release+0x50f/0x830
? drain_workqueue+0x94/0x300
drain_workqueue+0xe3/0x300
destroy_workqueue+0xac/0xc40
? workqueue_sysfs_register+0x159/0x2f0
__alloc_workqueue+0x1506/0x1760
alloc_workqueue+0x61/0x150
...
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
kernel/workqueue.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index bfeeefeee332..59bd2c1e55af 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3870,6 +3870,9 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
static void touch_wq_lockdep_map(struct workqueue_struct *wq)
{
#ifdef CONFIG_LOCKDEP
+ if (unlikely(!wq->lockdep_map))
+ return;
+
if (wq->flags & WQ_BH)
local_bh_disable();
--
2.46.0.76.ge559c4bf1a-googOn Thu, Aug 15, 2024 at 04:02:58PM +0900, Sergey Senozhatsky wrote: > wq->lockdep_map is set only after __alloc_workqueue() > successfully returns. However, on its error path > __alloc_workqueue() may call destroy_workqueue() which > expects wq->lockdep_map to be already set, which results > in a null-ptr-deref in touch_wq_lockdep_map(). > > Add a simple NULL-check to touch_wq_lockdep_map(). > > Oops: general protection fault, probably for non-canonical address > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > RIP: 0010:__lock_acquire+0x81/0x7800 > [..] > Call Trace: > <TASK> > ? __die_body+0x66/0xb0 > ? die_addr+0xb2/0xe0 > ? exc_general_protection+0x300/0x470 > ? asm_exc_general_protection+0x22/0x30 > ? __lock_acquire+0x81/0x7800 > ? mark_lock+0x94/0x330 > ? __lock_acquire+0x12fd/0x7800 > ? __lock_acquire+0x3439/0x7800 > lock_acquire+0x14c/0x3e0 > ? __flush_workqueue+0x167/0x13a0 > ? __init_swait_queue_head+0xaf/0x150 > ? __flush_workqueue+0x167/0x13a0 > __flush_workqueue+0x17d/0x13a0 > ? __flush_workqueue+0x167/0x13a0 > ? lock_release+0x50f/0x830 > ? drain_workqueue+0x94/0x300 > drain_workqueue+0xe3/0x300 > destroy_workqueue+0xac/0xc40 > ? workqueue_sysfs_register+0x159/0x2f0 > __alloc_workqueue+0x1506/0x1760 > alloc_workqueue+0x61/0x150 > ... > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> Applied to wq/for-6.12. Thanks. -- tejun
On (24/08/15 16:02), Sergey Senozhatsky wrote:
> wq->lockdep_map is set only after __alloc_workqueue()
> successfully returns. However, on its error path
> __alloc_workqueue() may call destroy_workqueue() which
> expects wq->lockdep_map to be already set, which results
> in a null-ptr-deref in touch_wq_lockdep_map().
>
> Add a simple NULL-check to touch_wq_lockdep_map().
>
> Oops: general protection fault, probably for non-canonical address
> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> RIP: 0010:__lock_acquire+0x81/0x7800
> [..]
> Call Trace:
> <TASK>
> ? __die_body+0x66/0xb0
> ? die_addr+0xb2/0xe0
> ? exc_general_protection+0x300/0x470
> ? asm_exc_general_protection+0x22/0x30
> ? __lock_acquire+0x81/0x7800
> ? mark_lock+0x94/0x330
> ? __lock_acquire+0x12fd/0x7800
> ? __lock_acquire+0x3439/0x7800
> lock_acquire+0x14c/0x3e0
> ? __flush_workqueue+0x167/0x13a0
> ? __init_swait_queue_head+0xaf/0x150
> ? __flush_workqueue+0x167/0x13a0
> __flush_workqueue+0x17d/0x13a0
> ? __flush_workqueue+0x167/0x13a0
> ? lock_release+0x50f/0x830
> ? drain_workqueue+0x94/0x300
> drain_workqueue+0xe3/0x300
> destroy_workqueue+0xac/0xc40
> ? workqueue_sysfs_register+0x159/0x2f0
> __alloc_workqueue+0x1506/0x1760
> alloc_workqueue+0x61/0x150
> ...
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
> kernel/workqueue.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index bfeeefeee332..59bd2c1e55af 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3870,6 +3870,9 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
> static void touch_wq_lockdep_map(struct workqueue_struct *wq)
> {
> #ifdef CONFIG_LOCKDEP
> + if (unlikely(!wq->lockdep_map))
> + return;
> +
> if (wq->flags & WQ_BH)
> local_bh_disable();
Oh, okay, so this is related to [1]
workqueue: Split alloc_workqueue into internal function and lockdep init
Cc-ing Matthew on this.
[1] https://lore.kernel.org/all/20240809222827.3211998-2-matthew.brost@intel.com/
On (24/08/15 16:09), Sergey Senozhatsky wrote:
> On (24/08/15 16:02), Sergey Senozhatsky wrote:
> Oh, okay, so this is related to [1]
>
> workqueue: Split alloc_workqueue into internal function and lockdep init
>
> Cc-ing Matthew on this.
>
> [1] https://lore.kernel.org/all/20240809222827.3211998-2-matthew.brost@intel.com/
Matthew, did you mean to do something like below instead?
Otherwise it breaks boot for me.
---
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7a2f4cbbe8b2..55e0e69ee604 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5615,12 +5615,11 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
} while (activated);
}
-__printf(1, 4)
static struct workqueue_struct *__alloc_workqueue(const char *fmt,
unsigned int flags,
- int max_active, ...)
+ int max_active,
+ va_list args)
{
- va_list args;
struct workqueue_struct *wq;
size_t wq_size;
int name_len;
@@ -5652,9 +5651,7 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
goto err_free_wq;
}
- va_start(args, max_active);
name_len = vsnprintf(wq->name, sizeof(wq->name), fmt, args);
- va_end(args);
if (name_len >= WQ_NAME_LEN)
pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
On Thu, Aug 15, 2024 at 04:24:27PM +0900, Sergey Senozhatsky wrote:
> On (24/08/15 16:09), Sergey Senozhatsky wrote:
> > On (24/08/15 16:02), Sergey Senozhatsky wrote:
> > Oh, okay, so this is related to [1]
> >
> > workqueue: Split alloc_workqueue into internal function and lockdep init
> >
> > Cc-ing Matthew on this.
> >
> > [1] https://lore.kernel.org/all/20240809222827.3211998-2-matthew.brost@intel.com/
>
> Matthew, did you mean to do something like below instead?
> Otherwise it breaks boot for me.
>
Yikes, my bad. My change worked locally and in Intel's CI for Xe [2]. So
I did not realize this was an issue the way I had it coded.
I believe you when you say this is an issue.
With that, the way I coded alloc_ordered_workqueue_lockdep_map could
also be dangerous. Thinking that also should also be changed back to a
macro too.
Let's see what Tejon thinks?
Matt
[2] https://patchwork.freedesktop.org/series/136701/
> ---
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 7a2f4cbbe8b2..55e0e69ee604 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -5615,12 +5615,11 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
> } while (activated);
> }
>
> -__printf(1, 4)
> static struct workqueue_struct *__alloc_workqueue(const char *fmt,
> unsigned int flags,
> - int max_active, ...)
> + int max_active,
> + va_list args)
> {
> - va_list args;
> struct workqueue_struct *wq;
> size_t wq_size;
> int name_len;
> @@ -5652,9 +5651,7 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
> goto err_free_wq;
> }
>
> - va_start(args, max_active);
> name_len = vsnprintf(wq->name, sizeof(wq->name), fmt, args);
> - va_end(args);
>
> if (name_len >= WQ_NAME_LEN)
> pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
On Thu, Aug 15, 2024 at 03:56:39PM +0000, Matthew Brost wrote:
> On Thu, Aug 15, 2024 at 04:24:27PM +0900, Sergey Senozhatsky wrote:
> > On (24/08/15 16:09), Sergey Senozhatsky wrote:
> > > On (24/08/15 16:02), Sergey Senozhatsky wrote:
> > > Oh, okay, so this is related to [1]
> > >
> > > workqueue: Split alloc_workqueue into internal function and lockdep init
> > >
> > > Cc-ing Matthew on this.
> > >
> > > [1] https://lore.kernel.org/all/20240809222827.3211998-2-matthew.brost@intel.com/
> >
> > Matthew, did you mean to do something like below instead?
> > Otherwise it breaks boot for me.
> >
>
> Yikes, my bad. My change worked locally and in Intel's CI for Xe [2]. So
> I did not realize this was an issue the way I had it coded.
>
> I believe you when you say this is an issue.
>
> With that, the way I coded alloc_ordered_workqueue_lockdep_map could
> also be dangerous. Thinking that also should also be changed back to a
> macro too.
>
Sergey,
Sorry double reply. Looked into this and va_start / va_end should only
be called once, so I think the change you have is correct.
The change wrt to alloc_ordered_workqueue_lockdep_map should look like
this I think:
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
Matt
> Let's see what Tejon thinks?
>
> Matt
>
> [2] https://patchwork.freedesktop.org/series/136701/
>
> > ---
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 7a2f4cbbe8b2..55e0e69ee604 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -5615,12 +5615,11 @@ static void wq_adjust_max_active(struct workqueue_struct *wq)
> > } while (activated);
> > }
> >
> > -__printf(1, 4)
> > static struct workqueue_struct *__alloc_workqueue(const char *fmt,
> > unsigned int flags,
> > - int max_active, ...)
> > + int max_active,
> > + va_list args)
> > {
> > - va_list args;
> > struct workqueue_struct *wq;
> > size_t wq_size;
> > int name_len;
> > @@ -5652,9 +5651,7 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
> > goto err_free_wq;
> > }
> >
> > - va_start(args, max_active);
> > name_len = vsnprintf(wq->name, sizeof(wq->name), fmt, args);
> > - va_end(args);
> >
> > if (name_len >= WQ_NAME_LEN)
> > pr_warn_once("workqueue: name exceeds WQ_NAME_LEN. Truncating to: %s\n",
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.
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)?
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
Hello, On Fri, Aug 16, 2024 at 02:45:20AM +0000, Matthew Brost wrote: > > 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. Can you just send a fixup patch? Thanks. -- tejun
On (24/08/19 11:15), Tejun Heo wrote: > Hello, > > On Fri, Aug 16, 2024 at 02:45:20AM +0000, Matthew Brost wrote: > > > 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. > > Can you just send a fixup patch? Well, this will make the tree unbisectable (for a range of versions), because the errors in question break the boot.
On Wed, Aug 21, 2024 at 08:54:12AM +0900, Sergey Senozhatsky wrote: ... > Well, this will make the tree unbisectable (for a range of versions), > because the errors in question break the boot. The span is three commits with one htmldoc fix commit inbetween. If anyone is unlucky enough to hit that, `git bisect skip` is going to do just fine. Thanks. -- tejun
On (24/08/20 14:04), Tejun Heo wrote: > On Wed, Aug 21, 2024 at 08:54:12AM +0900, Sergey Senozhatsky wrote: > ... > > Well, this will make the tree unbisectable (for a range of versions), > > because the errors in question break the boot. > > The span is three commits with one htmldoc fix commit inbetween. If anyone > is unlucky enough to hit that, `git bisect skip` is going to do just fine. Well, OK. Can you also pick-up the null-ptr-deref fixup?
On Wed, Aug 21, 2024 at 08:54:12AM +0900, Sergey Senozhatsky wrote: > On (24/08/19 11:15), Tejun Heo wrote: > > Hello, > > > > On Fri, Aug 16, 2024 at 02:45:20AM +0000, Matthew Brost wrote: > > > > 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. > > > > Can you just send a fixup patch? > > Well, this will make the tree unbisectable (for a range of versions), > because the errors in question break the boot. Can the patches not just get squashed together in the 6.12 PR? Matt
On (24/08/20 23:56), Matthew Brost wrote: > On Wed, Aug 21, 2024 at 08:54:12AM +0900, Sergey Senozhatsky wrote: > > On (24/08/19 11:15), Tejun Heo wrote: > > > Hello, > > > > > > On Fri, Aug 16, 2024 at 02:45:20AM +0000, Matthew Brost wrote: > > > > > 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. > > > > > > Can you just send a fixup patch? > > > > Well, this will make the tree unbisectable (for a range of versions), > > because the errors in question break the boot. > > Can the patches not just get squashed together in the 6.12 PR? squash would be my personal preference. Usually resend works in cases like this (so that Link: in commit messages point to the relevant thread), but I don't know what Tejun's planning to do.
On Mon, Aug 19, 2024 at 11:15:38AM -1000, Tejun Heo wrote: > Hello, > > On Fri, Aug 16, 2024 at 02:45:20AM +0000, Matthew Brost wrote: > > > 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. > > Can you just send a fixup patch? > Yes, will sent one out shortly. Matt > Thanks. > > -- > tejun
On (24/08/16 02:45), Matthew Brost wrote: [..] > > 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. Oh, yes, correct, that's what I meant too - we need both. // Plus a null-ptr-deref fix in __alloc_workqueue() error path.
© 2016 - 2026 Red Hat, Inc.