Sending signals to a kernel thread, broken in 2.6.22

Sending signals to a kernel thread, broken in 2.6.22

[Alan Stern]

The g_file_storage driver uses a kernel thread and communicates with
that thread in part by means of signals.  It also relies on the thread 
receiving signals from userspace as an indication that the thread 
should terminate.

This was all working in 2.6.21, but as of 2.6.22-rc3 the signal 
delivery mechanism (entirely within the kernel!) is no longer 
functional.

What's the story?  Do I need to do something new and different to get 
signals working again?  Should I avoid using signals entirely?

Thanks for any help,

Alan Stern

Re: Sending signals to a kernel thread, broken in 2.6.22

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

Hi.

On Tue, 2007-05-29 at 16:55 -0400, Alan Stern wrote:
> The g_file_storage driver uses a kernel thread and communicates with
> that thread in part by means of signals.  It also relies on the thread 
> receiving signals from userspace as an indication that the thread 
> should terminate.
> 
> This was all working in 2.6.21, but as of 2.6.22-rc3 the signal 
> delivery mechanism (entirely within the kernel!) is no longer 
> functional.
> 
> What's the story?  Do I need to do something new and different to get 
> signals working again?  Should I avoid using signals entirely?
> 
> Thanks for any help,
> 
> Alan Stern

Hmm. Given Oleg's reply, it could pay to check that freezable kernel
threads are still being frozen.

