From: Trond Myklebust <trondmy-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
To: "elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org"
<elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
"netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org"
<jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
Cc: "herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org"
<herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>,
"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org"
<peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>,
"ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org"
<ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>,
"linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"ishkamiel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org"
<ishkamiel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
"bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org"
<bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>,
"steffen.klassert-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org"
<steffen.klassert-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org>,
"nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org"
<nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>,
"linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"jreuter-K7Hl1MveuGQ@public.gmane.org"
<jreuter-K7Hl1MveuGQ@public.gmane.org>,
"keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org"
<keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
"linux-hams-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-hams-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org"
<dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
"zyan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org"
<zyan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, sage@redhat.
Subject: Re: [PATCH 01/23] net, sunrpc: convert rpc_cred.cr_count from atomic_t to refcount_t
Date: Fri, 17 Mar 2017 14:28:37 +0000 [thread overview]
Message-ID: <1489760913.8441.1.camel@primarydata.com> (raw)
In-Reply-To: <1489755736.2810.10.camel-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
On Fri, 2017-03-17 at 09:02 -0400, Jeff Layton wrote:
> On Fri, 2017-03-17 at 12:50 +0000, Trond Myklebust wrote:
> > On Fri, 2017-03-17 at 14:10 +0200, Elena Reshetova wrote:
> > > 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>
> > > ---
> > > include/linux/sunrpc/auth.h | 8 ++++----
> > > net/sunrpc/auth.c | 12 ++++++------
> > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/linux/sunrpc/auth.h
> > > b/include/linux/sunrpc/auth.h
> > > index b1bc62b..bd36e0b 100644
> > > --- a/include/linux/sunrpc/auth.h
> > > +++ b/include/linux/sunrpc/auth.h
> > > @@ -15,7 +15,7 @@
> > > #include <linux/sunrpc/msg_prot.h>
> > > #include <linux/sunrpc/xdr.h>
> > >
> > > -#include <linux/atomic.h>
> > > +#include <linux/refcount.h>
> > > #include <linux/rcupdate.h>
> > > #include <linux/uidgid.h>
> > > #include <linux/utsname.h>
> > > @@ -68,7 +68,7 @@ struct rpc_cred {
> > > #endif
> > > unsigned long cr_expire; /* when
> > > to gc
> > > */
> > > unsigned long cr_flags; /* various
> > > flags */
> > > - atomic_t cr_count; /* ref count */
> > > + refcount_t cr_count; /* ref count
> > > */
> > >
> >
> > NACK. That's going to be hitting
> > WARN_ONCE(!refcount_inc_not_zero(r),
> > "refcount_t: increment on 0; use-after-free.\n") like there's no
> > tomorrow...
> >
> > Please stop with these automated conversions. They are going to
> > cause a
> > lot more bugs than they fix.
> >
>
> Agreed. These patchsets are touching places where we've already
> banged
> out most of the refcounting bugs. I'm against doing large scale
> conversions like this without a damned good reason.
>
> I think it may be best to do this sort of thing in a more piecemeal
> fashion. Pick a subsystem or two and do the conversions there to
> prove
> that they're better than what we have. If the subsystem already has
> problems with its refcounting, then so much the better. Point to bugs
> that this new infrastructure helped find.
>
> Encourage people to adopt your new infrastructure as new refcounted
> objects are introduced into the kernel. You might even consider a LWN
> article about this.
>
> Eventually we'll get around to changing existing code to use it, once
> there is a sufficient advantage to doing so. Most likely when we're
> reworking the code for other reasons, or when we're chasing some
> horrid
> refcounting bug and think that this might help find it.
The main issue is that this "refcount_t" implementation appears to be
assuming that there is one and only one model for refcounts (the one
where a value of "0" means "free me immediately").
The kernel has a plethora of object caching implementations where this
is simply not the case; the dcache is a prime example, and this cache
is another. In both these implementation, the atomic_t variable is
being used more as a semaphore-style lock that prevents freeing of the
object while it is in active use as opposed to being freeable, but
cached. This is why these automated conversions are a nuisance and a
source of bugs.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
next prev parent reply other threads:[~2017-03-17 14:28 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-17 12:10 [PATCH 00/23] various networking refcount conversions, part 2 Elena Reshetova
2017-03-17 12:10 ` [PATCH 01/23] net, sunrpc: convert rpc_cred.cr_count from atomic_t to refcount_t Elena Reshetova
[not found] ` <1489752646-8749-2-git-send-email-elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-03-17 12:50 ` Trond Myklebust
2017-03-17 13:02 ` Jeff Layton
[not found] ` <1489755736.2810.10.camel-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
2017-03-17 14:28 ` Trond Myklebust [this message]
2017-03-20 16:15 ` Reshetova, Elena
2017-03-17 12:10 ` [PATCH 02/23] net, sunrpc: convert gss_cl_ctx.count " Elena Reshetova
2017-03-17 12:10 ` [PATCH 03/23] net, sunrpc: convert gss_upcall_msg.count " Elena Reshetova
2017-03-17 12:10 ` [PATCH 04/23] net, ceph: convert ceph_snap_context.nref " Elena Reshetova
2017-03-24 13:20 ` Ilya Dryomov
2017-03-17 12:10 ` [PATCH 05/23] net, ceph: convert ceph_osd.o_ref " Elena Reshetova
[not found] ` <1489752646-8749-6-git-send-email-elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-03-24 13:49 ` Ilya Dryomov
2017-03-17 12:10 ` [PATCH 06/23] net, ceph: convert ceph_pagelist.refcnt " Elena Reshetova
[not found] ` <1489752646-8749-7-git-send-email-elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-03-24 13:49 ` Ilya Dryomov
2017-03-17 12:10 ` [PATCH 07/23] net, rds: convert rds_ib_device.refcount " Elena Reshetova
2017-03-17 12:10 ` [PATCH 08/23] net, rds: convert rds_incoming.i_refcount " Elena Reshetova
2017-03-17 12:10 ` [PATCH 09/23] net, rds: convert rds_mr.r_refcount " Elena Reshetova
2017-03-17 12:10 ` [PATCH 11/23] net, x25: convert x25_route.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 12/23] net, x25: convert x25_neigh.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 15/23] net, xfrm: convert sec_path.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 16/23] net, sctp: convert sctp_auth_bytes.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 17/23] net, sctp: convert sctp_datamsg.refcnt " Elena Reshetova
[not found] ` <1489752646-8749-1-git-send-email-elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-03-17 12:10 ` [PATCH 10/23] net, rds: convert rds_message.m_refcount " Elena Reshetova
2017-03-17 12:10 ` [PATCH 13/23] net, xfrm: convert xfrm_state.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 14/23] net, xfrm: convert xfrm_policy.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 18/23] net, sctp: convert sctp_chunk.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 19/23] net, sctp: convert sctp_transport.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 21/23] net, ax25: convert ax25_uid_assoc.refcount " Elena Reshetova
2017-03-17 12:10 ` [PATCH 22/23] net, ax25: convert ax25_route.refcount " Elena Reshetova
2017-03-17 12:10 ` [PATCH 20/23] net, sctp: convert sctp_ep_common.refcnt " Elena Reshetova
2017-03-17 12:10 ` [PATCH 23/23] net, ax25: convert ax25_cb.refcount " Elena Reshetova
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1489760913.8441.1.camel@primarydata.com \
--to=trondmy-7i+n7zu2hftekmmhf/gkza@public.gmane.org \
--cc=bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org \
--cc=dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=elena.reshetova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
--cc=herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org \
--cc=ishkamiel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org \
--cc=jreuter-K7Hl1MveuGQ@public.gmane.org \
--cc=keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
--cc=linux-hams-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org \
--cc=peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org \
--cc=ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org \
--cc=sage@redhat. \
--cc=steffen.klassert-opNxpl+3fjRBDgjK7y7TUQ@public.gmane.org \
--cc=zyan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).