linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stas Sergeev <stsp@list.ru>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Linux kernel <linux-kernel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Amanieu d'Antras <amanieu@gmail.com>,
	Richard Weinberger <richard@nod.at>, Tejun Heo <tj@kernel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Jason Low <jason.low2@hp.com>,
	Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Konstantin Khlebnikov <khlebnikov@yandex-team.ru>,
	Josh Triplett <josh@joshtriplett.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Aleksa Sarai <cyphar@cyphar.com>, Paul Moore <pmoore@redhat.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Vladimir Davydov <vdavydov@parallels.com>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler
Date: Sun, 31 Jan 2016 20:33:27 +0300	[thread overview]
Message-ID: <56AE4567.9000403@list.ru> (raw)
In-Reply-To: <CALCETrU1sB4FDkyrGcxY0CrCRr7qyGW0tzmpyaQDwM0UTjCRMw@mail.gmail.com>

31.01.2016 20:00, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp@list.ru> 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.
>>
>> With this patch, disabling sigaltstack inside a signal handler became
>> possible, and the swapcontext() can then be used safely. After switching
>> back to the sighandler, the app can re-enable the sigatlstack.
>> The oss->ss_flags will correctly indicate the current use of sigaltstack,
>> even if it is temporarily disabled. Any attempt to modify the sigaltstack
>> (rather than to disable or re-enable it) within the sighandler, will still
>> be punished with EPERM as suggested by POSIX.
> This seems considerably more complicated than my previous proposal to
> add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
> let me change the sigaltstack configuration even if it's in use".
> What's the advantage?
To me, the main advantage is to stay POSIX-compatible, rather
than adding the linux-specific flag. Please note that this flag does
not add any value other than to say "I want to ignore POSIX here"
in your interpretation of POSIX, and in my interpretation it doesn't
say even this, because in my interpretation the temporary disabling
is not prohibited. So if it doesn't even fit my interpretation, how would
I write a man description for it? I'll have to first clarify the vague
wording to clearly sound your way, and then add the flag to override
this. This whole procedure looks very illogical to me. So to find out
if it is just me, I'd like to hear from anyone else supporting the idea
of adding this flag. If people think its existence is justified, then fine.
But to me this flag is non-portable, while the both sigaltstack() and
swapcontext() are portable. So what will I gain with adding a
non-portable flag to my apps? A bunch of ifdefs?
IMHO as long as both swapcontext() and sigaltstack() are POSIX-compatible,
they should be compatible with each other in a POSIX-compatible
way. If POSIX needs the linux-specific flags to make them compatible
with each other, then POSIX is inconsistent. So lets just don't interpret
it the way that makes it so.

So in short:
Your concern is the patch complexity. Doing things your way will
however move the problem to the user: he will have to deal with the
linux-specific flags and add ifdefs for just a mere use of a 
posix-compatible
interfaces.

There can also be the subtle technical differences.
With your approach the nested signal can AFAIU overflow the
the disabled sigaltstack because you don't maintain the oss->ss_flags
in a consistent way. There is an overflow protection code:
---
/*
  * If we are on the alternate signal stack and would overflow it, don't.
  * Return an always-bogus address instead so we will die with SIGSEGV.
  */
  if (onsigstack && !likely(on_sig_stack(sp)))
          return (void __user *)-1L;
