sched: rt_rq runtime leakage bug fix

This patch is to fix bug report https://lkml.org/lkml/2011/4/26/13

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().

The patch is based on above analysis, appears to fix the problem, but is only lightly tested.

Signed-off-by Cheng Xu <[email protected]>

Tested-by Paul E. McKenney <[email protected]>

---
 kernel/sched_rt.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

kernel/sched_rt.c +7 -0

index e7cebdc..7f478ff 100644
@@ -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);
 }
 

Quoted 1 line · Cheng Xu

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.

Quoted 5 lines

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.

Quoted 6 lines

The patch is based on above analysis, appears to fix the problem, but is only lightly tested.

Signed-off-by Cheng Xu <[email protected]>

Tested-by Paul E. McKenney <[email protected]>

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

Quoted 26 lines

---
 kernel/sched_rt.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

kernel/sched_rt.c +7 -0

index e7cebdc..7f478ff 100644
@@ -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);
 }
 

kernel/sched_rt.c static inline u64 sched_rt_period(struct rt_rq *rt_rq) · line 307

Quoted 10 lines

kernel/sched_rt.c +7 -0

+#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 */
	}

I tried but hit a boot-time error "Unable to handle kernel paging request for data at address 0x100000008", and therefore would like to propose an alternative patch like,

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

This worked, it seems to pass the tests. Is this correct from a scheduler perspective?

For the not CONFIG_RT_GROUP_SCHED part, I used

typedef struct rt_rq *rt_rq_iter_t;

#define for_each_rt_rq(rt_rq, iter, rq) \
        (void) iter; \
        for (rt_rq = &rq->rt; rt_rq; rt_rq = NULL)

An alternative is

#define for_each_rt_rq(rt_rq, iter, rq) \
        for (rt_rq = iter = &rq->rt; iter; rt_rq = iter = NULL)

The patch is attached below. Could you check whether it is workable? Thank you.

Quoted 5 lines · Cheng Xu

I tried but hit a boot-time error "Unable to handle kernel paging request for data at address 0x100000008", and therefore would like to propose an alternative patch like,

I probably made a silly mistake somehwere, it was after all something quickly typed in an email :-)

Quoted 5 lines

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

Quoted 1 line

This worked, it seems to pass the tests. Is this correct from a scheduler perspective?

Creative ;-), it would be nice to know why the , operator version doesn't work though, since that looks to be the more conventional way to write it.

That said, I don't see a problem with using your existing on.

Quoted 11 lines

For the not CONFIG_RT_GROUP_SCHED part, I used

typedef struct rt_rq *rt_rq_iter_t;

#define for_each_rt_rq(rt_rq, iter, rq) \
        (void) iter; \
        for (rt_rq = &rq->rt; rt_rq; rt_rq = NULL)

An alternative is

#define for_each_rt_rq(rt_rq, iter, rq) \
        for (rt_rq = iter = &rq->rt; iter; rt_rq = iter = NULL)

Tough call that, the first has a multi-statement macro, which is generally discouraged because then:

  for()
   for_each_rt_rq() {
   }

will not work as expected, so I think we want the second version.

Quoted 2 lines

The patch is attached below. Could you check whether it is workable? Thank you.

Yes, given how things are I can't really see it getting any better, thanks!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#define for_each_rt_rq(rt_rq, iter, rq) \
        for ((void) iter, rt_rq = &rq->rt; rt_rq; rt_rq = NULL)

maybe we can still use it?

it would be nice to know why the , operator version
doesn't work though, since that looks to be the more conventional way to write it.