<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=koi8-r">
</head>
<body>
<p dir="ltr"></p>
<p dir="ltr">17 окт. 2016 г. 19:23 пользователь Maxim Patlasov <MPatlasov@virtuozzo.com> написал:<br>
><br>
> Stas,<br>
><br>
><br>
> Now, when we fully understand the patch, will you fix description of <br>
> 98cbcb14d?</p>
<p dir="ltr">Yes, I"sent the update to Konstantin already.<br>
Thanks a lot again.</p>
<p dir="ltr">The following does not seem correct:<br>
><br>
><br>
> > IOW, any signal, arrived to the process, which does page fault <br>
> handling on fuse<br>
> > file, _before_ request_wait_answer() is called, will lead to request<br>
> > interruption, producing SIGBUS error in page fault handler <br>
> (filemap_fault).<br>
><br>
><br>
> Thanks,<br>
><br>
> Maxim<br>
><br>
><br>
> On 10/17/2016 04:59 AM, Stanislav Kinsburskiy wrote:<br>
><br>
> ><br>
> ><br>
> > 16.10.2016 05:21, Maxim Patlasov пишет:<br>
> >> Stas,<br>
> >><br>
> >><br>
> >> On 10/14/2016 03:30 AM, Stanislav Kinsburskiy wrote:<br>
> >><br>
> >>><br>
> >>><br>
> >>> 14.10.2016 02:23, Maxim Patlasov пишет:<br>
> >>>> Stas,<br>
> >>>><br>
> >>>><br>
> >>>> The series look fine, so:<br>
> >>>><br>
> >>>> Acked-by: Maxim Patlasov <mpatlasov@virtuozzo.com><br>
> >>>><br>
> >>>><br>
> >>>> But, please, refine the description of the second patch. It must <br>
> >>>> explain clearly why the patch fixes the problem:<br>
> >>>><br>
> >>>> block_sigs() blocks ordinary non-fatal signals as expected, but <br>
> >>>> surprisingly SIGTRAP is special: it does not matter whether it <br>
> >>>> comes before or after block_sigs(), the latter does not affect <br>
> >>>> SIGTRAP at all! And in contrast, wait_event_killable() is immune to <br>
> >>>> it -- only fatal sig can wake it up.<br>
> >>>><br>
> >>><br>
> >>> No, Maxim. You make a mistake here.<br>
> >><br>
> >> Yes, I agree.<br>
> >><br>
> >>><br>
> >>> There is nothing special with SIGTRAP (although it's being sometimes <br>
> >>> sent via force_sig_info()).<br>
> >><br>
> >> OK.<br>
> >><br>
> >>><br>
> >>> The problem is described as it is: block_sigs() doesn't (!) clear <br>
> >>> TIG_SIGPENDING flag. All it does is blocking future signals to arrive.<br>
> >><br>
> >> OK. But I disagree with your explanation why it doesn't clear the flag.<br>
> >><br>
> >>><br>
> >>> Moreover, __set_task_blocked() call recalc_sigpending(), which check <br>
> >>> whether any of the signals to block is present in process pending <br>
> >>> mask, and if so - set (!) TIF_SIGPENDING on the task.<br>
> >><br>
> >> Only if the correspondent bit is NOT set in blocked->sig[]:<br>
> >><br>
> >> > case 1: ready = signal->sig[0] &~ blocked->sig[0];<br>
> >><br>
> >> That's definitely not the case for block_sigs() who set all bits in <br>
> >> blocked->sig[] except sigmask(SIGKILL). So, in our case <br>
> >> recalc_sigpending() can only clear TIG_SIGPENDING flag, not set it.<br>
> >><br>
> > Agreed.<br>
> ><br>
> >>> IOW, any pending signal remains pending after call to blocks_sigs().<br>
> >><br>
> >> No. Conversely: all non-fatal signals does NOT remain pending after <br>
> >> call to blocks_sigs(). You can ascertain it yourself by debugging how <br>
> >> block_sigs() react on "kill -USR1".<br>
> >><br>
> >>> And that's is the root of the issue (as it described in the patch <br>
> >>> comment).<br>
> >><br>
> >> No. The root of the issue is in ptrace(2) calling ptrace_request(), <br>
> >> calling task_set_jobctl_pending(), setting JOBCTL_TRAP_STOP in <br>
> >> task->jobctl. So, when fuse calls block_sigs(), it eventually calls <br>
> >> recalc_sigpending() which calls recalc_sigpending_tsk() which looks <br>
> >> like this:<br>
> >><br>
> >> > if ((t->jobctl & JOBCTL_PENDING_MASK) ||<br>
> >> > PENDING(&t->pending, &t->blocked) ||<br>
> >> > PENDING(&t->signal->shared_pending, &t->blocked)) {<br>
> >> > set_tsk_thread_flag(t, TIF_SIGPENDING);<br>
> >> > return 1;<br>
> >><br>
> >> but as we know ptrace(2) already set JOBCTL_TRAP_STOP in task->jobctl:<br>
> >><br>
> >> > #define JOBCTL_TRAP_MASK (JOBCTL_TRAP_STOP | JOBCTL_TRAP_NOTIFY)<br>
> >> > #define JOBCTL_PENDING_MASK (JOBCTL_STOP_PENDING | <br>
> >> JOBCTL_TRAP_MASK)<br>
> >><br>
> ><br>
> > Nice catch, thanks.<br>
> ><br>
> >> To sum it up, the patch from Al Viro that you backported doesn't <br>
> >> change fuse behavior w.r.t signals, but it nicely replace <br>
> >> signal_pending with fatal_signal_pending, and the latter solves our <br>
> >> case because it checks for SIGKILL explicitly:<br>
> >><br>
> >> > static inline int fatal_signal_pending(struct task_struct *p)<br>
> >> > {<br>
> >> > return signal_pending(p) && __fatal_signal_pending(p);<br>
> >> > }<br>
> >><br>
> >> > static inline int __fatal_signal_pending(struct task_struct *p)<br>
> >> > {<br>
> >> > return unlikely(sigismember(&p->pending.signal, SIGKILL));<br>
> >> > }<br>
> >><br>
> >> Thanks,<br>
> >> Maxim<br>
> >><br>
> >>><br>
> >>>> Thanks,<br>
> >>>> Maxim<br>
> >>>><br>
> >>>> On 10/13/2016 03:03 AM, Stanislav Kinsburskiy wrote:<br>
> >>>>> This patch fixes wrong SIGBUS result in page fault handler for <br>
> >>>>> fuse file, when<br>
> >>>>> process received a signal.<br>
> >>>>><br>
> >>>>> https://jira.sw.ru/browse/PSBM-53581<br>
> >>>>><br>
> >>>>> ---<br>
> >>>>><br>
> >>>>> Stanislav Kinsburskiy (2):<br>
> >>>>> new helper: wait_event_killable_exclusive()<br>
> >>>>> fuse: handle only fatal signals while waiting request answer<br>
> >>>>><br>
> >>>>><br>
> >>>>> fs/fuse/dev.c | 42 <br>
> >>>>> ++++++++++++++++--------------------------<br>
> >>>>> include/linux/wait.h | 26 ++++++++++++++++++++++++++<br>
> >>>>> 2 files changed, 42 insertions(+), 26 deletions(-)<br>
> >>>>><br>
> >>>>> -- <br>
> >>>><br>
> >>><br>
> >><br>
> ><br>
></p>
</body>
</html>