[PATCH 1/3] posix-timers: remove idr_lock

[PATCH 1/3] posix-timers: remove idr_lock

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

idr_lock is only used to protect the global posix_timers idr,
but idr does its own locking (or RCUing) anyways and doesn't
need it. So remove it.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 kernel/posix-timers.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 4556182..a6dcc10 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -71,7 +71,6 @@
  */
 static struct kmem_cache *posix_timers_cache;
 static struct idr posix_timers_id;
-static DEFINE_SPINLOCK(idr_lock);
 
 /*
  * we assume that the new SIGEV_THREAD_ID shares no bits with the other
@@ -502,12 +501,8 @@ static void k_itimer_rcu_free(struct rcu_head *head)
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 {
-	if (it_id_set) {
-		unsigned long flags;
-		spin_lock_irqsave(&idr_lock, flags);
+	if (it_id_set)
 		idr_remove(&posix_timers_id, tmr->it_id);
-		spin_unlock_irqrestore(&idr_lock, flags);
-	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
 	call_rcu(&tmr->it.rcu, k_itimer_rcu_free);
@@ -557,9 +552,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 		error = -EAGAIN;
 		goto out;
 	}
-	spin_lock_irq(&idr_lock);
 	error = idr_get_new(&posix_timers_id, new_timer, &new_timer_id);
-	spin_unlock_irq(&idr_lock);
 	if (error) {
 		if (error == -EAGAIN)
 			goto retry;
-- 
1.7.4.4

[PATCH 2/3] posix-timers: move global timer id management to signal_struct

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Move the global posix timer ids IDR to signal_struct. This removes
a minor global scalability bottleneck and also allows to finally limit
the number of process timers in a sane way (see next patch)

I put it into signal_struct following the other posix timer per process
structures.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 include/linux/init_task.h |    2 ++
 include/linux/sched.h     |    3 +++
 kernel/fork.c             |    2 ++
 kernel/posix-timers.c     |   17 ++++++++++-------
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 580f70c..5c34aab 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -10,6 +10,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/user_namespace.h>
 #include <linux/securebits.h>
+#include <linux/idr.h>
 #include <net/net_namespace.h>
 
 #ifdef CONFIG_SMP
@@ -46,6 +47,7 @@ extern struct fs_struct init_fs;
 	},								\
 	.cred_guard_mutex =						\
 		 __MUTEX_INITIALIZER(sig.cred_guard_mutex),		\
+	.posix_timers_id = IDR_INIT(posix_timer_id),			\
 	INIT_THREADGROUP_FORK_LOCK(sig)					\
 }
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a837b20..2f738ed 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -62,6 +62,7 @@ struct sched_param {
 #include <linux/errno.h>
 #include <linux/nodemask.h>
 #include <linux/mm_types.h>
+#include <linux/idr.h>
 
 #include <asm/system.h>
 #include <asm/page.h>
@@ -652,6 +653,8 @@ struct signal_struct {
 	struct mutex cred_guard_mutex;	/* guard against foreign influences on
 					 * credential calculations
 					 * (notably. ptrace) */
+
+	struct idr posix_timers_id;
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */
diff --git a/kernel/fork.c b/kernel/fork.c
index 0276c30..90ee129 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -942,6 +942,8 @@ static void posix_cpu_timers_init_group(struct signal_struct *sig)
 	INIT_LIST_HEAD(&sig->cpu_timers[0]);
 	INIT_LIST_HEAD(&sig->cpu_timers[1]);
 	INIT_LIST_HEAD(&sig->cpu_timers[2]);
+
+	idr_init(&sig->posix_timers_id);
 }
 
 static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index a6dcc10..7351e51 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -70,7 +70,6 @@
  * Lets keep our timers in a slab cache :-)
  */
 static struct kmem_cache *posix_timers_cache;
