[PATCH 0/2] fs, ceph filesystem refcount conversions

[PATCH 0/2] fs, ceph filesystem refcount conversions

[Elena Reshetova]

Now when new refcount_t type and API are finally merged
(see include/linux/refcount.h), the following
patches convert various refcounters in the ceph filesystem from atomic_t
to refcount_t. By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.

The below patches are fully independent and can be cherry-picked separately.
Since we convert all kernel subsystems in the same fashion, resulting
in about 300 patches, we have to group them for sending at least in some
fashion to be manageable. Please excuse the long cc list.

Not runtime tested, since I am not sure how to do it for ceph.
However changes are pretty trivial in this case.

Elena Reshetova (2):
  fs, ceph: convert ceph_mds_session.s_ref from atomic_t to refcount_t
  fs, ceph: convert ceph_cap_snap.nref from atomic_t to refcount_t

 fs/ceph/caps.c       |  4 ++--
 fs/ceph/mds_client.c | 18 +++++++++---------
 fs/ceph/mds_client.h |  5 +++--
 fs/ceph/snap.c       |  2 +-
 fs/ceph/super.h      |  5 +++--
 5 files changed, 18 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH 1/2] fs, ceph: convert ceph_mds_session.s_ref from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/ceph/mds_client.c | 18 +++++++++---------
 fs/ceph/mds_client.h |  5 +++--
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index c681762..0744905 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -378,9 +378,9 @@ const char *ceph_session_state_name(int s)
 
 static struct ceph_mds_session *get_session(struct ceph_mds_session *s)
 {
-	if (atomic_inc_not_zero(&s->s_ref)) {
+	if (refcount_inc_not_zero(&s->s_ref)) {
 		dout("mdsc get_session %p %d -> %d\n", s,
-		     atomic_read(&s->s_ref)-1, atomic_read(&s->s_ref));
+		     refcount_read(&s->s_ref)-1, refcount_read(&s->s_ref));
 		return s;
 	} else {
 		dout("mdsc get_session %p 0 -- FAIL", s);
@@ -391,8 +391,8 @@ static struct ceph_mds_session *get_session(struct ceph_mds_session *s)
 void ceph_put_mds_session(struct ceph_mds_session *s)
 {
 	dout("mdsc put_session %p %d -> %d\n", s,
-	     atomic_read(&s->s_ref), atomic_read(&s->s_ref)-1);
-	if (atomic_dec_and_test(&s->s_ref)) {
+	     refcount_read(&s->s_ref), refcount_read(&s->s_ref)-1);
+	if (refcount_dec_and_test(&s->s_ref)) {
 		if (s->s_auth.authorizer)
 			ceph_auth_destroy_authorizer(s->s_auth.authorizer);
 		kfree(s);
@@ -411,7 +411,7 @@ struct ceph_mds_session *__ceph_lookup_mds_session(struct ceph_mds_client *mdsc,
 		return NULL;
 	session = mdsc->sessions[mds];
 	dout("lookup_mds_session %p %d\n", session,
-	     atomic_read(&session->s_ref));
+	     refcount_read(&session->s_ref));
 	get_session(session);
 	return session;
 }
@@ -466,7 +466,7 @@ static struct ceph_mds_session *register_session(struct ceph_mds_client *mdsc,
 	INIT_LIST_HEAD(&s->s_caps);
 	s->s_nr_caps = 0;
 	s->s_trim_caps = 0;
-	atomic_set(&s->s_ref, 1);
+	refcount_set(&s->s_ref, 1);
 	INIT_LIST_HEAD(&s->s_waiting);
 	INIT_LIST_HEAD(&s->s_unsafe);
 	s->s_num_cap_releases = 0;
@@ -494,7 +494,7 @@ static struct ceph_mds_session *register_session(struct ceph_mds_client *mdsc,
 	}
 	mdsc->sessions[mds] = s;
 	atomic_inc(&mdsc->num_sessions);
-	atomic_inc(&s->s_ref);  /* one ref to sessions[], one to caller */
+	refcount_inc(&s->s_ref);  /* one ref to sessions[], one to caller */
 
 	ceph_con_open(&s->s_con, CEPH_ENTITY_TYPE_MDS, mds,
 		      ceph_mdsmap_get_addr(mdsc->mdsmap, mds));
@@ -3881,7 +3881,7 @@ static struct ceph_connection *con_get(struct ceph_connection *con)
 	struct ceph_mds_session *s = con->private;
 
 	if (get_session(s)) {
-		dout("mdsc con_get %p ok (%d)\n", s, atomic_read(&s->s_ref));
+		dout("mdsc con_get %p ok (%d)\n", s, refcount_read(&s->s_ref));
 		return con;
 	}
 	dout("mdsc con_get %p FAIL\n", s);
@@ -3892,7 +3892,7 @@ static void con_put(struct ceph_connection *con)
 {
 	struct ceph_mds_session *s = con->private;
 
-	dout("mdsc con_put %p (%d)\n", s, atomic_read(&s->s_ref) - 1);
+	dout("mdsc con_put %p (%d)\n", s, refcount_read(&s->s_ref) - 1);
 	ceph_put_mds_session(s);
 }
 
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index ac0475a..bbebcd5 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -7,6 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/spinlock.h>
+#include <linux/refcount.h>
 
 #include <linux/ceph/types.h>
 #include <linux/ceph/messenger.h>
@@ -156,7 +157,7 @@ struct ceph_mds_session {
 	unsigned long     s_renew_requested; /* last time we sent a renew req */
 	u64               s_renew_seq;
 
-	atomic_t          s_ref;
+	refcount_t          s_ref;
 	struct list_head  s_waiting;  /* waiting requests */
 	struct list_head  s_unsafe;   /* unsafe requests */
 };
@@ -373,7 +374,7 @@ __ceph_lookup_mds_session(struct ceph_mds_client *, int mds);
 static inline struct ceph_mds_session *
 ceph_get_mds_session(struct ceph_mds_session *s)
 {
-	atomic_inc(&s->s_ref);
+	refcount_inc(&s->s_ref);
 	return s;
 }
 
-- 
2.7.4

[PATCH 2/2] fs, ceph: convert ceph_cap_snap.nref from atomic_t to refcount_t

[Elena Reshetova]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
 fs/ceph/caps.c  | 4 ++--
 fs/ceph/snap.c  | 2 +-
 fs/ceph/super.h | 5 +++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index cd966f2..99a77c2c 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -1389,7 +1389,7 @@ static void __ceph_flush_snaps(struct ceph_inode_info *ci,
 		first_tid = cf->tid + 1;
 
 		capsnap = container_of(cf, struct ceph_cap_snap, cap_flush);
-		atomic_inc(&capsnap->nref);
+		refcount_inc(&capsnap->nref);
 		spin_unlock(&ci->i_ceph_lock);
 
 		dout("__flush_snaps %p capsnap %p tid %llu %s\n",
@@ -2202,7 +2202,7 @@ static void __kick_flushing_caps(struct ceph_mds_client *mdsc,
 			     inode, capsnap, cf->tid,
 			     ceph_cap_string(capsnap->dirty));
 
-			atomic_inc(&capsnap->nref);
+			refcount_inc(&capsnap->nref);
 			spin_unlock(&ci->i_ceph_lock);
 
 			ret = __send_flush_snap(inode, session, capsnap, cap->mseq,
diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index 8f8b41c..dab5d67 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -519,7 +519,7 @@ void ceph_queue_cap_snap(struct ceph_inode_info *ci)
 	     capsnap->need_flush ? "" : "no_flush");
 	ihold(inode);
 
-	atomic_set(&capsnap->nref, 1);
+	refcount_set(&capsnap->nref, 1);
 	INIT_LIST_HEAD(&capsnap->ci_item);
 
 	capsnap->follows = old_snapc->seq;
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index fe6b9cf..c68e6a0 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -14,6 +14,7 @@
 #include <linux/writeback.h>
 #include <linux/slab.h>
 #include <linux/posix_acl.h>
+#include <linux/refcount.h>
 
 #include <linux/ceph/libceph.h>
 
@@ -162,7 +163,7 @@ struct ceph_cap_flush {
  * data before flushing the snapped state (tracked here) back to the MDS.
  */
 struct ceph_cap_snap {
-	atomic_t nref;
+	refcount_t nref;
 	struct list_head ci_item;
 
 	struct ceph_cap_flush cap_flush;
@@ -191,7 +192,7 @@ struct ceph_cap_snap {
 
 static inline void ceph_put_cap_snap(struct ceph_cap_snap *capsnap)
 {
-	if (atomic_dec_and_test(&capsnap->nref)) {
+	if (refcount_dec_and_test(&capsnap->nref)) {
 		if (capsnap->xattr_blob)
 			ceph_buffer_put(capsnap->xattr_blob);
 		kfree(capsnap);
-- 
2.7.4

Re: [PATCH 0/2] fs, ceph filesystem refcount conversions

[Ilya Dryomov]

On Fri, Mar 3, 2017 at 10:15 AM, Elena Reshetova
<elena.reshetova@intel.com> wrote:
> Now when new refcount_t type and API are finally merged
> (see include/linux/refcount.h), the following
> patches convert various refcounters in the ceph filesystem from atomic_t
> to refcount_t. By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.
>
> The below patches are fully independent and can be cherry-picked separately.
> Since we convert all kernel subsystems in the same fashion, resulting
> in about 300 patches, we have to group them for sending at least in some
> fashion to be manageable. Please excuse the long cc list.
>
> Not runtime tested, since I am not sure how to do it for ceph.
> However changes are pretty trivial in this case.
>
> Elena Reshetova (2):
>   fs, ceph: convert ceph_mds_session.s_ref from atomic_t to refcount_t
>   fs, ceph: convert ceph_cap_snap.nref from atomic_t to refcount_t
>
>  fs/ceph/caps.c       |  4 ++--
>  fs/ceph/mds_client.c | 18 +++++++++---------
>  fs/ceph/mds_client.h |  5 +++--
>  fs/ceph/snap.c       |  2 +-
>  fs/ceph/super.h      |  5 +++--
>  5 files changed, 18 insertions(+), 16 deletions(-)

I'll pull these into testing.

Thanks,

                Ilya

RE: [PATCH 0/2] fs, ceph filesystem refcount conversions

[Reshetova, Elena]

> On Fri, Mar 3, 2017 at 10:15 AM, Elena Reshetova
> <elena.reshetova@intel.com> wrote:
> > Now when new refcount_t type and API are finally merged
> > (see include/linux/refcount.h), the following
> > patches convert various refcounters in the ceph filesystem from atomic_t
> > to refcount_t. By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> >
> > The below patches are fully independent and can be cherry-picked separately.
> > Since we convert all kernel subsystems in the same fashion, resulting
> > in about 300 patches, we have to group them for sending at least in some
> > fashion to be manageable. Please excuse the long cc list.
> >
> > Not runtime tested, since I am not sure how to do it for ceph.
> > However changes are pretty trivial in this case.
> >
> > Elena Reshetova (2):
> >   fs, ceph: convert ceph_mds_session.s_ref from atomic_t to refcount_t
> >   fs, ceph: convert ceph_cap_snap.nref from atomic_t to refcount_t
> >
> >  fs/ceph/caps.c       |  4 ++--
> >  fs/ceph/mds_client.c | 18 +++++++++---------
> >  fs/ceph/mds_client.h |  5 +++--
> >  fs/ceph/snap.c       |  2 +-
> >  fs/ceph/super.h      |  5 +++--
> >  5 files changed, 18 insertions(+), 16 deletions(-)
> 
> I'll pull these into testing.

Just a check: how did testing went on this? 
If everything is good, can you merge the changes?

Best Regards,
Elena.

> 
> Thanks,
> 
>                 Ilya

Re: [PATCH 0/2] fs, ceph filesystem refcount conversions

[Ilya Dryomov]

On Thu, Apr 20, 2017 at 9:40 AM, Reshetova, Elena
<elena.reshetova@intel.com> wrote:
>
>> On Fri, Mar 3, 2017 at 10:15 AM, Elena Reshetova
>> <elena.reshetova@intel.com> wrote:
>> > Now when new refcount_t type and API are finally merged
>> > (see include/linux/refcount.h), the following
>> > patches convert various refcounters in the ceph filesystem from atomic_t
>> > to refcount_t. By doing this we prevent intentional or accidental
>> > underflows or overflows that can led to use-after-free vulnerabilities.
>> >
>> > The below patches are fully independent and can be cherry-picked separately.
>> > Since we convert all kernel subsystems in the same fashion, resulting
>> > in about 300 patches, we have to group them for sending at least in some
>> > fashion to be manageable. Please excuse the long cc list.
>> >
>> > Not runtime tested, since I am not sure how to do it for ceph.
>> > However changes are pretty trivial in this case.
>> >
>> > Elena Reshetova (2):
>> >   fs, ceph: convert ceph_mds_session.s_ref from atomic_t to refcount_t
>> >   fs, ceph: convert ceph_cap_snap.nref from atomic_t to refcount_t
>> >
>> >  fs/ceph/caps.c       |  4 ++--
>> >  fs/ceph/mds_client.c | 18 +++++++++---------
>> >  fs/ceph/mds_client.h |  5 +++--
>> >  fs/ceph/snap.c       |  2 +-
>> >  fs/ceph/super.h      |  5 +++--
>> >  5 files changed, 18 insertions(+), 16 deletions(-)
>>
>> I'll pull these into testing.
>
> Just a check: how did testing went on this?
> If everything is good, can you merge the changes?

Yep, I'll merge these into 4.12.

Thanks,

                Ilya

RE: [PATCH 0/2] fs, ceph filesystem refcount conversions

[Reshetova, Elena]

> On Thu, Apr 20, 2017 at 9:40 AM, Reshetova, Elena
> <elena.reshetova@intel.com> wrote:
> >
> >> On Fri, Mar 3, 2017 at 10:15 AM, Elena Reshetova
> >> <elena.reshetova@intel.com> wrote:
> >> > Now when new refcount_t type and API are finally merged
> >> > (see include/linux/refcount.h), the following
> >> > patches convert various refcounters in the ceph filesystem from atomic_t
> >> > to refcount_t. By doing this we prevent intentional or accidental
> >> > underflows or overflows that can led to use-after-free vulnerabilities.
> >> >
> >> > The below patches are fully independent and can be cherry-picked
> separately.
> >> > Since we convert all kernel subsystems in the same fashion, resulting
> >> > in about 300 patches, we have to group them for sending at least in some
> >> > fashion to be manageable. Please excuse the long cc list.
> >> >
> >> > Not runtime tested, since I am not sure how to do it for ceph.
> >> > However changes are pretty trivial in this case.
> >> >
> >> > Elena Reshetova (2):
> >> >   fs, ceph: convert ceph_mds_session.s_ref from atomic_t to refcount_t
> >> >   fs, ceph: convert ceph_cap_snap.nref from atomic_t to refcount_t
> >> >
> >> >  fs/ceph/caps.c       |  4 ++--
> >> >  fs/ceph/mds_client.c | 18 +++++++++---------
> >> >  fs/ceph/mds_client.h |  5 +++--
> >> >  fs/ceph/snap.c       |  2 +-
> >> >  fs/ceph/super.h      |  5 +++--
> >> >  5 files changed, 18 insertions(+), 16 deletions(-)
> >>
> >> I'll pull these into testing.
> >
> > Just a check: how did testing went on this?
> > If everything is good, can you merge the changes?
> 
> Yep, I'll merge these into 4.12.

Great, thank you!

Best Regards,
Elena.
> 
> Thanks,
> 
>                 Ilya