[PATCH] KEYS: dispatch key_gc_work to system_wq.

[PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

schedule_work() dispatches key_gc_work on the per-CPU system_wq, which can
choke the queue:

BUG: workqueue lockup - pool cpus=0 node=0 flags=0x2 nice=0 stuck for 30s!

Queue the work on system_dfl_long_wq instead, which is an unbounded wq for
long-running tasks i.e., it has the WQ_BOUND flag set and thus cannot hog
the CPU core.

Link: https://lore.kernel.org/keyrings/20261024042944.52493-1-hackerowskiy@gmail.com/
Reported-by: syzbot+c15de43f1e7fffbb7b9b@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6ad35b60.7952420f.283480.0360.GAE@google.com/T/
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
I sanity checked with perf that scheduler latency improved clearly as it
should, and used large keyrings that I expired, as the test payload.

It was ad-hoc testing but I can scrape from transcripts a test script
if wanted.
 security/keys/gc.c  | 8 ++++----
 security/keys/key.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/keys/gc.c b/security/keys/gc.c
index eda445f815d4..2337d39722e4 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -57,7 +57,7 @@ void key_schedule_gc(time64_t gc_at)
 
 	if (gc_at <= now || test_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags)) {
 		kdebug("IMMEDIATE");
-		schedule_work(&key_gc_work);
+		queue_work(system_dfl_long_wq, &key_gc_work);
 	} else if (gc_at < key_gc_next_run) {
 		kdebug("DEFERRED");
 		key_gc_next_run = gc_at;
@@ -85,7 +85,7 @@ void key_set_expiry(struct key *key, time64_t expiry)
 void key_schedule_gc_links(void)
 {
 	set_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags);
-	schedule_work(&key_gc_work);
+	queue_work(system_dfl_long_wq, &key_gc_work);
 }
 
 /*
@@ -119,7 +119,7 @@ void key_gc_keytype(struct key_type *ktype)
 	set_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags);
 
 	kdebug("schedule");
-	schedule_work(&key_gc_work);
+	queue_work(system_dfl_long_wq, &key_gc_work);
 
 	kdebug("sleep");
 	wait_on_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE,
@@ -322,7 +322,7 @@ static void key_garbage_collector(struct work_struct *work)
 	}
 
 	if (gc_state & KEY_GC_REAP_AGAIN)
-		schedule_work(&key_gc_work);
+		queue_work(system_dfl_long_wq, &key_gc_work);
 	kleave(" [end %x]", gc_state);
 	return;
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 6ebb8691551f..599626c897b2 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -668,7 +668,7 @@ void key_put(struct key *key)
 			}
 			/* Mark key as safe for GC after key->user done. */
 			clear_bit_unlock(KEY_FLAG_USER_ALIVE, &key->flags);
-			schedule_work(&key_gc_work);
+			queue_work(system_dfl_long_wq, &key_gc_work);
 		}
 	}
 }
-- 
2.47.3

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

On Mon, Oct 26, 2026 at 05:12:18AM +0200, Jarkko Sakkinen wrote:
> schedule_work() dispatches key_gc_work on the per-CPU system_wq, which can
> choke the queue:
> 
> BUG: workqueue lockup - pool cpus=0 node=0 flags=0x2 nice=0 stuck for 30s!
> 
> Queue the work on system_dfl_long_wq instead, which is an unbounded wq for
> long-running tasks i.e., it has the WQ_BOUND flag set and thus cannot hog
> the CPU core.
> 
> Link: https://lore.kernel.org/keyrings/20261024042944.52493-1-hackerowskiy@gmail.com/
> Reported-by: syzbot+c15de43f1e7fffbb7b9b@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/6ad35b60.7952420f.283480.0360.GAE@google.com/T/
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Because old code has weird assumptions regardless what code it is, if it
has been for a while using one threading model, so I'm mainly concerned
are there "pinned to a CPU" assumptions in the keyring GC?

Br, Jarkko

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

short summary: /system_wq/system_dfl_long_wq/, /\.//

