Re: [PATCH] sigaltstack: support SS_AUTODISARM for CONFIG_COMPAT

Re: [PATCH] sigaltstack: support SS_AUTODISARM for CONFIG_COMPAT

[Andy Lutomirski]

On Sat, Feb 4, 2017 at 4:07 AM, Stas Sergeev <stsp@list.ru> wrote:
> Currently SS_AUTODISARM is not supported in compatibility
> mode, but does not return -EINVAL either. This makes dosemu
> built with -m32 on x86_64 to crash. Also the kernel's sigaltstack
> selftest fails if compiled with -m32.
>
> This patch adds the needed support. It also improves the
> selftest output a little (%i changed to %x for bitmasks).
>
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
>

Spurious newline.  Also, should this be cc:stable?

> CC: Shuah Khan <shuahkh@osg.samsung.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Jiri Kosina <jkosina@suse.cz>
> CC: Al Viro <viro@zeniv.linux.org.uk>
> CC: Stephen Bates <stephen.bates@pmcs.com>
> CC: Milosz Tanski <milosz@adfin.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> CC: Helge Deller <deller@gmx.de>
> CC: Wang Xiaoqiang <wangxq10@lzu.edu.cn>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: Sasha Levin <sasha.levin@oracle.com>
> CC: linux-kernel@vger.kernel.org
> ---
>  include/linux/compat.h                    |  4 +++-
>  kernel/signal.c                           | 11 +++++++++--
>  tools/testing/selftests/sigaltstack/sas.c |  7 ++++---
>  3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 6360939..d8535a4 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -711,8 +711,10 @@ int __compat_save_altstack(compat_stack_t __user *, unsigned long);
>         compat_stack_t __user *__uss = uss; \
>         struct task_struct *t = current; \
>         put_user_ex(ptr_to_compat((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); \

Should the sas_ss_flags() helper be deleted, perhaps?  This code seems
a bit odd -- it no longer does the on_sig_stack(sp) check..  (It
matches the non-compat code.)

Re: [PATCH] sigaltstack: support SS_AUTODISARM for CONFIG_COMPAT

[Stas Sergeev]

04.02.2017 20:32, Andy Lutomirski пишет:
> On Sat, Feb 4, 2017 at 4:07 AM, Stas Sergeev <stsp@list.ru> wrote:
>> Currently SS_AUTODISARM is not supported in compatibility
>> mode, but does not return -EINVAL either. This makes dosemu
>> built with -m32 on x86_64 to crash. Also the kernel's sigaltstack
>> selftest fails if compiled with -m32.
>>
>> This patch adds the needed support. It also improves the
>> selftest output a little (%i changed to %x for bitmasks).
>>
>> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
>>
> Spurious newline.
Is this a problem? :)

>    Also, should this be cc:stable?
It doesn't match the stable submission criteries AFAICS.
Can we declare it "obviously correct" and the problem
it fixes "critical"? If so, I'll need to strip the %i -> %x
change from it as it doesn't pass the stable submission
guidelines.

>> diff --git a/include/linux/compat.h b/include/linux/compat.h
>> index 6360939..d8535a4 100644
>> --- a/include/linux/compat.h
>> +++ b/include/linux/compat.h
>> @@ -711,8 +711,10 @@ int __compat_save_altstack(compat_stack_t __user *, unsigned long);
>>          compat_stack_t __user *__uss = uss; \
>>          struct task_struct *t = current; \
>>          put_user_ex(ptr_to_compat((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); \
> Should the sas_ss_flags() helper be deleted, perhaps?  This code seems
> a bit odd -- it no longer does the on_sig_stack(sp) check..  (It
> matches the non-compat code.)

sas_ss_flags() is still used in sigaltstack() code and
in signal delivery code, so why do you think it should
be removed? It is needed to return the correct status
via sigaltstack() and for deciding whether to switch stacks.
But saving and restoring to/from uc_stack must be done
with raw flags. I don't think we need to check on_sig_stack()
when saving to uc_stack, or do we?