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

[Maxim Patlasov]

Stas,


Now, when we fully understand the patch, will you fix description of 
98cbcb14d? The following does not seem correct:


 >    IOW, any signal, arrived to the process, which does page fault 
handling on fuse
 >    file, _before_ request_wait_answer() is called, will lead to request
 >    interruption, producing SIGBUS error in page fault handler 
(filemap_fault).


Thanks,

Maxim


On 10/17/2016 04:59 AM, Stanislav Kinsburskiy wrote:

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