On Mon, Oct 26, 2026 at 05:12:18AM +0200, Jarkko Sakkinen wrote:
> schedule_work() dispatches key_gc_work on the per-CPU system_wq, which can
> choke the queue:
> 
> BUG: workqueue lockup - pool cpus=0 node=0 flags=0x2 nice=0 stuck for 30s!
> 
> Queue the work on system_dfl_long_wq instead, which is an unbounded wq for
> long-running tasks i.e., it has the WQ_BOUND flag set and thus cannot hog
> the CPU core.
> 
> Link: https://lore.kernel.org/keyrings/20261024042944.52493-1-hackerowskiy@gmail.com/
> Reported-by: syzbot+c15de43f1e7fffbb7b9b@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/6ad35b60.7952420f.283480.0360.GAE@google.com/T/
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> I sanity checked with perf that scheduler latency improved clearly as it
> should, and used large keyrings that I expired, as the test payload.
> 
> It was ad-hoc testing but I can scrape from transcripts a test script
> if wanted.
>  security/keys/gc.c  | 8 ++++----
>  security/keys/key.c | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index eda445f815d4..2337d39722e4 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -57,7 +57,7 @@ void key_schedule_gc(time64_t gc_at)
>  
>  	if (gc_at <= now || test_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags)) {
>  		kdebug("IMMEDIATE");
> -		schedule_work(&key_gc_work);
> +		queue_work(system_dfl_long_wq, &key_gc_work);
>  	} else if (gc_at < key_gc_next_run) {
>  		kdebug("DEFERRED");
>  		key_gc_next_run = gc_at;
> @@ -85,7 +85,7 @@ void key_set_expiry(struct key *key, time64_t expiry)
>  void key_schedule_gc_links(void)
>  {
>  	set_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags);
> -	schedule_work(&key_gc_work);
> +	queue_work(system_dfl_long_wq, &key_gc_work);
>  }
>  
>  /*
> @@ -119,7 +119,7 @@ void key_gc_keytype(struct key_type *ktype)
>  	set_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags);
>  
>  	kdebug("schedule");
> -	schedule_work(&key_gc_work);
> +	queue_work(system_dfl_long_wq, &key_gc_work);
>  
>  	kdebug("sleep");
>  	wait_on_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE,
> @@ -322,7 +322,7 @@ static void key_garbage_collector(struct work_struct *work)
>  	}
>  
>  	if (gc_state & KEY_GC_REAP_AGAIN)
> -		schedule_work(&key_gc_work);
> +		queue_work(system_dfl_long_wq, &key_gc_work);
>  	kleave(" [end %x]", gc_state);
>  	return;
>  
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 6ebb8691551f..599626c897b2 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -668,7 +668,7 @@ void key_put(struct key *key)
>  			}
>  			/* Mark key as safe for GC after key->user done. */
>  			clear_bit_unlock(KEY_FLAG_USER_ALIVE, &key->flags);
> -			schedule_work(&key_gc_work);
> +			queue_work(system_dfl_long_wq, &key_gc_work);
>  		}
>  	}
>  }
> -- 
> 2.47.3
> 

Br, Jarkko

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Hillf Danton]

On Mon, 26 Oct 2026 05:12:18 +0200 Jarkko Sakkinen wrote:
> schedule_work() dispatches key_gc_work on the per-CPU system_wq, which can
> choke the queue:
> 
> BUG: workqueue lockup - pool cpus=0 node=0 flags=0x2 nice=0 stuck for 30s!
> 
> Queue the work on system_dfl_long_wq instead, which is an unbounded wq for
> long-running tasks i.e., it has the WQ_BOUND flag set and thus cannot hog
> the CPU core.
>
Nope there is no cpu hog at all given cond_resched().
Take a break instead then the workqueue has a chance for housekeeping.

