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

[Stanislav Kinsburskiy]

16.10.2016 05:21, 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.
>
Agreed.

>> 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)
>

Nice catch, thanks.

> 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(-)
>>>>
>>>> -- 
>>>
>>
>