From: James Bottomley <James.Bottomley@HansenPartnership•com>
To: "Reshetova, Elena" <elena.reshetova@intel•com>,
Michael Ellerman <mpe@ellerman•id.au>,
"gregkh@linuxfoundation•org" <gregkh@linuxfoundation•org>
Cc: "linux-kernel@vger•kernel.org" <linux-kernel@vger•kernel.org>,
"xen-devel@lists•xenproject.org" <xen-devel@lists•xenproject.org>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"linux1394-devel@lists•sourceforge.net"
<linux1394-devel@lists•sourceforge.net>,
"linux-bcache@vger•kernel.org" <linux-bcache@vger•kernel.org>,
"linux-raid@vger•kernel.org" <linux-raid@vger•kernel.org>,
"linux-media@vger•kernel.org" <linux-media@vger•kernel.org>,
"devel@linuxdriverproject•org" <devel@linuxdriverproject•org>,
"linux-pci@vger•kernel.org" <linux-pci@vger•kernel.org>,
"linux-s390@vger•kernel.org" <linux-s390@vger•kernel.org>,
"fcoe-devel@open-fcoe•org" <fcoe-devel@open-fcoe•org>,
"linux-scsi@vger•kernel.org" <linux-scsi@vger•kernel.org>,
"open-iscsi@googlegroups•com" <open-iscsi@googlegroups•com>,
"devel@driverdev•osuosl.org" <devel@driverdev•osuosl.org>,
"target-devel@vger•kernel.org" <target-devel@vger•kernel.org>,
"linux-serial@vger•kernel.org" <linux-serial@vger•kernel.org>,
"linux-usb@vger•kernel.org" <linux-usb@vger•kernel.org>,
"peterz@infradead•org" <peterz@infradead•org>,
Hans Liljestrand <ishkamiel@gmail•com>,
Kees Cook <keescook@chromium•org>,
David Windsor <dwindsor@gmail•com>,
"linuxppc-dev@lists•ozlabs.org" <linuxppc-dev@lists•ozlabs.org>
Subject: Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t
Date: Tue, 14 Mar 2017 07:58:59 -0700 [thread overview]
Message-ID: <1489503539.3214.17.camel@HansenPartnership.com> (raw)
In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B41C588E8@IRSMSX102.ger.corp.intel.com>
On Tue, 2017-03-14 at 12:29 +0000, Reshetova, Elena wrote:
> > Elena Reshetova <elena.reshetova@intel•com> writes:
> >
> > > 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>
> > > ---
> > > drivers/md/md.c | 6 +++---
> > > drivers/md/md.h | 3 ++-
> > > 2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > When booting linux-next (specifically 5be4921c9958ec) I'm seeing
> > the
> > backtrace below. I suspect this patch is just exposing an existing
> > issue?
>
> Yes, we have actually been following this issue in the another
> thread.
> It looks like the object is re-used somehow, but I can't quite
> understand how just by reading the code.
> This was what I put into the previous thread:
>
> "The log below indicates that you are using your refcounter in a bit
> weird way in mddev_find().
> However, I can't find the place (just by reading the code) where you
> would increment refcounter from zero (vs. setting it to one).
> It looks like you either iterate over existing nodes (and increment
> their counters, which should be >= 1 at the time of increment) or
> create a new node, but then mddev_init() sets the counter to 1. "
>
> If you can help to understand what is going on with the object
> creation/destruction, would be appreciated!
>
> Also Shaohua Li stopped this patch coming from his tree since the
> issue was caught at that time, so we are not going to merge this
> until we figure it out.
Asking on the correct list (dm-devel) would have got you the easy
answer: The refcount behind mddev->active is a genuine atomic. It has
refcount properties but only if the array fails to initialise (in that
case, final put kills it). Once it's added to the system as a gendisk,
it cannot be freed until md_free(). Thus its ->active count can go to
zero (when it becomes inactive; usually because of an unmount). On a
simple allocation regardless of outcome, the last executed statement in
md_alloc is mddev_put(): that destroys the device if we didn't manage
to create it or returns 0 and adds an inactive device to the system
which the user can get with mddev_find().
James
next prev parent reply other threads:[~2017-03-14 15:05 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1488810076-3754-1-git-send-email-elena.reshetova@intel.com>
[not found] ` <1488810076-3754-9-git-send-email-elena.reshetova@intel.com>
2017-03-14 12:11 ` [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t Michael Ellerman
2017-03-14 12:29 ` Reshetova, Elena
2017-03-14 14:58 ` James Bottomley [this message]
2017-03-16 18:00 ` Reshetova, Elena
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=1489503539.3214.17.camel@HansenPartnership.com \
--to=james.bottomley@hansenpartnership$(echo .)com \
--cc=devel@driverdev$(echo .)osuosl.org \
--cc=devel@linuxdriverproject$(echo .)org \
--cc=dwindsor@gmail$(echo .)com \
--cc=elena.reshetova@intel$(echo .)com \
--cc=fcoe-devel@open-fcoe$(echo .)org \
--cc=gregkh@linuxfoundation$(echo .)org \
--cc=ishkamiel@gmail$(echo .)com \
--cc=keescook@chromium$(echo .)org \
--cc=linux-bcache@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-media@vger$(echo .)kernel.org \
--cc=linux-pci@vger$(echo .)kernel.org \
--cc=linux-raid@vger$(echo .)kernel.org \
--cc=linux-s390@vger$(echo .)kernel.org \
--cc=linux-scsi@vger$(echo .)kernel.org \
--cc=linux-serial@vger$(echo .)kernel.org \
--cc=linux-usb@vger$(echo .)kernel.org \
--cc=linux1394-devel@lists$(echo .)sourceforge.net \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mpe@ellerman$(echo .)id.au \
--cc=netdev@vger$(echo .)kernel.org \
--cc=open-iscsi@googlegroups$(echo .)com \
--cc=peterz@infradead$(echo .)org \
--cc=target-devel@vger$(echo .)kernel.org \
--cc=xen-devel@lists$(echo .)xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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