--- x/security/keys/gc.c
+++ y/security/keys/gc.c
@@ -270,9 +270,8 @@ contended:
 
 maybe_resched:
 	if (cursor) {
-		cond_resched();
-		spin_lock(&key_serial_lock);
-		goto continue_scanning;
+		schedule_work(&key_gc_work);
+		return;
 	}
 
 	/* We've completed the pass.  Set the timer if we need to and queue a
--

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

On Mon, Oct 26, 2026 at 05:56:05PM +0800, Hillf Danton wrote:
> On Mon, 26 Oct 2026 05:12:18 +0200 Jarkko Sakkinen wrote:
> > schedule_work() dispatches key_gc_work on the per-CPU system_wq, which can
> > choke the queue:
> > 
> > BUG: workqueue lockup - pool cpus=0 node=0 flags=0x2 nice=0 stuck for 30s!
> > 
> > Queue the work on system_dfl_long_wq instead, which is an unbounded wq for
> > long-running tasks i.e., it has the WQ_BOUND flag set and thus cannot hog
> > the CPU core.
> >
> Nope there is no cpu hog at all given cond_resched().

I used wrong term, and it is good that you nitpicked that.

What about workqueue lockup, which I pointed out from transcript?

This does not help.

> Take a break instead then the workqueue has a chance for housekeeping.
> 
> --- x/security/keys/gc.c
> +++ y/security/keys/gc.c
> @@ -270,9 +270,8 @@ contended:
>  
>  maybe_resched:
>  	if (cursor) {
> -		cond_resched();
> -		spin_lock(&key_serial_lock);
> -		goto continue_scanning;
> +		schedule_work(&key_gc_work);
> +		return;
>  	}
>  
>  	/* We've completed the pass.  Set the timer if we need to and queue a
> --

Br, Jarkko

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

On Mon, Oct 26, 2026 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
> > --- x/security/keys/gc.c
> > +++ y/security/keys/gc.c
> > @@ -270,9 +270,8 @@ contended:
> >  
> >  maybe_resched:
> >  	if (cursor) {
> > -		cond_resched();
> > -		spin_lock(&key_serial_lock);
> > -		goto continue_scanning;
> > +		schedule_work(&key_gc_work);
> > +		return;
> >  	}

I guess I'll to elaborate my fix and this proposal as you gave feedback
of neither.

What you wanted to respond to my patch other than nitpicking is probably
this sequence:

  key_garbage_collector()
    synchronize_rcu()
      synchronize_rcu_normal()
        wait_for_completion()
          schedule_timeout()
            schedule()

I should have put this to the commit message, and missed also this
detail.

So the way I saw this, which could be wrong, is that the workqueue
lockup would could happen less often as also other threads are scheduled
preemptively in unbounded queue outside critical sections.

What you are doing with early return is at least not right as it 
corrupts the state. So you should supplement that with something
that carries it.

Br, Jarkko

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

On Mon, Oct 26, 2026 at 07:19:01PM +0200, Jarkko Sakkinen wrote:
> On Mon, Oct 26, 2026 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
> > > --- x/security/keys/gc.c
> > > +++ y/security/keys/gc.c
> > > @@ -270,9 +270,8 @@ contended:
> > >  
> > >  maybe_resched:
> > >  	if (cursor) {
> > > -		cond_resched();
> > > -		spin_lock(&key_serial_lock);
> > > -		goto continue_scanning;
> > > +		schedule_work(&key_gc_work);
> > > +		return;
> > >  	}
> 
> I guess I'll to elaborate my fix and this proposal as you gave feedback
> of neither.
> 
> What you wanted to respond to my patch other than nitpicking is probably
> this sequence:
> 
>   key_garbage_collector()
>     synchronize_rcu()
>       synchronize_rcu_normal()
>         wait_for_completion()
>           schedule_timeout()
>             schedule()
> 
> I should have put this to the commit message, and missed also this
> detail.
> 
> So the way I saw this, which could be wrong, is that the workqueue
> lockup would could happen less often as also other threads are scheduled
> preemptively in unbounded queue outside critical sections.

I did test this with fairly large keyrings. I think I'll scrape that
script. That does not mean by any means that the fix is correct at all
but. Please in future also provide context for your alternative
proposals because otherwise they pay no contribution to anything.

I'm not really interested whether I'm right or wrong here but
instead of finding the "truth" aka the correcting reasoning
that leads to a proper fix.

A random diff won't do it.

> 
> What you are doing with early return is at least not right as it 
> corrupts the state. So you should supplement that with something
> that carries it.


Br, Jarkko

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

On Mon, Oct 26, 2026 at 05:12:18AM +0200, Jarkko Sakkinen wrote:
> schedule_work() dispatches key_gc_work on the per-CPU system_wq, which can
> choke the queue:
> 
> BUG: workqueue lockup - pool cpus=0 node=0 flags=0x2 nice=0 stuck for 30s!
> 
> Queue the work on system_dfl_long_wq instead, which is an unbounded wq for
> long-running tasks i.e., it has the WQ_BOUND flag set and thus cannot hog
> the CPU core.
> 
> Link: https://lore.kernel.org/keyrings/20261024042944.52493-1-hackerowskiy@gmail.com/
> Reported-by: syzbot+c15de43f1e7fffbb7b9b@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/6ad35b60.7952420f.283480.0360.GAE@google.com/T/
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> I sanity checked with perf that scheduler latency improved clearly as it
> should, and used large keyrings that I expired, as the test payload.

Based on the feedback, rather than arguing about irrelevant stuff, I'll
focus on formalizing AB test for this.

I have couple of additional ideas to try out but without AB test fixing
this is really just shooting in the wind.

Br, Jarkko

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Hillf Danton]

On Mon, 26 Oct 2026 19:18:57 +0200 Jarkko Sakkinen wrote:
>On Mon, Oct 26, 2026 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
>> > --- x/security/keys/gc.c
>> > +++ y/security/keys/gc.c
>> > @@ -270,9 +270,8 @@ contended:
>> >  
>> >  maybe_resched:
>> >  	if (cursor) {
>> > -		cond_resched();
>> > -		spin_lock(&key_serial_lock);
>> > -		goto continue_scanning;
>> > +		schedule_work(&key_gc_work);
>> > +		return;
>> >  	}
>
>I guess I'll to elaborate my fix and this proposal as you gave feedback
>of neither.
>
>What you wanted to respond to my patch other than nitpicking is probably
>this sequence:
>
Nope because synchronize_rcu() has nothing to do with workqueue lockup.

>  key_garbage_collector()
>    synchronize_rcu()
>      synchronize_rcu_normal()
>        wait_for_completion()
>          schedule_timeout()
>            schedule()
>
>I should have put this to the commit message, and missed also this
>detail.
>
>So the way I saw this, which could be wrong, is that the workqueue
>lockup would could happen less often as also other threads are scheduled
>preemptively in unbounded queue outside critical sections.
>
The unbound queue could also suffer lockup.

>What you are doing with early return is at least not right as it 
>corrupts the state. So you should supplement that with something

How?

>that carries it.
>
Feel free to post a diff?

Re: [PATCH] KEYS: dispatch key_gc_work to system_wq.

[Jarkko Sakkinen]

On Tue, Oct 27, 2026 at 06:21:52AM +0800, Hillf Danton wrote:
> On Mon, 26 Oct 2026 19:18:57 +0200 Jarkko Sakkinen wrote:
> >On Mon, Oct 26, 2026 at 06:57:22PM +0200, Jarkko Sakkinen wrote:
> >> > --- x/security/keys/gc.c
> >> > +++ y/security/keys/gc.c
> >> > @@ -270,9 +270,8 @@ contended:
> >> >  
> >> >  maybe_resched:
> >> >  	if (cursor) {
> >> > -		cond_resched();
> >> > -		spin_lock(&key_serial_lock);
> >> > -		goto continue_scanning;
> >> > +		schedule_work(&key_gc_work);
> >> > +		return;
> >> >  	}
> >
> >I guess I'll to elaborate my fix and this proposal as you gave feedback
> >of neither.
> >
> >What you wanted to respond to my patch other than nitpicking is probably
> >this sequence:
> >
> Nope because synchronize_rcu() has nothing to do with workqueue lockup.
> 
> >  key_garbage_collector()
> >    synchronize_rcu()
> >      synchronize_rcu_normal()
> >        wait_for_completion()
> >          schedule_timeout()
> >            schedule()
> >
> >I should have put this to the commit message, and missed also this
> >detail.
> >
> >So the way I saw this, which could be wrong, is that the workqueue
> >lockup would could happen less often as also other threads are scheduled
> >preemptively in unbounded queue outside critical sections.
> >
> The unbound queue could also suffer lockup.
> 
> >What you are doing with early return is at least not right as it 
> >corrupts the state. So you should supplement that with something
> 
> How?
>
> >that carries it.
> >
> Feel free to post a diff?

Actually I choose not to answer to your questions as your only
argument is "show the diff" or providing a diff without making
your case. I'm sorry but I don't get anything from this.

If you want to help, be helpful.

Br, Jarkko