[Devel] [PATCH 0/2] fuse: fix signals handling while processing request

[Maxim Patlasov]

Stas,


On 10/14/2016 03:30 AM, Stanislav Kinsburskiy wrote:

>
>
> 14.10.2016 02:23, Maxim Patlasov пишет:
>> Stas,
>>
>>
>> The series look fine, so:
>>
>> Acked-by: Maxim Patlasov <mpatlasov at virtuozzo.com>
>>
>>
>> But, please, refine the description of the second patch. It must 
>> explain clearly why the patch fixes the problem:
>>
>> block_sigs() blocks ordinary non-fatal signals as expected, but 
>> surprisingly SIGTRAP is special: it does not matter whether it comes 
>> before or after block_sigs(), the latter does not affect SIGTRAP at 
>> all! And in contrast, wait_event_killable() is immune to it -- only 
>> fatal sig can wake it up.
>>
>
> No, Maxim. You make a mistake here.

Yes, I agree.

>
> There is nothing special with SIGTRAP (although it's being sometimes 
> sent via force_sig_info()).

OK.

>
> The problem is described as it is: block_sigs() doesn't (!) clear 
> TIG_SIGPENDING flag. All it does is blocking future signals to arrive.

OK. But I disagree with your explanation why it doesn't clear the flag.

>
> Moreover, __set_task_blocked() call recalc_sigpending(), which check 
> whether any of the signals to block is present in process pending 
> mask, and if so - set (!) TIF_SIGPENDING on the task.

Only if the correspondent bit is NOT set in blocked->sig[]:

 > case 1: ready  = signal->sig[0] &~ blocked->sig[0];

That's definitely not the case for block_sigs() who set all bits in 
blocked->sig[] except sigmask(SIGKILL). So, in our case 
recalc_sigpending() can only clear TIG_SIGPENDING flag, not set it.

> IOW, any pending signal remains pending after call to blocks_sigs().

No. Conversely: all non-fatal signals does NOT remain pending after call 
to blocks_sigs(). You can ascertain it yourself by debugging how 
block_sigs() react on "kill -USR1".

> And that's is the root of the issue (as it described in the patch 
> comment).

No. The root of the issue is in ptrace(2) calling ptrace_request(), 
calling task_set_jobctl_pending(), setting JOBCTL_TRAP_STOP in 
task->jobctl. So, when fuse calls block_sigs(), it eventually calls 
recalc_sigpending() which calls recalc_sigpending_tsk() which looks like 
this:

 >    if ((t->jobctl & JOBCTL_PENDING_MASK) ||
 >        PENDING(&t->pending, &t->blocked) ||
 >        PENDING(&t->signal->shared_pending, &t->blocked)) {
 >        set_tsk_thread_flag(t, TIF_SIGPENDING);
 >        return 1;

but as we know ptrace(2) already set JOBCTL_TRAP_STOP in task->jobctl:

 > #define JOBCTL_TRAP_MASK    (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)
 > #define JOBCTL_PENDING_MASK    (JOBCTL_STOP_PENDING | JOBCTL_TRAP_MASK)

To sum it up, the patch from Al Viro that you backported doesn't change 
fuse behavior w.r.t signals, but it nicely replace signal_pending with 
fatal_signal_pending, and the latter solves our case because it checks 
for SIGKILL explicitly:

 > static inline int fatal_signal_pending(struct task_struct *p)
 > {
 >     return signal_pending(p) && __fatal_signal_pending(p);
 > }

 > static inline int __fatal_signal_pending(struct task_struct *p)
 > {
 >     return unlikely(sigismember(&p->pending.signal, SIGKILL));
 > }

Thanks,
Maxim

>
>> Thanks,
>> Maxim
>>
>> On 10/13/2016 03:03 AM, Stanislav Kinsburskiy wrote:
>>> This patch fixes wrong SIGBUS result in page fault handler for fuse 
>>> file, when
>>> process received a signal.
>>>
>>> https://jira.sw.ru/browse/PSBM-53581
>>>
>>> ---
>>>
>>> Stanislav Kinsburskiy (2):
>>>        new helper: wait_event_killable_exclusive()
>>>        fuse: handle only fatal signals while waiting request answer
>>>
>>>
>>>   fs/fuse/dev.c        |   42 
>>> ++++++++++++++++--------------------------
>>>   include/linux/wait.h |   26 ++++++++++++++++++++++++++
>>>   2 files changed, 42 insertions(+), 26 deletions(-)
>>>
>>> -- 
>>
>