[PATCH 1/2] posix_timer: separate timer_cleanup from timer_del

[PATCH 1/2] posix_timer: separate timer_cleanup from timer_del

[Andrey Vagin]

Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 include/linux/posix-timers.h |    2 ++
 kernel/posix-cpu-timers.c    |   13 ++++++++++---
 kernel/posix-timers.c        |    4 ++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 4f71bf4..613ab27 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -78,6 +78,7 @@ struct k_clock {
 	int (*timer_set) (struct k_itimer * timr, int flags,
 			  struct itimerspec * new_setting,
 			  struct itimerspec * old_setting);
+	void (*timer_cleanup) (struct k_itimer * timr);
 	int (*timer_del) (struct k_itimer * timr);
 #define TIMER_RETRY 1
 	void (*timer_get) (struct k_itimer * timr,
@@ -103,6 +104,7 @@ int posix_cpu_nsleep(const clockid_t which_clock, int flags,
 long posix_cpu_nsleep_restart(struct restart_block *restart_block);
 int posix_cpu_timer_set(struct k_itimer *timer, int flags,
 			struct itimerspec *new, struct itimerspec *old);
+void posix_cpu_timer_cleanup(struct k_itimer *timer);
 int posix_cpu_timer_del(struct k_itimer *timer);
 void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp);
 
diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index bc7704b..7a57833 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -455,14 +455,21 @@ int posix_cpu_timer_del(struct k_itimer *timer)
 			spin_unlock(&p->sighand->siglock);
 		}
 		read_unlock(&tasklist_lock);
-
-		if (!ret)
-			put_task_struct(p);
 	}
 
+	if (!ret)
+		posix_cpu_timer_cleanup(timer);
+
 	return ret;
 }
 
+void posix_cpu_timer_cleanup(struct k_itimer *timer)
+{
+	struct task_struct *p = timer->it.cpu.task;
+	if (likely(p != NULL))
+		put_task_struct(p);
+}
+
 /*
  * Clean out CPU timers still ticking when a thread exited.  The task
  * pointer is cleared, and the expiry time is replaced with the residual
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 00d1fda..1195668 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -197,6 +197,10 @@ static int common_timer_create(struct k_itimer *new_timer)
 	return 0;
 }
 
+static inline void common_timer_cleanup(struct k_itimer *timer)
+{
+}
+
 static int no_timer_create(struct k_itimer *new_timer)
 {
 	return -EOPNOTSUPP;
-- 
1.6.6

[PATCH 2/2] posix_timer: clean up properly if anything fails after *_timer_create

[Andrey Vagin]

*_timer_create may allocate/get some resources, so need call *_timer_cleanup.

https://bugzilla.sw.ru/show_bug.cgi?id=473702

Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 kernel/posix-timers.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 1195668..8d7cb0e 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -574,19 +574,19 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	if (copy_to_user(created_timer_id,
 			 &new_timer_id, sizeof (new_timer_id))) {
 		error = -EFAULT;
-		goto out;
+		goto out_cleanup;
 	}
 	if (timer_event_spec) {
 		if (copy_from_user(&event, timer_event_spec, sizeof (event))) {
 			error = -EFAULT;
-			goto out;
+			goto out_cleanup;
 		}
 		rcu_read_lock();
 		new_timer->it_pid = get_pid(good_sigevent(&event));
 		rcu_read_unlock();
 		if (!new_timer->it_pid) {
 			error = -EINVAL;
-			goto out;
+			goto out_cleanup;
 		}
 	} else {
 		event.sigev_notify = SIGEV_SIGNAL;
@@ -613,6 +613,8 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	 * and may cease to exist at any time.  Don't use or modify
 	 * new_timer after the unlock call.
 	 */
+out_cleanup:
+	CLOCK_DISPATCH(new_timer->it_clock, timer_cleanup, (new_timer));
 out:
 	release_posix_timer(new_timer, it_id_set);
 	return error;
-- 
1.6.6

Re: [PATCH 2/2] posix_timer: clean up properly if anything fails after *_timer_create

[Oleg Nesterov]

On 05/14, Andrey Vagin wrote:
>
> *_timer_create may allocate/get some resources, so need call *_timer_cleanup.
>
> https://bugzilla.sw.ru/show_bug.cgi?id=473702

Authorization Required

> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -574,19 +574,19 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>  	if (copy_to_user(created_timer_id,
>  			 &new_timer_id, sizeof (new_timer_id))) {
>  		error = -EFAULT;
> -		goto out;
> +		goto out_cleanup;
>  	}
>  	if (timer_event_spec) {
>  		if (copy_from_user(&event, timer_event_spec, sizeof (event))) {
>  			error = -EFAULT;
> -			goto out;
> +			goto out_cleanup;
>  		}
>  		rcu_read_lock();
>  		new_timer->it_pid = get_pid(good_sigevent(&event));
>  		rcu_read_unlock();
>  		if (!new_timer->it_pid) {
>  			error = -EINVAL;
> -			goto out;
> +			goto out_cleanup;
>  		}
>  	} else {
>  		event.sigev_notify = SIGEV_SIGNAL;
> @@ -613,6 +613,8 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>  	 * and may cease to exist at any time.  Don't use or modify
>  	 * new_timer after the unlock call.
>  	 */
> +out_cleanup:
> +	CLOCK_DISPATCH(new_timer->it_clock, timer_cleanup, (new_timer));

But at first glance you are right, posix_cpu_timer_create() does
get_task_struct(it.cpu.task).

Oleg.

Re: [PATCH 2/2] posix_timer: clean up properly if anything fails after *_timer_create

[Stanislaw Gruszka]

On Fri, 14 May 2010 18:03:57 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/14, Andrey Vagin wrote:
> >
> > *_timer_create may allocate/get some resources, so need call *_timer_cleanup.
> >
> > https://bugzilla.sw.ru/show_bug.cgi?id=473702
> 
> Authorization Required
> 
> > --- a/kernel/posix-timers.c
> > +++ b/kernel/posix-timers.c
> > @@ -574,19 +574,19 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
> >  	if (copy_to_user(created_timer_id,
> >  			 &new_timer_id, sizeof (new_timer_id))) {
> >  		error = -EFAULT;
> > -		goto out;
> > +		goto out_cleanup;
> >  	}
> >  	if (timer_event_spec) {
> >  		if (copy_from_user(&event, timer_event_spec, sizeof (event))) {
> >  			error = -EFAULT;
> > -			goto out;
> > +			goto out_cleanup;
> >  		}
> >  		rcu_read_lock();
> >  		new_timer->it_pid = get_pid(good_sigevent(&event));
> >  		rcu_read_unlock();
> >  		if (!new_timer->it_pid) {
> >  			error = -EINVAL;
> > -			goto out;
> > +			goto out_cleanup;
> >  		}
> >  	} else {
> >  		event.sigev_notify = SIGEV_SIGNAL;
> > @@ -613,6 +613,8 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
> >  	 * and may cease to exist at any time.  Don't use or modify
> >  	 * new_timer after the unlock call.
> >  	 */
> > +out_cleanup:
> > +	CLOCK_DISPATCH(new_timer->it_clock, timer_cleanup, (new_timer));
> 
> But at first glance you are right, posix_cpu_timer_create() does
> get_task_struct(it.cpu.task).

If I understand problem correctly, seems to be fine to move
CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
after all possible EFAULT errors and solve leak without creating
new timer_cleanup() callback.

Stanislaw

Re: [PATCH 2/2] posix_timer: clean up properly if anything fails after *_timer_create

[Oleg Nesterov]

On 05/14, Stanislaw Gruszka wrote:
>
> On Fri, 14 May 2010 18:03:57 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 05/14, Andrey Vagin wrote:
> > >
> > > @@ -613,6 +613,8 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
> > >  	 * and may cease to exist at any time.  Don't use or modify
> > >  	 * new_timer after the unlock call.
> > >  	 */
> > > +out_cleanup:
> > > +	CLOCK_DISPATCH(new_timer->it_clock, timer_cleanup, (new_timer));
> >
> > But at first glance you are right, posix_cpu_timer_create() does
> > get_task_struct(it.cpu.task).
>
> If I understand problem correctly, seems to be fine to move
> CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
> after all possible EFAULT errors and solve leak without creating
> new timer_cleanup() callback.

I thought about this too, we are doing copy_to_user(created_timer_id)
"in advance" anyway. Probably we can move all this code block

	new_timer->it_id = (timer_t) new_timer_id;
	new_timer->it_clock = which_clock;
	new_timer->it_overrun = -1;
	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
	if (error)
		goto out;

down, right before we take ->siglock.

But I don't understand the change in posix_cpu_timer_del() from 1/2.


Otoh, currently "The next step is hard to back out if there is an error"
comment is not right, release_posix_timer() does put_pid(). We can
move copy_to_user(created_timer_id) down after "if (timer_event_spec)"
block too. (but before CLOCK_DISPATCH(), of course).

Andrey, what do you think?

Oleg.

Re: [PATCH 2/2] posix_timer: clean up properly if anything fails after *_timer_create

[Andrew Vagin]

On 05/14/2010 10:48 PM, Oleg Nesterov wrote:
> On 05/14, Stanislaw Gruszka wrote:
>    
>> On Fri, 14 May 2010 18:03:57 +0200
>> Oleg Nesterov<oleg@redhat.com>  wrote:
>>
>>      
>>> On 05/14, Andrey Vagin wrote:
>>>        
>>>> @@ -613,6 +613,8 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>>>>   	 * and may cease to exist at any time.  Don't use or modify
>>>>   	 * new_timer after the unlock call.
>>>>   	 */
>>>> +out_cleanup:
>>>> +	CLOCK_DISPATCH(new_timer->it_clock, timer_cleanup, (new_timer));
>>>>          
>>> But at first glance you are right, posix_cpu_timer_create() does
>>> get_task_struct(it.cpu.task).
>>>        
>> If I understand problem correctly, seems to be fine to move
>> CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
>> after all possible EFAULT errors and solve leak without creating
>> new timer_cleanup() callback.
>>      
> I thought about this too, we are doing copy_to_user(created_timer_id)
> "in advance" anyway. Probably we can move all this code block
>
> 	new_timer->it_id = (timer_t) new_timer_id;
> 	new_timer->it_clock = which_clock;
> 	new_timer->it_overrun = -1;
> 	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
> 	if (error)
> 		goto out;
>
> down, right before we take ->siglock.
>    
You are right. I will send a new patch sooner. Thanks for your comments.
> But I don't understand the change in posix_cpu_timer_del() from 1/2.
>    
timer_cleanup doesn't do the disarm timer. In case fail in timer_create 
it's enough to call timer_cleanup. This changes are not necessary in new 
version.
>
> Otoh, currently "The next step is hard to back out if there is an error"
> comment is not right, release_posix_timer() does put_pid(). We can
> move copy_to_user(created_timer_id) down after "if (timer_event_spec)"
> block too. (but before CLOCK_DISPATCH(), of course).
>
> Andrey, what do you think?
>    
I've look at code again and think that you are right. At first I created 
patches for 2.6.18 kernel, more complex code in this place and the 
comment "the next step is ..." induced me to make callback timer_cleanup.
> Oleg.
>
>    

Re: [PATCH 1/2] posix_timer: separate timer_cleanup from timer_del

[Oleg Nesterov]

(add Stanislaw)

On 05/14, Andrey Vagin wrote:
>
> Signed-off-by: Andrey Vagin <avagin@openvz.org>

You forgot to write the changelog ;)



> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -455,14 +455,21 @@ int posix_cpu_timer_del(struct k_itimer *timer)
>  			spin_unlock(&p->sighand->siglock);
>  		}
>  		read_unlock(&tasklist_lock);
> -
> -		if (!ret)
> -			put_task_struct(p);
>  	}
>  
> +	if (!ret)
> +		posix_cpu_timer_cleanup(timer);
> +
>  	return ret;
>  }

Could you please explain this change?

Oleg.

Re: [stable] [PATCH 1/2] posix_timer: separate timer_cleanup from timer_del

[Greg KH]

On Fri, May 14, 2010 at 05:28:50PM +0400, Andrey Vagin wrote:
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
>  include/linux/posix-timers.h |    2 ++
>  kernel/posix-cpu-timers.c    |   13 ++++++++++---
>  kernel/posix-timers.c        |    4 ++++

I have no idea why this is for the -stable kernels, do you?

Please, just put a:
	Cc: stable <stable@kernel.org>
in the signed-off-by area of the patch, so that way I will be notified
when it goes into Linus's tree, not before, which just wastes my time...

thanks,

greg k-h