Re: [PATCH] signals: Generate warning when flush_signals() is called from non-kthread context

[Evgeniy Polyakov]

Hi Ingo

01.05.2015, 22:38, "Ingo Molnar" <mingo@xxxxxxxxxx>:
> * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>  On Fri, May 1, 2015 at 10:40 AM, Alex Williamson
>>  <alex.williamson@xxxxxxxxxx> wrote:
>>>  - Flush signals on interrupted wait to retain polling interval (Alex Williamson)
>>  This cannot *possibly* be right. If I read this patch right, you're
>>  randomly just getting rid of signals. No way in hell is that correct.
>>
>>  "flush_signals()" is only for kernel threads, where it's a hacky
>>  alternative to actually handling them (since kernel threads never
>>  rreturn to user space and cannot really "handle" a signal). But you're
>>  doing it in the ->remove handler for the device, which can be called
>>  by arbitrary system processes. This is not a kernel thread thing, as
>>  far as I can see.
>>
>>  If you cannot handle signals, you damn well shouldn't be using
>>  "wait_event_interruptible_timeout()" to begin with. Get rid of the
>>  "interruptible", since it apparently *isn't* interruptible.
>>
>>  So I'm not pulling this.
>>
>>  Now I'm worried that other drivers do insane things like this. I
>>  wonder if we should add some sanity test to flush_signals() to make
>>  sure that it can only ever get called from a kernel thread.

> Looks unsafe: called from various module exit handlers in:
>
>   drivers/w1/slaves/w1_bq27000.c
>   drivers/w1/slaves/w1_ds2406.c
>   drivers/w1/slaves/w1_ds2408.c
>   drivers/w1/slaves/w1_ds2413.c
>   drivers/w1/slaves/w1_ds2423.c
>   drivers/w1/slaves/w1_ds2431.c
>   drivers/w1/slaves/w1_ds2433.c
>   drivers/w1/slaves/w1_ds2760.c
>   drivers/w1/slaves/w1_ds2780.c
>   drivers/w1/slaves/w1_ds2781.c
>   drivers/w1/slaves/w1_ds28e04.c
>   drivers/w1/slaves/w1_smem.c
>   drivers/w1/slaves/w1_therm.c
>
> which would be executed in rmmod context, losing signals.
> Cc:-ed Evgeniy.

w1 uses a little bit strange refcnt logic, basically every w1 module waits
for all its users to release their w1 resources and doesn't exit until its safe.

To wait for resources to be freed at module exit time it checks its refcnt to drop to zero periodically
and sleeps between the checks for a second. w1 flushes signals between these
checks for no particular reason, it can be safely removed from w1_unregister_family()
and interruptible sleep replaced with the normal one.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/