sched: static_key sched_feat race + cleanup

Hi,

Andi Kleen previously pointed out a get/set race in the sched_feat() infrastructure. I had previously proposed to address this with a new static_key set_true/set_false API:

http://www.gossamer-threads.com/lists/linux/kernel/1955879

However, I think its simpler to just add higher level locking as Ingo previously suggested, and that is how other consumers of the this interface are already operating. It seems a bit like overkill to add this new API, just for this one case.

So, this locking change is in patch 2. Patch 1 is a small cleanup, but no functional change.

Thanks,

-Jason

Jason Baron (2):
sched: remove extra static_key function indirection
sched: fix static_key race with sched_feat

 kernel/sched/core.c  |  5 +++++
 kernel/sched/sched.h | 12 +-----------
 2 files changed, 6 insertions(+), 11 deletions(-)

1.8.2.rc2

I think its a bit simpler without having to follow an extra layer of static inline fuctions. No functional change just cosmetic.

---
 kernel/sched/sched.h | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

index 31cc02e..10494c0 100644
@@ -884,20 +884,10 @@ enum {
 #undef SCHED_FEAT
 
 #if defined(CONFIG_SCHED_DEBUG) && defined(HAVE_JUMP_LABEL)
-static __always_inline bool static_branch__true(struct static_key *key)
-{
-	return static_key_true(key); /* Not out of line branch. */
-}
-
-static __always_inline bool static_branch__false(struct static_key *key)
-{
-	return static_key_false(key); /* Out of line branch. */
-}
-
 #define SCHED_FEAT(name, enabled)					\
 static __always_inline bool static_branch_##name(struct static_key *key) \
 {									\
-	return static_branch__##enabled(key);				\
+	return static_key_##enabled(key);				\
 }
 
 #include "features.h"

1.8.2.rc2

As pointed out by Andi Kleen, the usage of static keys can be racy in sched_feat_disable() vs. sched_feat_enable(). Currently, we first check the value of keys->enabled, and subsequently update the branch direction. This, can be racy and can potentially leave the keys in an inconsistent state.

Take the i_mutex around these calls to resolve the race.

---
 kernel/sched/core.c | 5 +++++
 1 file changed, 5 insertions(+)

index 3bdf01b..249c865 100644
@@ -243,6 +243,7 @@ sched_feat_write(struct file *filp, const char __user *ubuf,
 	char buf[64];
 	char *cmp;
 	int i;
+	struct inode *inode;
 
 	if (cnt > 63)
 		cnt = 63;

@@ -253,7 +254,11 @@ sched_feat_write(struct file *filp, const char __user *ubuf,
 	buf[cnt] = 0;
 	cmp = strstrip(buf);
 
+	/* Ensure the static_key remains in a consistent state */
+	inode = file_inode(filp);
+	mutex_lock(&inode->i_mutex);
 	i = sched_feat_set(cmp);
+	mutex_unlock(&inode->i_mutex);
 	if (i == __SCHED_FEAT_NR)
 		return -EINVAL;
 

1.8.2.rc2