I'm about to go off to bed, and am away for half the day tomorrow, but
will check as soon as I can if noone else gets to it first. (Rafael
added for a heads-up, just in case he hasn't noticed this thread).

Regards,

Nigel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Sending signals to a kernel thread, broken in 2.6.22

[Alan Stern]

On Wed, 30 May 2007, Nigel Cunningham wrote:

> Hmm. Given Oleg's reply, it could pay to check that freezable kernel
> threads are still being frozen.

I don't think we have to worry about this.  See this discussion thread:

	http://marc.info/?t=118045713200006&r=1&w=2

and in particular this message:

	http://marc.info/?l=linux-kernel&m=118046964817995&w=2

Alan Stern

Re: Sending signals to a kernel thread, broken in 2.6.22

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Hi.

On Wed, 2007-05-30 at 10:51 -0400, Alan Stern wrote:
> On Wed, 30 May 2007, Nigel Cunningham wrote:
> 
> > Hmm. Given Oleg's reply, it could pay to check that freezable kernel
> > threads are still being frozen.
> 
> I don't think we have to worry about this.  See this discussion thread:
> 
> 	http://marc.info/?t=118045713200006&r=1&w=2
> 
> and in particular this message:
> 
> 	http://marc.info/?l=linux-kernel&m=118046964817995&w=2

Ok, cool. Thanks both of you for the quick reply.

Nigel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Sending signals to a kernel thread, broken in 2.6.22

[Rafael J. Wysocki]

On Wednesday, 30 May 2007 14:38, Nigel Cunningham wrote:
> Hi.
> 
> On Tue, 2007-05-29 at 16:55 -0400, Alan Stern wrote:
> > The g_file_storage driver uses a kernel thread and communicates with
> > that thread in part by means of signals.  It also relies on the thread 
> > receiving signals from userspace as an indication that the thread 
> > should terminate.
> > 
> > This was all working in 2.6.21, but as of 2.6.22-rc3 the signal 
> > delivery mechanism (entirely within the kernel!) is no longer 
> > functional.
> > 
> > What's the story?  Do I need to do something new and different to get 
> > signals working again?  Should I avoid using signals entirely?
> > 
> > Thanks for any help,
> > 
> > Alan Stern
> 
> Hmm. Given Oleg's reply, it could pay to check that freezable kernel
> threads are still being frozen.

I think they are.  Otherwise the freezer would have been failing.

> I'm about to go off to bed, and am away for half the day tomorrow, but
> will check as soon as I can if noone else gets to it first. (Rafael
> added for a heads-up, just in case he hasn't noticed this thread).

I have noticed it, but thanks anyway. :-)

Greetings,
Rafael

Re: Sending signals to a kernel thread, broken in 2.6.22

[Oleg Nesterov]

Alan Stern wrote:
>
> The g_file_storage driver uses a kernel thread and communicates with
> that thread in part by means of signals.  It also relies on the thread
> receiving signals from userspace as an indication that the thread
> should terminate.
>
> This was all working in 2.6.21, but as of 2.6.22-rc3 the signal
> delivery mechanism (entirely within the kernel!) is no longer
> functional.
>
> What's the story?  Do I need to do something new and different to get
> signals working again?  Should I avoid using signals entirely?

I guess you mean drivers/usb/gadget/file_storage.c

	fsg_main_thread:
		
		siginitsetinv(&fsg->thread_signal_mask, SIGTERM | ...);
		sigprocmask(SIG_SETMASK, &fsg->thread_signal_mask, NULL);

Yes?

Please look at

	change kernel threads to ignore signals instead of blocking them
	commit: 10ab825bdef8df510f99c703a5a2d9b13a4e31a5

I think you can convert the code above to use allow_signal().


Please note that it is not good to just unblock the signal, SIG_DFL means
that __group_complete_signal() starts doing SIGNAL_GROUP_EXIT things. In
particular, SIGTERM implies sigaddset(SIGKILL).

Oleg.

Re: Sending signals to a kernel thread, broken in 2.6.22

[Oleg Nesterov]

On 05/30, Oleg Nesterov wrote:
>
> Alan Stern wrote:
> >
> > The g_file_storage driver uses a kernel thread and communicates with
> > that thread in part by means of signals.  It also relies on the thread
> > receiving signals from userspace as an indication that the thread
> > should terminate.
> >
> > This was all working in 2.6.21, but as of 2.6.22-rc3 the signal
> > delivery mechanism (entirely within the kernel!) is no longer
> > functional.
> 
> I guess you mean drivers/usb/gadget/file_storage.c
> 
> 	fsg_main_thread:
> 		
> 		siginitsetinv(&fsg->thread_signal_mask, SIGTERM | ...);
> 		sigprocmask(SIG_SETMASK, &fsg->thread_signal_mask, NULL);
> 
> Yes?
> 
> Please look at
> 
> 	change kernel threads to ignore signals instead of blocking them
> 	commit: 10ab825bdef8df510f99c703a5a2d9b13a4e31a5
> 
> I think you can convert the code above to use allow_signal().

something like untested/uncompiled patch below, what do you think?

Oleg.

--- u/drivers/usb/gadget/file_storage.c~	2007-05-30 16:24:19.000000000 +0400
+++ u/drivers/usb/gadget/file_storage.c	2007-05-30 16:29:21.000000000 +0400
@@ -686,7 +686,6 @@ struct fsg_dev {
 	int			thread_wakeup_needed;
 	struct completion	thread_notifier;
 	struct task_struct	*thread_task;
-	sigset_t		thread_signal_mask;
 
 	int			cmnd_size;
 	u8			cmnd[MAX_COMMAND_SIZE];
@@ -3277,8 +3276,7 @@ static void handle_exception(struct fsg_
 	/* Clear the existing signals.  Anything but SIGUSR1 is converted
 	 * into a high-priority EXIT exception. */
 	for (;;) {
-		sig = dequeue_signal_lock(current, &fsg->thread_signal_mask,
-				&info);
+		sig = dequeue_signal_lock(current, &current->blocked, &info);
 		if (!sig)
 			break;
 		if (sig != SIGUSR1) {
@@ -3431,10 +3429,10 @@ static int fsg_main_thread(void *fsg_)
 
 	/* Allow the thread to be killed by a signal, but set the signal mask
 	 * to block everything but INT, TERM, KILL, and USR1. */
-	siginitsetinv(&fsg->thread_signal_mask, sigmask(SIGINT) |
-			sigmask(SIGTERM) | sigmask(SIGKILL) |
-			sigmask(SIGUSR1));
-	sigprocmask(SIG_SETMASK, &fsg->thread_signal_mask, NULL);
+	allow_signal(SIGINT);
+	allow_signal(SIGTERM);
+	allow_signal(SIGKILL);
+	allow_signal(SIGUSR1);
 
 	/* Arrange for userspace references to be interpreted as kernel
 	 * pointers.  That way we can pass a kernel pointer to a routine

Re: Sending signals to a kernel thread, broken in 2.6.22

[Alan Stern]

On Wed, 30 May 2007, Oleg Nesterov wrote:

> > Please look at
> > 
> > 	change kernel threads to ignore signals instead of blocking them
> > 	commit: 10ab825bdef8df510f99c703a5a2d9b13a4e31a5
> > 
> > I think you can convert the code above to use allow_signal().
> 
> something like untested/uncompiled patch below, what do you think?

I tried it out, and it worked perfectly.  Thank you very much.

Alan Stern