8139too termination

8139too termination

[Robert Kuebel]

hi,

i have been getting this message at shutdown ...

"eth1: unable to signal thread"

it turns out that 8139too's kernel thread gets killed at shutdown (or
reboot) when SIGTERM is sent to all processes.  then the network
shutdown script comes along and takes down the interface.  the driver
complains ...

"eth1: unable to signal thread"

because the thread has already terminated.  the driver currently does
not block any signals.

my question is, should 8139too really not block any signals (and allow
itself to be killed by them)?  isn't it a bad thing to allow a kernel
thread to be killed accidentally like this?

give me some feedback.  i would be happy to write a fix.

also, please cc me on replies.  i can only handle the digest form of
lkml.

thanks.
rob.

Re: 8139too termination

[Andrew Morton]

Robert Kuebel wrote:
> 
> hi,
> 
> i have been getting this message at shutdown ...
> 
> "eth1: unable to signal thread"
> 
> it turns out that 8139too's kernel thread gets killed at shutdown (or
> reboot) when SIGTERM is sent to all processes.  then the network
> shutdown script comes along and takes down the interface.  the driver
> complains ...
> 
> "eth1: unable to signal thread"
> 
> because the thread has already terminated.  the driver currently does
> not block any signals.
> 
> my question is, should 8139too really not block any signals (and allow
> itself to be killed by them)?  isn't it a bad thing to allow a kernel
> thread to be killed accidentally like this?
> 

Yes, I'd agree that the driver should ignore random signals.
The kernel thread should only allow itself to be terminated
via the driver's close() method.

An obvious approach is to change rtl8139_close() to do:

	tp->diediedie = 1;
	wmb();
	ret = kill_proc(...);

and test the flag in rtl8139_thread().

The tricky part is teaching the thread to ignore the
spurious signals - the signal_pending() state needs to be
cleared.  I think flush_signals() is the way to do this.
See context_thread() for an example.

           spin_lock_irq(&curtask->sigmask_lock);
           flush_signals(curtask);
           recalc_sigpending(curtask);
           spin_unlock_irq(&curtask->sigmask_lock);

The recalc_sigpending() here appears to be unnecessary...

The kernel thread in 8139too has certainly been an interesting
learning exercise :)  Using signals and task management in-kernel
is full of pitfalls.  In retrospect, probably it should have used
waitqueues directly.

-

Re: 8139too termination

[Robert Kuebel]

> 
> 	tp->diediedie = 1;
> 	wmb();
> 	ret = kill_proc(...);
> 
> and test the flag in rtl8139_thread().
> 

i had something like that in mind.

> The tricky part is teaching the thread to ignore the
> spurious signals - the signal_pending() state needs to be
> cleared.  I think flush_signals() is the way to do this.
> See context_thread() for an example.
> 
>            spin_lock_irq(&curtask->sigmask_lock);
>            flush_signals(curtask);
>            recalc_sigpending(curtask);
>            spin_unlock_irq(&curtask->sigmask_lock);
> 
> The recalc_sigpending() here appears to be unnecessary...
> 

what about changing doing
	spin_lock_irq(&current->sigmask_lock);
	sigfillset(&current->blocked);	/* block all sig's */
	recalc_sigpending(current);
	spin_unlock_irq(&current->sigmask_lock);

instead of 

	spin_lock_irq(&current->sigmask_lock);
	sigemptyset(&current->blocked);  
	recalc_sigpending(current);
	spin_unlock_irq(&current->sigmask_lock);

and replacing the signal_pending() stuff in the loops of
rtl8139_thread() with checks for tp->diediedie?

Re: 8139too termination

[Robert Kuebel]

On Mon, Oct 29, 2001 at 04:30:40PM -0800, Andrew Morton wrote:
> Robert Kuebel wrote:
> > 
> > what about changing doing
> >         spin_lock_irq(&current->sigmask_lock);
> >         sigfillset(&current->blocked);  /* block all sig's */
> >         recalc_sigpending(current);
> >         spin_unlock_irq(&current->sigmask_lock);
> > 
> > instead of
> > 
> >         spin_lock_irq(&current->sigmask_lock);
> >         sigemptyset(&current->blocked);
> >         recalc_sigpending(current);
> >         spin_unlock_irq(&current->sigmask_lock);
> > 
> > and replacing the signal_pending() stuff in the loops of
> > rtl8139_thread() with checks for tp->diediedie?
> 
> If you block all the signals then the kill_proc() won't
> bring the thread out of interruptible sleep?

right, you would have to take out the kill_proc().
can't you just let the thread return and not use kill_proc()?  i have
been checking out the reiserfsd thread.

i could be missing something.