linux-kernel - Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 Apr 2011 14:57:10 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>,
	"Nikita V. Youshchenko" <nyoushchenko@...sta.com>,
	Matt Fleming <matt@...sole-pimps.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs
	retarget_shared_pending()

On 04/26, Linus Torvalds wrote:
>
> > +               sigemptyset(&new_full_set);
> > +               if (how == SIG_SETMASK)
> > +                       new_full_set = current->blocked;
> > +               new_full_set.sig[0] = new_set;
>
> Ugh. This is just ugly.

Agreed.

> Could we not instead turn the whole thing into a "clear these bits"
> and "set these bits", and get rid of the "how" entirely for the helper
> function?
>
> IOW, we'd have
>
>   switch (how) {
>   case SIG_BLOCK:
>       clear_bits = 0;
>       set_bits = new_set;
>       break;
>   case SIG_UNBLOCK:
>       clear_bits = new_set;
>       set_bits = 0;
>       break
>   case SIG_SET:
>      clear_bits = low_bits;
>      set_bits = new_set;
>      break;
>    default:
>      return -EINVAL;
>   }
>
> and notice how you now can do that helper function *WITHOUT* any
> conditionals, and just make it do
>
>     sigprocmask(&clear, &set, NULL);
>
> which handles all cases correctly (just "andn clear" + "or set") with
> no if's or switch'es.
>
> This is why I _detest_ that idiotic "sigprocmask()" interface.

Agreed, but...

Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and
sys_sigprocmask() which have to handle these SIG_* operations anyway.
So, I think we should do:

	1. Almost all callers of sigprocmask() use SIG_SETMASK, we can
	   simply change them to use set_current_blocked().

	2. Add the new helper (probably like you suggested) and convert
	   other 9 callers.

	3. Unexport sigprocmask() and remove the last "*oldset" argument,
	   it should be only used by sys_*sigprocmask() syscalls.

But firstly I'd like to finish this "don't change ->blocked directly"
conversion. And this patch changes sys_sigprocmask() so that it looks
similar to sys_rt_sigprocmask().

What do you think?

If you can't accept sys_sigprocmask()->sigprocmask(), will you agree
with

	SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, nset,
			old_sigset_t __user *, oset)
	{
		old_sigset_t old_set, new_set;
		sigset_t new_blocked;

		old_set = current->blocked.sig[0];

		if (nset) {
			if (copy_from_user(&new_set, nset, sizeof(*nset)))
				return -EFAULT;
			new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));

			new_blocked = current->blocked;

			switch (how) {
			case SIG_BLOCK:
				sigaddsetmask(&new_blocked, new_set);
				break;
			case SIG_UNBLOCK:
				sigdelsetmask(&new_blocked, new_set);
				break;
			case SIG_SETMASK:
				new_blocked.sig[0] = new_set;
				break;
			default:
				return -EINVAL;
			}

			set_current_blocked(&new_blocked);
		}

		if (oset) {
			if (copy_to_user(oset, &old_set, sizeof(*oset)))
				return -EFAULT;
		}

		return 0;
	}
?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/