-static struct idr posix_timers_id;
 
 /*
  * we assume that the new SIGEV_THREAD_ID shares no bits with the other
@@ -281,7 +280,6 @@ static __init int init_posix_timers(void)
 	posix_timers_cache = kmem_cache_create("posix_timers_cache",
 					sizeof (struct k_itimer), 0, SLAB_PANIC,
 					NULL);
-	idr_init(&posix_timers_id);
 	return 0;
 }
 
@@ -501,8 +499,10 @@ static void k_itimer_rcu_free(struct rcu_head *head)
 #define IT_ID_NOT_SET	0
 static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 {
-	if (it_id_set)
-		idr_remove(&posix_timers_id, tmr->it_id);
+	if (it_id_set) {
+		struct signal_struct *s = current->signal;
+		idr_remove(&s->posix_timers_id, tmr->it_id);
+	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
 	call_rcu(&tmr->it.rcu, k_itimer_rcu_free);
@@ -536,6 +536,7 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	int error, new_timer_id;
 	sigevent_t event;
 	int it_id_set = IT_ID_NOT_SET;
+	struct signal_struct *s = current->signal;
 
 	if (!kc)
 		return -EINVAL;
@@ -548,11 +549,11 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 
 	spin_lock_init(&new_timer->it_lock);
  retry:
-	if (unlikely(!idr_pre_get(&posix_timers_id, GFP_KERNEL))) {
+	if (unlikely(!idr_pre_get(&s->posix_timers_id, GFP_KERNEL))) {
 		error = -EAGAIN;
 		goto out;
 	}
-	error = idr_get_new(&posix_timers_id, new_timer, &new_timer_id);
+	error = idr_get_new(&s->posix_timers_id, new_timer, &new_timer_id);
 	if (error) {
 		if (error == -EAGAIN)
 			goto retry;
@@ -631,9 +632,10 @@ out:
 static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags)
 {
 	struct k_itimer *timr;
+	struct signal_struct *s = current->signal;
 
 	rcu_read_lock();
-	timr = idr_find(&posix_timers_id, (int)timer_id);
+	timr = idr_find(&s->posix_timers_id, (int)timer_id);
 	if (timr) {
 		spin_lock_irqsave(&timr->it_lock, *flags);
 		if (timr->it_signal == current->signal) {
@@ -938,6 +940,7 @@ void exit_itimers(struct signal_struct *sig)
 		tmr = list_entry(sig->posix_timers.next, struct k_itimer, list);
 		itimer_delete(tmr);
 	}
+	idr_destroy(&sig->posix_timers_id);
 }
 
 SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
-- 
1.7.4.4

[PATCH 3/3] posix-timers: limit the number of posix timers per process

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Now this is the main reason I wrote the whole patchkit: previously
there was no limit on the maximum number of POSIX timers a process
could allocate.  This limits the amount of unswappable kernel memory
a process can pin down this way.

With the POSIX timer ids being per process we can do this limit
per process now without allowing one process DoSing another.

I implemented it as a sysctl, not a rlimit for now, because
there was no clear use case for rlimit.

The 1024 default is completely arbitrary, but seems reasonable
for now.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 Documentation/sysctl/kernel.txt |    7 +++++++
 kernel/posix-timers.c           |    8 ++++++++
 kernel/sysctl.c                 |    9 +++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 5e7cb39..311c4b3 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -68,6 +68,7 @@ show up in /proc/sys/kernel:
 - threads-max
 - unknown_nmi_panic
 - version
+- max_posix_timers
 
 ==============================================================
 
@@ -555,3 +556,9 @@ A small number of systems do generate NMI's for bizarre random reasons such as
 power management so the default is off. That sysctl works like the existing
 panic controls already in that directory.
 
+===============================================================
+
+max_posix_timers
+
+The maximum number of POSIX timer ids per process.
+
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 7351e51..2d5b1f0 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -71,6 +71,8 @@
  */
 static struct kmem_cache *posix_timers_cache;
 
+int sysctl_max_posix_timers __read_mostly = 1024;
+
 /*
  * we assume that the new SIGEV_THREAD_ID shares no bits with the other
  * SIGEV values.  Here we put out an error if this assumption fails.
@@ -567,6 +569,12 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 
 	it_id_set = IT_ID_SET;
 	new_timer->it_id = (timer_t) new_timer_id;
+
+	if (new_timer_id >= sysctl_max_posix_timers) {
+		error = -EMFILE;  /* better error? */
+		goto out;
+	}
+
 	new_timer->it_clock = which_clock;
 	new_timer->it_overrun = -1;
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f175d98..d24eec6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -108,6 +108,7 @@ extern int sysctl_nr_trim_pages;
 #ifdef CONFIG_BLOCK
 extern int blk_iopoll_enabled;
 #endif
+extern int sysctl_max_posix_timers;
 
 /* Constants used for minimum and  maximum */
 #ifdef CONFIG_LOCKUP_DETECTOR
@@ -984,6 +985,14 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 #endif
+	{
+		.procname	= "max_posix_timers",
+		.data		= &sysctl_max_posix_timers,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
+
 	{ }
 };
 
-- 
1.7.4.4

Re: [PATCH 1/3] posix-timers: remove idr_lock

[Eric Dumazet]

Le vendredi 10 juin 2011 à 13:13 -0700, Andi Kleen a écrit :
> From: Andi Kleen <ak@linux.intel.com>
> 
> idr_lock is only used to protect the global posix_timers idr,
> but idr does its own locking (or RCUing) anyways and doesn't
> need it. So remove it.

Are you sure of this assertion Andi ?

I believe we need an external synchronization before calling
idr_remove() / idr_get_new()

Re: [PATCH 1/3] posix-timers: remove idr_lock

[Andi Kleen]

On Fri, Jun 10, 2011 at 10:40:38PM +0200, Eric Dumazet wrote:
> Le vendredi 10 juin 2011 à 13:13 -0700, Andi Kleen a écrit :
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > idr_lock is only used to protect the global posix_timers idr,
> > but idr does its own locking (or RCUing) anyways and doesn't
> > need it. So remove it.
> 
> Are you sure of this assertion Andi ?
> 
> I believe we need an external synchronization before calling
> idr_remove() / idr_get_new()

idr has its own lock and from a quick check it seemed to always
take it when needed. Also ipc/* don't take a lock for this as far
as I can see, and I think RCU idr was designed for that.

-Andi

Re: [PATCH 1/3] posix-timers: remove idr_lock

[Eric Dumazet]

Le vendredi 10 juin 2011 à 22:44 +0200, Andi Kleen a écrit :
> On Fri, Jun 10, 2011 at 10:40:38PM +0200, Eric Dumazet wrote:
> > Le vendredi 10 juin 2011 à 13:13 -0700, Andi Kleen a écrit :
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > idr_lock is only used to protect the global posix_timers idr,
> > > but idr does its own locking (or RCUing) anyways and doesn't
> > > need it. So remove it.
> > 
> > Are you sure of this assertion Andi ?
> > 
> > I believe we need an external synchronization before calling
> > idr_remove() / idr_get_new()
> 
> idr has its own lock and from a quick check it seemed to always
> take it when needed. Also ipc/* don't take a lock for this as far
> as I can see, and I think RCU idr was designed for that.
> 

I dont think so. Just read idr_remove() code : its obviously unsafe.

RCU permits readers to only hold rcu_read_lock()

But writers need their (external) synchronization.