What does this sparse warning mean in posix_acl.h?

It apparently has something to do with rcu and "address spaces" but I'm not completely sure what sparse is complaining about --- and whether it is a false positive or a bug in the posix_acl.h.

Can someone explain what's going on, and whether it's something to be concerned about?

Thanks,

- Ted

% ../make-ext4 C=1 fs/ext4/acl.o
make[2]: Nothing to be done for `all'.
GEN /u1/ext4/Makefile
CHK include/generated/uapi/linux/version.h
make[2]: Nothing to be done for `relocs'.
Using /usr/projects/linux/ext4 as source for kernel
CHK include/generated/utsrelease.h
CALL /usr/projects/linux/ext4/scripts/checksyscalls.sh
<stdin>:1220:2: warning: #warning syscall kcmp not implemented [-Wcpp]
<stdin>:1223:2: warning: #warning syscall finit_module not implemented [-Wcpp]
CHECK /usr/projects/linux/ext4/fs/ext4/acl.c
/usr/projects/linux/ext4/include/linux/posix_acl.h:137:9: warning: incorrect type in assignment (different address spaces)
/usr/projects/linux/ext4/include/linux/posix_acl.h:137:9: expected struct posix_acl *<noident>
/usr/projects/linux/ext4/include/linux/posix_acl.h:137:9: got struct posix_acl [noderef] <asn:4>*<noident>
/usr/projects/linux/ext4/include/linux/posix_acl.h:137:9: warning: incorrect type in assignment (different address spaces)
/usr/projects/linux/ext4/include/linux/posix_acl.h:137:9: expected struct posix_acl *<noident>
/usr/projects/linux/ext4/include/linux/posix_acl.h:137:9: got struct posix_acl [noderef] <asn:4>*<noident>

