2007-05-29 20:55:23

by Alan Stern

[permalink] [raw]
Subject: Sending signals to a kernel thread, broken in 2.6.22

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


2007-05-30 11:22:33

by Oleg Nesterov

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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.

2007-05-30 12:33:17

by Oleg Nesterov

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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

2007-05-30 12:39:29

by Nigel Cunningham

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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


Attachments:
signature.asc (189.00 B)
This is a digitally signed message part

2007-05-30 14:51:31

by Alan Stern

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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

2007-05-30 14:57:53

by Alan Stern

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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

2007-05-30 20:04:07

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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

2007-05-30 22:22:28

by Nigel Cunningham

[permalink] [raw]
Subject: Re: Sending signals to a kernel thread, broken in 2.6.22

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


Attachments:
signature.asc (189.00 B)
This is a digitally signed message part