linux-kernel - Re: [PATCH] Add support for deferrable timers (respun)

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]

Message-ID: <20070327222227.GA279@tv-sign.ru>
Date:	Wed, 28 Mar 2007 02:22:27 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Venki Pallipadi <venkatesh.pallipadi@...el.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	akpm@...ux-foundation.org, davej@...emonkey.org.uk,
	johnstul@...ibm.com, mingo@...e.hu, tglx@...utronix.de
Subject: Re: [PATCH] Add support for deferrable timers (respun)

On 03/27, Venki Pallipadi wrote:
>
> Incremental patch below eliminates this race.
>
> Index: new/kernel/timer.c
> ===================================================================
> --- new.orig/kernel/timer.c	2007-03-26 15:19:35.000000000 -0800
> +++ new/kernel/timer.c	2007-03-27 13:00:33.000000000 -0800
> @@ -96,9 +96,9 @@
>  	return tbase_get_deferrable(timer->base);
>  }
>  
> -static inline struct tvec_t_base_s *timer_get_base(struct timer_list *timer)
> +static inline struct tvec_t_base_s *tbase_get_base(struct tvec_t_base_s *base)
>  {
> -	return ((struct tvec_t_base_s *)((unsigned long)(timer->base) &
> +	return ((struct tvec_t_base_s *)((unsigned long)base &
>  	                                 ~TBASE_DEFERRABLE_FLAG));
>  }
>  
> @@ -368,7 +368,7 @@
>  
>  	for (;;) {
>  		tvec_base_t *prelock_base = timer->base;
> -		base = timer_get_base(timer);
> +		base = tbase_get_base(prelock_base);
>  		if (likely(base != NULL)) {
>  			spin_lock_irqsave(&base->lock, *flags);
>  			if (likely(prelock_base == timer->base))

Looks correct to me... Personally, I'd prefer

	static tvec_base_t *lock_timer_base(struct timer_list *timer,
						unsigned long *flags)
		__acquires(timer->base->lock)
	{
		tvec_base_t *base;

		for (;;) {
			base = timer_get_base(timer);
			if (likely(base != NULL)) {
				spin_lock_irqsave(&base->lock, *flags);
				if (likely(base == timer_get_base(timer))
					return base;
				/* The timer has migrated to another CPU */
				spin_unlock_irqrestore(&base->lock, *flags);
			}
			cpu_relax();
		}
	}

but this is a matter of taste.

A minor nitpick,

> +/* new_base is guaranteed to have last bit not set, in all callers below */
> +static inline void timer_set_base(struct timer_list *timer,
> +                                       struct tvec_t_base_s *old_base,
> +                                       struct tvec_t_base_s *new_base)
> +{
> +       timer->base = (struct tvec_t_base_s *)((unsigned long)(new_base) |
> +                                              tbase_get_deferrable(old_base));
> +}

looks a little bit ugly, but may be this is just me. How about

	void timer_set_base(struct timer_list *timer, struct tvec_t_base_s *new_base)
	{
		timer->base = (struct tvec_t_base_s *)
			((unsigned long)(new_base) | tbase_get_deferrable(timer->base));
	}

__mod_timer:
	-	tvec_base_t *old_base = timer->base;
	-	timer->base = NULL;
	+	timer_set_base(timer, NULL);

?

> +                       /* Make sure that tvec_base is 2 byte aligned */
> +                       if (tbase_get_deferrable(base)) {
> +                               WARN_ON(1);
> +                               kfree(base);
> +                               return -ENOMEM;
> +                       }

Not a comment, but a question: do we really need this?

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/