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, 4 Jun 2008 11:33:18 -0600
From:	Matthew Wilcox <matthew@....cx>
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Dmitry Adamushko <dmitry.adamushko@...il.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] schedule: fix TASK_WAKEKILL vs SIGKILL race

On Wed, Jun 04, 2008 at 09:09:05PM +0400, Oleg Nesterov wrote:
> Note this "__TASK_STOPPED | __TASK_TRACED" check in signal_pending_state().
> Probably it would be better to remove it, but this will change the current
> behaviour and thus needs a separate discussion.

We're changing the behaviour anyway.  Let's have the discussion and get
it right.

In my opinion, not checking for TASK_STOPPED or TASK_TRACED previously was
an oversight.  This should be fixed.

> Note also that with or without this patch TASK_WAKEKILL is not exactly right
> wrt /sbin/init, but this is another issue.

That's certainly an interesting conversation to have.

> +int signal_pending_state(long state, struct task_struct *p)
> +{
> +	if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
> +		return 0;
> +	if (!signal_pending(p))
> +		return 0;
> +
> +	if (state & TASK_INTERRUPTIBLE)
> +		return 1;
> +	if (state & (__TASK_STOPPED | __TASK_TRACED))
> +		return 0;

Just deleting the above two lines should do it?

> +	return __fatal_signal_pending(p);
> +}
> +
>  struct sighand_struct *lock_task_sighand(struct task_struct *tsk, unsigned long *flags)
>  {
>  	struct sighand_struct *sighand;
> --- 26-rc2/kernel/sched.c~1_SCHED_KILLABLE	2008-05-18 15:44:18.000000000 +0400
> +++ 26-rc2/kernel/sched.c	2008-06-04 17:42:59.000000000 +0400
> @@ -4510,12 +4510,10 @@ need_resched_nonpreemptible:
>  	clear_tsk_need_resched(prev);
>  
>  	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
> -		if (unlikely((prev->state & TASK_INTERRUPTIBLE) &&
> -				signal_pending(prev))) {
> +		if (unlikely(signal_pending_state(prev->state, prev)))
>  			prev->state = TASK_RUNNING;
> -		} else {
> +		else
>  			deactivate_task(rq, prev, 1);
> -		}

Getting rid of the extra braces is against CodingStyle:

  Do not unnecessarily use braces where a single statement will do.

  if (condition)
          action();

  This does not apply if one branch of a conditional statement is a single
  statement. Use braces in both branches.

  if (condition) {
          do_this();
          do_that();
  } else {
          otherwise();
  }

This patch is going to add quite a few cycles to schedule().  Has anyone
done any benchmarks with a schedule-heavy workload?

I don't think signal_pending_state() should be in signal.c, just put it
in sched.c along with its only caller.  That way, gcc can choose to
inline it if that's more efficient.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--
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/

Powered by blists - more mailing lists