| Message ID | 1467762904-4241-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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]; };
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(-)