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

[Stanislav Kinsburskiy]

17 окт. 2016 г. 19:23 пользователь Maxim Patlasov <MPatlasov at virtuozzo.com> написал:
>
> Stas,
>
>
> Now, when we fully understand the patch, will you fix description of
> 98cbcb14d?

Yes, I"sent the update to Konstantin already.
Thanks a lot again.

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(-)
> >>>>>
> >>>>> --
> >>>>
> >>>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/devel/attachments/20161017/7ecb1b2d/attachment.html>