linux-kernel - Re: [PATCH] sched: rt_rq runtime leakage bug fix

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, 11 May 2011 11:21:51 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Cheng Xu <chengxu@...ux.vnet.ibm.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Paul Mckenney <paulmck@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sched: rt_rq runtime leakage bug fix

On Wed, 2011-05-11 at 15:34 +0800, Cheng Xu wrote:
> This patch is to fix bug report https://lkml.org/lkml/2011/4/26/13

This really doesn't tell me anything, please restate the relevant
information.

> Function __disable_runtime() reports leakage of rt_rq runtime. The
> root cause is __disable_runtime() assumes it iterates through all the
> existing rt_rq's while walking rq->leaf_rt_rq_list, which actually
> contains only runnable rt_rq's. This problem also applies to
> __enable_runtime() and print_rt_stats().

Teach your mailer to wrap at 78 characters for changelogs.

> The patch is based on above analysis, appears to fix the problem, but is only lightly tested.
> 
> 
> Signed-off-by: Cheng Xu <chengxu@...ux.vnet.ibm.com>
> Tested-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> 

Don't leave whitespace between the tags and the tripple-dash. Also, I'm
suspecting you're missing a Reported-by: paulmck tag.

> ---
>  kernel/sched_rt.c |   31 ++++++++++++++++++++++++-------
>  1 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index e7cebdc..7f478ff 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -183,6 +183,13 @@ static inline u64 sched_rt_period(struct rt_rq *rt_rq)
>  	return ktime_to_ns(rt_rq->tg->rt_bandwidth.rt_period);
>  }
>  
> +#define rt_rq_of_rq_decls(name) struct task_group *name
> +
> +#define list_for_rt_rq_of_rq(iterator, rq) \
> +	list_for_each_entry_rcu(iterator, &task_groups, list)
> +
> +#define rt_rq_of_rq_deref(iterator, rq) (iterator->rt_rq[cpu_of(rq)])
> +
>  static inline void list_add_leaf_rt_rq(struct rt_rq *rt_rq)
>  {
>  	list_add_rcu(&rt_rq->leaf_rt_rq_list,
> @@ -288,6 +295,13 @@ static inline u64 sched_rt_period(struct rt_rq *rt_rq)
>  	return ktime_to_ns(def_rt_bandwidth.rt_period);
>  }
>  
> +#define rt_rq_of_rq_decls(name) struct rt_rq *name
> +
> +#define list_for_rt_rq_of_rq(iterator, rq) \
> +	for (iterator = &rq->rt; iterator; iterator = NULL)
> +
> +#define rt_rq_of_rq_deref(iterator, rq) (iterator)
> +
>  static inline void list_add_leaf_rt_rq(struct rt_rq *rt_rq)
>  {
>  }

So I see why you did that, I just don't much like it.. esp the decls
macros, C has typedef to deal with that problem, also you can get rid of
the deref macros (now if we were allowed C99 we could avoid the whole
iter thing and declare a for-scope variable).

How about something like:

typedef struct task_group *rt_rq_iter_t;

#define for_each_rt_rq(rt_rq, iter, rq)                                     \
	for (iter = list_entry_rcu(task_groups.next, typeof(*iter), list),  \
	     rt_rq = iter->rt_rq[cpu_of(rq)]; &iter->list != &task_groups;  \
	     iter = list_entry_rcu(iter->list.next, typeof(*iter), list),   \
	     rt_rq = iter->rt_rq[cpu_of(rq)])

	    
which is then used like:

	rt_rq_iter_t iter;
	struct rt_rq *rt_rq;

	for_each_rt_rq(rt_rq, iter, rq) {
		/* do something with rt_rq */
	}


--
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/