So rcu_assign_pointer() wants the destination to be a pointer to something in the "__rcu" address space, but here "p" is just a normal kernel pointer (it's a pointer to a regular "struct posix_acl *").

(The unreadable "asn:4" comes from the fact that sparse only has a generic notion of address spaces, where address space zero is the "normal" C pointer address space, and then you can just make up your own ones by number. It was initially done for just "user space", but designed so that it could be extended to other things, and "rcu space" was another later use for checking certain pointer semantics).

I guess we should mark i_acl and i_defauly_acl to be RCU-accessed, and then annotate all the accesses properly.

The attached patch does so, but I only compile-tested that one file (fs/ext4/acl.c), so it will quite possible cause tons of new warnings elsewhere. But I'm hoping the patch at least illustrates the concept.

Linus

 include/linux/fs.h        |  4 ++--
 include/linux/posix_acl.h | 18 ++++++++++--------
 2 files changed, 12 insertions(+), 10 deletions(-)

index 981874773e85..883c07068485 100644
@@ -528,8 +528,8 @@ struct inode {
 	unsigned int		i_flags;
 
 #ifdef CONFIG_FS_POSIX_ACL
-	struct posix_acl	*i_acl;
-	struct posix_acl	*i_default_acl;
+	struct posix_acl __rcu	*i_acl;
+	struct posix_acl __rcu	*i_default_acl;
 #endif
 
 	const struct inode_operations	*i_op;

index 7931efe71175..c3c46a37dd64 100644
@@ -95,7 +95,7 @@ extern struct posix_acl *get_posix_acl(struct inode *, int);
 extern int set_posix_acl(struct inode *, int, struct posix_acl *);
 
 #ifdef CONFIG_FS_POSIX_ACL
-static inline struct posix_acl **acl_by_type(struct inode *inode, int type)
+static inline struct posix_acl __rcu **acl_by_type(struct inode *inode, int type)
 {
 	switch (type) {
 	case ACL_TYPE_ACCESS:

@@ -109,11 +109,11 @@ static inline struct posix_acl **acl_by_type(struct inode *inode, int type)
 
 static inline struct posix_acl *get_cached_acl(struct inode *inode, int type)
 {
-	struct posix_acl **p = acl_by_type(inode, type);
-	struct posix_acl *acl = ACCESS_ONCE(*p);
+	struct posix_acl __rcu **p = acl_by_type(inode, type);
+	struct posix_acl *acl = rcu_dereference(*p);
 	if (acl) {
 		spin_lock(&inode->i_lock);
-		acl = *p;
+		acl = rcu_dereference(*p);
 		if (acl != ACL_NOT_CACHED)
 			acl = posix_acl_dup(acl);
 		spin_unlock(&inode->i_lock);

@@ -130,11 +130,13 @@ static inline void set_cached_acl(struct inode *inode,
 				  int type,
 				  struct posix_acl *acl)
 {
-	struct posix_acl **p = acl_by_type(inode, type);
-	struct posix_acl *old;
+	struct posix_acl __rcu **p = acl_by_type(inode, type);
+	struct posix_acl *old, *new;
+
+	new = posix_acl_dup(acl);
 	spin_lock(&inode->i_lock);
-	old = *p;
-	rcu_assign_pointer(*p, posix_acl_dup(acl));
+	old = rcu_dereference_protected(*p, true);
+	rcu_assign_pointer(*p, new);
 	spin_unlock(&inode->i_lock);
 	if (old != ACL_NOT_CACHED)
 		posix_acl_release(old);

I may be missing something, but it looks like the ACL code isn't following the RCU rules at _all_. Even with the missing rcu_derference() macro invocations which you added in your proof-of-concept patch, we're still missing the rcu_read_lock() calls around the use of the rcu pointers.

If so, I'm kind of wondering why we haven't noticed massive problems here before.

- Ted

Yeah, the ACL caches aren't really using RCU, although they should probably be made to use it more widely.

The ACL cache is this mixture of an unlocked "ACCESS_ONCE()" to check for the very special case of NULL (which is a "negative ACL cache" - no ACL's at all), and an atomic reference count that is gotten under i_lock. See get_cached_acl() for details.

The "unlocked ACCESS_ONCE()" case is basically an optimization for the "no ACL's case" - it isn't actually racy despite the lack of locking, because if you see a NULL ACL pointer, that *was* true at some time, so if it "races" with somebody setting it to anything else, you might as well just choose to pick that simpler state.

So anybody using "get_cached_acl()" is safe. It gets either a locked ref-counted ACL pointer, or an unlocked optimistic NULL pointer.

There is *one* special case that is actually truly using RCU, namely "get_cached_acl_rcu()". And that one *is* used under the RCU read lock (it's the RCU lock for lookup). That is all safe because the ACL's are free'd by kfree_rcu() (see posix_acl_release()).

So the ACL accesses are this somewhat strange mix of RCU and non-RCU use. We probably could make *more* of them use the RCU model, but apart from the RCU pathname lookup nothing else has ever been critical enough to care.

Linus

Ah, I see. But as we start adding the RCU annotations and the calls to rcu_derference(), then I imagine we'll start triggering warnings from various other static analysis tools, even if it makes sparse happy.

Ugh. OK, I care mostly because it's adding noise to my make C=1 runs, but I'm not sure I care enough to fix it up right away. I'll put it on my todo list, and hope someone beats me to it. :-)

- Ted

Yes. I suspect the patch I sent out might need some tweaking to make the dynamic RCU accessor checkers happy.

That said, it's probably fairly simple. I think my patch was *almost* correct, and the only required change would be to change the two occurrences of

    rcu_dereference(*p);

in "get_cached_acl()" into a "rcu_dereference_protected(*p, true)" in order to avoid the RCU read-lock sanity checking.

The second access is actually under the i_lock (so the "protected" part is clearly correct), and as mentioned, the first one is special and we only use the value if it is NULL, so the "protected" is not technically true, but it's a special case, and together with a comment about why NULL is fine and doesn't need rcu locking it should all be good.

The alternative is to basically say that the ACL pointers aren't really RCU at all, and just use ACCESS_ONCE() like we do. And then just cast things in get_cached_acl_rcu() to shut things up.

Linus

Side note: the whole concept of mixing locking *and* RCU is a bit odd, but it has turned out to be a very successful model for pathname lookup. Doing everything under RCU is basically impossible, so having the ability to looking things up under RCU for the simple case, but then falling back on locking for complex cases is very very powerful.

So arguably that "somewhat strange mix of RCU and non-RCU use" shouldn't really be strange. It's just that it is complicated enough that we only use it for pathnames right now. Everything else tends to be more black-and-white and do one or the other, not both.

Linus

[...]

Address space 4 is __rcu. This warning means you accessed an RCU pointer directly, without using the appropriate RCU primitive (rcu_assign_pointer, rcu_dereference, etc).

- Josh Triplett

Well, actually, in this case it was the other way around: using an RCU primitive (rcu_assign_pointer) without having marked the variable for RCU.

Linus