From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stas Sergeev Subject: Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler Date: Mon, 1 Feb 2016 19:57:45 +0300 Message-ID: <56AF8E89.5090400@list.ru> References: <56AE3369.2090709@list.ru> <56AE3626.7080706@list.ru> <20160201160625.GA18276@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160201160625.GA18276-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Oleg Nesterov Cc: Linux kernel , linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Andy Lutomirski , Ingo Molnar , Peter Zijlstra , Andrew Morton , Amanieu d'Antras , Richard Weinberger , Tejun Heo , "Kirill A. Shutemov" , Jason Low , Heinrich Schuchardt , Andrea Arcangeli , Konstantin Khlebnikov , Josh Triplett , "Eric W. Biederman" , Aleksa Sarai , Paul Moore , Palmer Dabbelt , Vladimir Davydov List-Id: linux-api@vger.kernel.org 01.02.2016 19:06, Oleg Nesterov =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > Honestly, I am not sure I understand what this patch does and why, an= d it is > white space damaged, please fix. Arrr. > On 01/31, Stas Sergeev wrote: >> linux implements the sigaltstack() in a way that makes it impossible= to >> use with swapcontext(). Per the man page, sigaltstack is allowed to = return >> EPERM if the process is altering its sigaltstack while running on >> sigaltstack. >> This is likely needed to consistently return oss->ss_flags, that ind= icates >> whether the process is being on sigaltstack or not. >> Unfortunately, linux takes that permission to return EPERM too liter= ally: >> it returns EPERM even if you don't want to change to another sigalts= tack, >> but only want to temporarily disable sigaltstack with SS_DISABLE. >> You can't use swapcontext() without disabling sigaltstack first, or = the >> stack will be re-used and overwritten by a subsequent signal. > So iiuc you want to switch the stack from the signal handler running = on the > alt stack, and you need to ensure that another SA_ONSTACK signal won'= t corrupt > the alt stack in between, right? Yes. > Perhaps you can update the changelog to explain why do we want this c= hange. Beyond the fact that swapcontext() is then usable for switching in/out of sigaltstack? But this is already mentioned and I have no other reason for getting this in. >> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long = sp) >> { >> if (!current->sas_ss_size) >> return SS_DISABLE; >> - >> - return on_sig_stack(sp) ? SS_ONSTACK : 0; >> + if (on_sig_stack(sp)) >> + return SS_ONSTACK; >> + if (current->sas_ss_flags =3D=3D SS_DISABLE) >> + return SS_DISABLE; >> + return 0; > So this always return SS_ONSTACK if on_sig_stack(), see below. > >> + onsigstack =3D on_sig_stack(sp); >> + if (ss_size =3D=3D 0) { >> + switch (ss_flags) { >> + case 0: >> + error =3D -EPERM; >> + if (onsigstack) >> + goto out; >> + current->sas_ss_sp =3D 0; >> + current->sas_ss_size =3D 0; >> + current->sas_ss_flags =3D SS_DISABLE; >> + break; >> + case SS_ONSTACK: >> + /* re-enable previously disabled sas */ >> + error =3D -EINVAL; >> + if (current->sas_ss_size =3D=3D 0) >> + goto out; >> + break; >> + default: >> + break; >> + } > and iiuc the "default" case allows you to write SS_DISABLE into ->sas= _ss_flags > even if on_sig_stack(). > > So the sequence is > > // running on alt stack > > sigaltstack(SS_DISABLE); > > temporary_run_on_another_stack(); > > sigaltstack(SS_ONSTACK); > > and SS_DISABLE saves us from another SA_ONSTACK signal, right? Yes. Note: there is a test-case in that patch serie from which you can see or copy/paste the sample code. > But afaics it can only help after we change the stack. Suppose that S= A_ONSTACK signal > comess before temporary_run_on_another_stack(). get_sigframe() should= be fine after > your changes (afaics), it won't pick the alt stack after SS_DISABLE. > > However, unless I missed something save_altstack_ex() will record SS_= ONSTACK in > uc_stack->ss_flags, and after return from signal handler restore_alts= tack() will > enable alt stack again? I don't think so. Please see the following hunk: diff --git a/include/linux/signal.h b/include/linux/signal.h index 92557bb..844b113 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long= ); stack_t __user *__uss =3D uss; \ struct task_struct *t =3D current; \ put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \ - put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \ + put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \ put_user_ex(t->sas_ss_size, &__uss->ss_size); \ } while (0); It pretends as if it changes __save_altstack(), but the reality is that it actually changes save_altstack_ex(). This is some bug in git perhaps (or it can't parse macros), I didn't apply any manual editing to the patch. The hunk that really modifies __save_altstack() also exists btw: @@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned= =20 long sp) { struct task_struct *t =3D current; return __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) | - __put_user(sas_ss_flags(sp), &uss->ss_flags) | + __put_user(t->sas_ss_flags, &uss->ss_flags) | __put_user(t->sas_ss_size, &uss->ss_size); } So I understand this is very confusing, but I think the patch is correct. Do you think adding the SS_FORCE flag would be a better solution? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754111AbcBAQ6J (ORCPT ); Mon, 1 Feb 2016 11:58:09 -0500 Received: from smtp42.i.mail.ru ([94.100.177.102]:57799 "EHLO smtp42.i.mail.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753510AbcBAQ6H (ORCPT ); Mon, 1 Feb 2016 11:58:07 -0500 Subject: Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler To: Oleg Nesterov References: <56AE3369.2090709@list.ru> <56AE3626.7080706@list.ru> <20160201160625.GA18276@redhat.com> Cc: Linux kernel , linux-api@vger.kernel.org, Andy Lutomirski , Ingo Molnar , Peter Zijlstra , Andrew Morton , "Amanieu d'Antras" , Richard Weinberger , Tejun Heo , "Kirill A. Shutemov" , Jason Low , Heinrich Schuchardt , Andrea Arcangeli , Konstantin Khlebnikov , Josh Triplett , "Eric W. Biederman" , Aleksa Sarai , Paul Moore , Palmer Dabbelt , Vladimir Davydov From: Stas Sergeev Message-ID: <56AF8E89.5090400@list.ru> Date: Mon, 1 Feb 2016 19:57:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160201160625.GA18276@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mras: Ok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 01.02.2016 19:06, Oleg Nesterov пишет: > Honestly, I am not sure I understand what this patch does and why, and it is > white space damaged, please fix. Arrr. > On 01/31, Stas Sergeev wrote: >> linux implements the sigaltstack() in a way that makes it impossible to >> use with swapcontext(). Per the man page, sigaltstack is allowed to return >> EPERM if the process is altering its sigaltstack while running on >> sigaltstack. >> This is likely needed to consistently return oss->ss_flags, that indicates >> whether the process is being on sigaltstack or not. >> Unfortunately, linux takes that permission to return EPERM too literally: >> it returns EPERM even if you don't want to change to another sigaltstack, >> but only want to temporarily disable sigaltstack with SS_DISABLE. >> You can't use swapcontext() without disabling sigaltstack first, or the >> stack will be re-used and overwritten by a subsequent signal. > So iiuc you want to switch the stack from the signal handler running on the > alt stack, and you need to ensure that another SA_ONSTACK signal won't corrupt > the alt stack in between, right? Yes. > Perhaps you can update the changelog to explain why do we want this change. Beyond the fact that swapcontext() is then usable for switching in/out of sigaltstack? But this is already mentioned and I have no other reason for getting this in. >> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp) >> { >> if (!current->sas_ss_size) >> return SS_DISABLE; >> - >> - return on_sig_stack(sp) ? SS_ONSTACK : 0; >> + if (on_sig_stack(sp)) >> + return SS_ONSTACK; >> + if (current->sas_ss_flags == SS_DISABLE) >> + return SS_DISABLE; >> + return 0; > So this always return SS_ONSTACK if on_sig_stack(), see below. > >> + onsigstack = on_sig_stack(sp); >> + if (ss_size == 0) { >> + switch (ss_flags) { >> + case 0: >> + error = -EPERM; >> + if (onsigstack) >> + goto out; >> + current->sas_ss_sp = 0; >> + current->sas_ss_size = 0; >> + current->sas_ss_flags = SS_DISABLE; >> + break; >> + case SS_ONSTACK: >> + /* re-enable previously disabled sas */ >> + error = -EINVAL; >> + if (current->sas_ss_size == 0) >> + goto out; >> + break; >> + default: >> + break; >> + } > and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags > even if on_sig_stack(). > > So the sequence is > > // running on alt stack > > sigaltstack(SS_DISABLE); > > temporary_run_on_another_stack(); > > sigaltstack(SS_ONSTACK); > > and SS_DISABLE saves us from another SA_ONSTACK signal, right? Yes. Note: there is a test-case in that patch serie from which you can see or copy/paste the sample code. > But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal > comess before temporary_run_on_another_stack(). get_sigframe() should be fine after > your changes (afaics), it won't pick the alt stack after SS_DISABLE. > > However, unless I missed something save_altstack_ex() will record SS_ONSTACK in > uc_stack->ss_flags, and after return from signal handler restore_altstack() will > enable alt stack again? I don't think so. Please see the following hunk: diff --git a/include/linux/signal.h b/include/linux/signal.h index 92557bb..844b113 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long); stack_t __user *__uss = uss; \ struct task_struct *t = current; \ put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \ - put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \ + put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \ put_user_ex(t->sas_ss_size, &__uss->ss_size); \ } while (0); It pretends as if it changes __save_altstack(), but the reality is that it actually changes save_altstack_ex(). This is some bug in git perhaps (or it can't parse macros), I didn't apply any manual editing to the patch. The hunk that really modifies __save_altstack() also exists btw: @@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned long sp) { struct task_struct *t = current; return __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) | - __put_user(sas_ss_flags(sp), &uss->ss_flags) | + __put_user(t->sas_ss_flags, &uss->ss_flags) | __put_user(t->sas_ss_size, &uss->ss_size); } So I understand this is very confusing, but I think the patch is correct. Do you think adding the SS_FORCE flag would be a better solution?