posix_acl: de-union a_refcount and a_rcu

Commit Message

Andreas Gruenbacher July 11, 2016, 3:01 p.m. UTC

From: Jeff Layton <[e-mail]>

Currently the two are unioned together, but I don't think that's safe.

It looks like get_cached_acl could race with the last put in
posix_acl_release. get_cached_acl calls atomic_inc_not_zero on
a_refcount, but that field could have already been clobbered by
call_rcu, and may no longer be zero. Fix this by de-unioning the two
fields.

Fixes: b8a7a3a66747 (v4.7-rc1, posix_acl: Inode acl caching fixes)
Signed-off-by: Jeff Layton <[e-mail]>
Acked-by: David Howells <[e-mail]>
Signed-off-by: Andreas Gruenbacher <[e-mail]>
---
 include/linux/posix_acl.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Linus Torvalds July 11, 2016, 4:50 p.m. UTC | #1

On Mon, Jul 11, 2016 at 8:01 AM, Andreas Gruenbacher
<[e-mail]> wrote:
> From: Jeff Layton <[e-mail]>
>
> Currently the two are unioned together, but I don't think that's safe.

Ack. That does look fishy. Almost anything else can probably be
unioned together with the rcu list, but *not* the refcount that we
might want to look at during the rcu grace period.

Al, this is your code (from long long ago). Want to take it through
the vfs tree, or should I just apply direectly? I'd like to have your
ack regardless, but the fix looks ObviouslyCorrect(tm) to me.

           Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton July 11, 2016, 5:08 p.m. UTC | #2

On Mon, 2016-07-11 at 09:50 -0700, Linus Torvalds wrote:
> On Mon, Jul 11, 2016 at 8:01 AM, Andreas Gruenbacher
> <[e-mail]> wrote:
> > From: Jeff Layton <[e-mail]>
> > 
> > Currently the two are unioned together, but I don't think that's
> > safe.
> 
> Ack. That does look fishy. Almost anything else can probably be
> unioned together with the rcu list, but *not* the refcount that we
> might want to look at during the rcu grace period.
> 

I looked at unioning the rcu_head it with something else, but there's
nothing else that doesn't get peeked at in an rcu-critical section. So
I think this is the only real fix, unfortunately.

> Al, this is your code (from long long ago). Want to take it through
> the vfs tree, or should I just apply direectly? I'd like to have your
> ack regardless, but the fix looks ObviouslyCorrect(tm) to me.
> 

I think this is a v4.7 regression. We've freed these things via RCU for
a long time, but monkeying with the refcount under the rcu_read_lock is
new with the patch in the "Fixes:" line, AFAICT.

Al Viro July 11, 2016, 5:49 p.m. UTC | #3

On Mon, Jul 11, 2016 at 09:50:21AM -0700, Linus Torvalds wrote:
> On Mon, Jul 11, 2016 at 8:01 AM, Andreas Gruenbacher
> <[e-mail]> wrote:
> > From: Jeff Layton <[e-mail]>
> >
> > Currently the two are unioned together, but I don't think that's safe.
> 
> Ack. That does look fishy. Almost anything else can probably be
> unioned together with the rcu list, but *not* the refcount that we
> might want to look at during the rcu grace period.
> 
> Al, this is your code (from long long ago). Want to take it through
> the vfs tree, or should I just apply direectly? I'd like to have your
> ack regardless, but the fix looks ObviouslyCorrect(tm) to me.

Applied; the mess had been caused by get_cached_acl() switch from ->i_lock
to rcu_read_lock.  The code is mine, but relevant part is fairly recent.

I'll send a pull request tonight, with that thing included.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 5b5a80c..c818772 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -43,10 +43,8 @@  struct posix_acl_entry {
 };
 
 struct posix_acl {
-	union {
-		atomic_t		a_refcount;
-		struct rcu_head		a_rcu;
-	};
+	atomic_t		a_refcount;
+	struct rcu_head		a_rcu;
 	unsigned int		a_count;
 	struct posix_acl_entry	a_entries[0];
 };