Skip to contentSkip to conversation

fix rcu annotations noise in cred.h

PATCH Al Viro 3 participants 7 messages Aug 7, 2011 – Aug 8, 2011 Credentials

lore
Share & tools
lore

Al Viro

author
#0 lore raw

task->cred is declared as __rcu, and access to other tasks' ->cred is, indeed, protected. Access to current->cred does not need rcu_dereference() at all, since only the task itself can change its ->cred. sparse, of course, has no way of knowing that...

Add force-cast in current_cred(), make current_fsuid() et.al. use it.

Signed-off-by Al Viro <[email protected]>

---

include/linux/cred.h +6 -5
index 48e82af..98f46ef 100644
@@ -265,10 +265,11 @@ static inline void put_cred(const struct cred *_cred)
 /**
  * current_cred - Access the current task's subjective credentials
  *
- * Access the subjective credentials of the current task.
+ * Access the subjective credentials of the current task.  RCU-safe,
+ * since nobody else can modify it.
  */
 #define current_cred() \
-	(current->cred)
+	(*(__force struct cred **)&current->cred)
 
 /**
  * __task_cred - Access a task's objective credentials

@@ -307,7 +308,7 @@ static inline void put_cred(const struct cred *_cred)
 ({							\
 	struct user_struct *__u;			\
 	struct cred *__cred;				\
-	__cred = (struct cred *) current_cred();	\
+	__cred = current_cred();			\
 	__u = get_uid(__cred->user);			\
 	__u;						\
 })

@@ -322,7 +323,7 @@ static inline void put_cred(const struct cred *_cred)
 ({							\
 	struct group_info *__groups;			\
 	struct cred *__cred;				\
-	__cred = (struct cred *) current_cred();	\
+	__cred = current_cred();			\
 	__groups = get_group_info(__cred->group_info);	\
 	__groups;					\
 })

@@ -341,7 +342,7 @@ static inline void put_cred(const struct cred *_cred)
 
 #define current_cred_xxx(xxx)			\
 ({						\
-	current->cred->xxx;			\
+	current_cred()->xxx;			\
 })
 
 #define current_uid()		(current_cred_xxx(uid))
#1 lore raw

include/linux/cred.h static inline void put_cred(const struct cred *_cred) · line 272

Quoted 3 lines · Al Viro

#define current_cred() \

include/linux/cred.h +1 -1
-	(current->cred)
+	(*(__force struct cred **)&current->cred)

No. You've cast away the const. Please don't do that.

Paul: isn't there some better way of doing this using an RCU wrapper macro?

	#define current_cred() \
		rcu_dereference_protected(current->cred, 1)

Perhaps?

David

author
#2 lore raw
Quoted 8 lines · David Howells

include/linux/cred.h static inline void put_cred(const struct cred *_cred) · line 272

Quoted 3 lines · Al Viro

#define current_cred() \

include/linux/cred.h +1 -1
-	(current->cred)
+	(*(__force struct cred **)&current->cred)

No. You've cast away the const. Please don't do that.

So let's bring const back, not a problem...

#3 lore raw
In reply to David Howells
Quoted 9 lines · David Howells

include/linux/cred.h static inline void put_cred(const struct cred *_cred) · line 272

Quoted 3 lines · Al Viro

#define current_cred() \

include/linux/cred.h +1 -1
-	(current->cred)
+	(*(__force struct cred **)&current->cred)

No. You've cast away the const. Please don't do that.

Quoted 6 lines

Paul: isn't there some better way of doing this using an RCU wrapper macro?

	#define current_cred() \
		rcu_dereference_protected(current->cred, 1)

Perhaps?

This is indeed what rcu_dereference_protected() is intended for.

Thanx, Paul

#4 lore raw
Quoted 1 line · Paul E. McKenney

This is indeed what rcu_dereference_protected() is intended for.

How about the attached patch?

David
---
From: David Howells <[email protected]>
Subject: [PATCH] CRED: Restore const to current_cred()

Commit 3295514841c2112d94451ba5deaf54f5afb78ea9 accidentally dropped the const of current->cred inside current_cred() by the insertion of a cast to deal with an RCU annotation loss warning from sparce.

Use an appropriate RCU wrapper instead so as not to lose the const.

Signed-off-by David Howells <[email protected]>
cc Al Viro <[email protected]>

---

 include/linux/cred.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/cred.h +1 -1
index 98f46ef..8e2fd44 100644
@@ -269,7 +269,7 @@ static inline void put_cred(const struct cred *_cred)
  * since nobody else can modify it.
  */
 #define current_cred() \
-	(*(__force struct cred **)&current->cred)
+	rcu_dereference_protected(current->cred, 1)
 
 /**
  * __task_cred - Access a task's objective credentials
Reviewed-by
#5 lore raw
Quoted 11 lines · David Howells
Quoted 1 line · Paul E. McKenney

This is indeed what rcu_dereference_protected() is intended for.

How about the attached patch?

David
---
From: David Howells <[email protected]>
Subject: [PATCH] CRED: Restore const to current_cred()

Quoted 5 lines

Commit 3295514841c2112d94451ba5deaf54f5afb78ea9 accidentally dropped the const of current->cred inside current_cred() by the insertion of a cast to deal with an RCU annotation loss warning from sparce.

Use an appropriate RCU wrapper instead so as not to lose the const.

Reviewed-by Paul E. McKenney <[email protected]>
Quoted 21 lines
Signed-off-by David Howells <[email protected]>
cc Al Viro <[email protected]>

---

 include/linux/cred.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/linux/cred.h +1 -1
index 98f46ef..8e2fd44 100644
@@ -269,7 +269,7 @@ static inline void put_cred(const struct cred *_cred)
  * since nobody else can modify it.
  */
 #define current_cred() \
-	(*(__force struct cred **)&current->cred)
+	rcu_dereference_protected(current->cred, 1)
 
 /**
  * __task_cred - Access a task's objective credentials
#6 lore raw
In reply to Al Viro
Quoted 4 lines · Al Viro

task->cred is declared as __rcu, and access to other tasks' ->cred is, indeed, protected. Access to current->cred does not need rcu_dereference() at all, since only the task itself can change its ->cred. sparse, of course, has no way of knowing that...

What was the Sparse warning, btw? You didn't say. I don't see any warnings, but it's possible I have a too-old version of Sparse or the wrong config options? (Or maybe I just don't know what to look for)

David