[PATCH] posix_acl: de-union a_refcount and a_rcu

[PATCH] posix_acl: de-union a_refcount and a_rcu

[Andreas Gruenbacher]

From: Jeff Layton <jlayton@redhat.com>

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 <jlayton@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 include/linux/posix_acl.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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];
 };
-- 
2.5.5

Re: [PATCH] posix_acl: de-union a_refcount and a_rcu

[Linus Torvalds]

On Mon, Jul 11, 2016 at 8:01 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> From: Jeff Layton <jlayton@redhat.com>
>
> 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

Re: [PATCH] posix_acl: de-union a_refcount and a_rcu

[Jeff Layton]

On Mon, 2016-07-11 at 09:50 -0700, Linus Torvalds wrote:
> On Mon, Jul 11, 2016 at 8:01 AM, Andreas Gruenbacher
> <agruenba@redhat.com> wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > 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.
-- 

Jeff Layton <jlayton@redhat.com>

Re: [PATCH] posix_acl: de-union a_refcount and a_rcu

[Al Viro]

On Mon, Jul 11, 2016 at 09:50:21AM -0700, Linus Torvalds wrote:
> On Mon, Jul 11, 2016 at 8:01 AM, Andreas Gruenbacher
> <agruenba@redhat.com> wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> >
> > 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.

Re: [PATCH] posix_acl: de-union a_refcount and a_rcu

[Jeff Layton]

On Thu, 2016-07-07 at 15:11 +0100, David Howells wrote:
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > 
> > 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 (posix_acl: Inode acl caching fixes)
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Acked-by: David Howells <dhowells@redhat.com>

Thanks David,

I think we're spared in most cases, as long as the atomic_inc_not_zero
occurs while the next pointer in the rcu_head is still NULL. If it's
not though, then we're set up for a GPF and/or a use-after-free.

AFAICT, this is a regression from v4.6, so I think we want this in
v4.7. Al, do you mind picking this up? Or NAK it and propose an
alternate fix?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] posix_acl: de-union a_refcount and a_rcu

[David Howells]

Jeff Layton <jlayton@redhat.com> wrote:

> 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 (posix_acl: Inode acl caching fixes)
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Acked-by: David Howells <dhowells@redhat.com>

[PATCH] posix_acl: de-union a_refcount and a_rcu

[Jeff Layton]

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 (posix_acl: Inode acl caching fixes)
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/posix_acl.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 5b5a80cc5926..c818772d9f9d 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];
 };
-- 
2.5.5