---
In your approach it will be bypassed.
And its not possible for an app to find out if it is running on a
sigaltstack now or not, after it is disabled.
Yes, maybe minor. But overall I see more disadvantages here
compared to the only disadvantage of my patch: its complexity.
But isn't the kernel developed to make user's life easier? :)
So to make it your way and start to add ifdef HAVE_SS_FORCE
to my apps, I'd like at least any other person to vote for your way.
Then fine.

  reply	other threads:[~2016-01-31 17:33 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-31 16:16 [PATCH 0/4] make sigaltstack() compatible with swapcontext() Stas Sergeev
     [not found] ` <56AE3369.2090709-cmBhpYW9OiY@public.gmane.org>
2016-01-31 16:18   ` [PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler Stas Sergeev
2016-02-12 16:12     ` Shuah Khan
     [not found]       ` <56BE046D.4080203-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
2016-02-12 16:17         ` Stas Sergeev
2016-01-31 16:28   ` [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler Stas Sergeev
     [not found]     ` <56AE3626.7080706-cmBhpYW9OiY@public.gmane.org>
2016-01-31 17:00       ` Andy Lutomirski
2016-01-31 17:33         ` Stas Sergeev [this message]
     [not found]           ` <56AE4567.9000403-cmBhpYW9OiY@public.gmane.org>
2016-01-31 19:03             ` Andy Lutomirski
     [not found]               ` <CALCETrUVODhNRwvbAfC0q3RVJAFw-ZFGhsww2uQsk3UZjLynnQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-01-31 20:08                 ` Stas Sergeev
     [not found]                   ` <56AE69AD.6090005-cmBhpYW9OiY@public.gmane.org>
2016-01-31 20:11                     ` Andy Lutomirski
     [not found]                       ` <CALCETrXPYLqunBNxjS8bpmpg+jG_MXbSyZtUVK3X3m+pGSQ1Og-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-01-31 22:36                         ` Stas Sergeev
     [not found]                           ` <56AE8C80.6030408-cmBhpYW9OiY@public.gmane.org>
2016-01-31 22:44                             ` Andy Lutomirski
     [not found]                               ` <CALCETrU2u7h98oqtMcgvU54j21-bpTfBXUEJNQO9r1w5FHc-HQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-01-31 23:45                                 ` Stas Sergeev
2016-02-01 16:06       ` Oleg Nesterov
     [not found]         ` <20160201160625.GA18276-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-01 16:57           ` Stas Sergeev
     [not found]             ` <56AF8E89.5090400-cmBhpYW9OiY@public.gmane.org>
2016-02-01 17:27               ` Oleg Nesterov
2016-02-01 17:09           ` Oleg Nesterov
2016-02-01 17:26             ` Stas Sergeev
2016-02-01 18:04               ` Oleg Nesterov
     [not found]                 ` <20160201180443.GA21064-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-01 18:16                   ` Stas Sergeev
     [not found]                     ` <56AFA0E2.1030302-cmBhpYW9OiY@public.gmane.org>
2016-02-01 18:28                       ` Andy Lutomirski
     [not found]                         ` <CALCETrWv87BS5hH20qKd7WGuf6EAEb4f78Myq+6fqXgSJWoiew-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-01 18:40                           ` Stas Sergeev
2016-02-01 18:52                       ` Oleg Nesterov
     [not found]                         ` <20160201185223.GA21136-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-01 19:01                           ` Stas Sergeev
2016-02-01 19:29                             ` Oleg Nesterov
     [not found]                               ` <20160201192936.GA21214-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-01 19:46                                 ` Stas Sergeev
2016-02-01 20:41                                   ` Oleg Nesterov
     [not found]                                     ` <20160201204114.GA21638-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-02-01 23:06                                       ` Stas Sergeev
  -- strict thread matches above, loose matches on Subject: below --
2016-01-31 19:10 [PATCH v2 0/4] make sigaltstack() compatible with swapcontext() Stas Sergeev
     [not found] ` <56AE5C08.6010403-cmBhpYW9OiY@public.gmane.org>
2016-01-31 19:18   ` [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler Stas Sergeev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56AE4567.9000403@list.ru \
    --to=stsp@list.ru \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=amanieu@gmail.com \
    --cc=cyphar@cyphar.com \
    --cc=ebiederm@xmission.com \
    --cc=jason.low2@hp.com \
    --cc=josh@joshtriplett.org \
    --cc=khlebnikov@yandex-team.ru \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mingo@redhat.com \
    --cc=oleg@redhat.com \
    --cc=palmer@dabbelt.com \
    --cc=peterz@infradead.org \
    --cc=pmoore@redhat.com \
    --cc=richard@nod.at \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=vdavydov@parallels.com \
    --cc=xypron.glpk@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).