headers
Steven Haber | 11 Jun 2012 19:19

Geom / destroy_dev() deadlock

Hey FreeBSD devs,

I hope this is the right forum for this. I haven't used the FreeBSD
mailing lists before. I'm a relatively new hire at EMC Isilon. We are
using FreeBSD 7.0 as the basis for our scale-out NAS product line. We
are frequently hitting the deadlock described here:

http://lists.freebsd.org/pipermail/freebsd-geom/2010-February/003888.htm
l

The race is that destroy_dev() sleeps indefinitely waiting for thread
references to drop from a dev. In the case of geom, we hold the topology
lock the whole time we're in the dev layer. In devfs_open() and
devfs_close(), we take a ref on the dev before calling into to geom.

The proposed solution of destroy_dev_sched() makes sense to me. I am
trying to understand the necessity of Alexander Motin's additional
changes. destroy_dev_tq() holds the devmtx during the destroy and devfs
uses this lock to take refs before calling into geom. To me it seems we
should be able to protect the cdev with just the devmtx, provided
consumers of d_close(), d_open(), etc. ref and rel the dev
appropriately.

Steven Haber
Software Engineer, EMC Isilon
(206) 753-1526
Permalink | Reply |
headers
Konstantin Belousov | 11 Jun 2012 22:43
Picon

Re: Geom / destroy_dev() deadlock

On Mon, Jun 11, 2012 at 10:19:07AM -0700, Steven Haber wrote:
> Hey FreeBSD devs,
> 
> I hope this is the right forum for this. I haven't used the FreeBSD
> mailing lists before. I'm a relatively new hire at EMC Isilon. We are
> using FreeBSD 7.0 as the basis for our scale-out NAS product line. We
> are frequently hitting the deadlock described here:
> 
> http://lists.freebsd.org/pipermail/freebsd-geom/2010-February/003888.htm
> l
> 
> The race is that destroy_dev() sleeps indefinitely waiting for thread
> references to drop from a dev. In the case of geom, we hold the topology
> lock the whole time we're in the dev layer. In devfs_open() and
> devfs_close(), we take a ref on the dev before calling into to geom.
> 
> The proposed solution of destroy_dev_sched() makes sense to me. I am
> trying to understand the necessity of Alexander Motin's additional
> changes. destroy_dev_tq() holds the devmtx during the destroy and devfs
> uses this lock to take refs before calling into geom. To me it seems we
> should be able to protect the cdev with just the devmtx, provided
> consumers of d_close(), d_open(), etc. ref and rel the dev
> appropriately.
devmtx is mutex. If taken, then cdevsw methods would be unable to sleep.
Permalink | Reply |
headers
Steven Haber | 11 Jun 2012 23:15

RE: Geom / destroy_dev() deadlock

> On Mon, Jun 11, 2012 at 10:19:07AM -0700, Steven Haber wrote:
>> Hey FreeBSD devs,
>>  
>> I hope this is the right forum for this. I haven't used the FreeBSD
>> mailing lists before. I'm a relatively new hire at EMC Isilon. We are
>> using FreeBSD 7.0 as the basis for our scale-out NAS product line. We
>> are frequently hitting the deadlock described here:
>> 
>>
http://lists.freebsd.org/pipermail/freebsd-geom/2010-February/003888.htm
>> l
>> 
>> The race is that destroy_dev() sleeps indefinitely waiting for thread
>> references to drop from a dev. In the case of geom, we hold the
topology
>> lock the whole time we're in the dev layer. In devfs_open() and
>> devfs_close(), we take a ref on the dev before calling into to geom.
>> 
>> The proposed solution of destroy_dev_sched() makes sense to me. I am
>> trying to understand the necessity of Alexander Motin's additional
>> changes. destroy_dev_tq() holds the devmtx during the destroy and
devfs
>> uses this lock to take refs before calling into geom. To me it seems
we
>> should be able to protect the cdev with just the devmtx, provided
>> consumers of d_close(), d_open(), etc. ref and rel the dev
>> appropriately.

> From: Konstantin Belousov
> Sent: Monday, June 11, 2012 1:44 PM
(Continue reading)

Permalink | Reply |
headers
Konstantin Belousov | 11 Jun 2012 23:56
Picon

Re: Geom / destroy_dev() deadlock

On Mon, Jun 11, 2012 at 02:15:56PM -0700, Steven Haber wrote:
> > On Mon, Jun 11, 2012 at 10:19:07AM -0700, Steven Haber wrote:
> >> Hey FreeBSD devs,
> >>  
> >> I hope this is the right forum for this. I haven't used the FreeBSD
> >> mailing lists before. I'm a relatively new hire at EMC Isilon. We are
> >> using FreeBSD 7.0 as the basis for our scale-out NAS product line. We
> >> are frequently hitting the deadlock described here:
> >> 
> >>
> http://lists.freebsd.org/pipermail/freebsd-geom/2010-February/003888.htm
> >> l
> >> 
> >> The race is that destroy_dev() sleeps indefinitely waiting for thread
> >> references to drop from a dev. In the case of geom, we hold the
> topology
> >> lock the whole time we're in the dev layer. In devfs_open() and
> >> devfs_close(), we take a ref on the dev before calling into to geom.
> >> 
> >> The proposed solution of destroy_dev_sched() makes sense to me. I am
> >> trying to understand the necessity of Alexander Motin's additional
> >> changes. destroy_dev_tq() holds the devmtx during the destroy and
> devfs
> >> uses this lock to take refs before calling into geom. To me it seems
> we
> >> should be able to protect the cdev with just the devmtx, provided
> >> consumers of d_close(), d_open(), etc. ref and rel the dev
> >> appropriately.
> 
>
(Continue reading)

Permalink | Reply |
headers
Steven Haber | 12 Jun 2012 00:27

RE: Geom / destroy_dev() deadlock

> On Mon, Jun 11, 2012 at 02:15:56PM -0700, Steven Haber wrote:
> > > On Mon, Jun 11, 2012 at 10:19:07AM -0700, Steven Haber wrote:
> > > > Hey FreeBSD devs,
> > > >  
> > > > I hope this is the right forum for this. I haven't used the
FreeBSD
> > > > mailing lists before. I'm a relatively new hire at EMC Isilon.
We are
> > > > using FreeBSD 7.0 as the basis for our scale-out NAS product
line. We
> > > > are frequently hitting the deadlock described here:
> > > > 
> > > >
> >
http://lists.freebsd.org/pipermail/freebsd-geom/2010-February/003888.htm
l
> > > > 
> > > > The race is that destroy_dev() sleeps indefinitely waiting for
thread
> > > > references to drop from a dev. In the case of geom, we hold the
> > topology
> > > > lock the whole time we're in the dev layer. In devfs_open() and
> > > > devfs_close(), we take a ref on the dev before calling into to
geom.
> > > > 
> > > > The proposed solution of destroy_dev_sched() makes sense to me.
I am
> > > > trying to understand the necessity of Alexander Motin's
additional
> > > > changes. destroy_dev_tq() holds the devmtx during the destroy
(Continue reading)

Permalink | Reply |
headers
Konstantin Belousov | 12 Jun 2012 00:53
Picon

Re: Geom / destroy_dev() deadlock

On Mon, Jun 11, 2012 at 03:27:39PM -0700, Steven Haber wrote:
> > I do not understand what you are proposing. Could you, please, show
> > the patch ?
> 
> ---
>  src/sys/geom/geom_dev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sys/geom/geom_dev.c b/src/sys/geom/geom_dev.c
> index 38251e1..787235a 100644
> --- a/src/sys/geom/geom_dev.c
> +++ b/src/sys/geom/geom_dev.c
>  <at>  <at>  -497,7 +497,7  <at>  <at>  g_dev_orphan(struct g_consumer *cp)
>  
>         /* Destroy the struct cdev *so we get no more requests */
>         unit = dev2unit(dev);
> -       destroy_dev(dev);
> +       destroy_dev_sched(dev);
>         free_unr(unithdr, unit);
>  
>         /* Wait for the cows to come home */

Did you noted the comment above the block you changing ?
The patch would cause races allowing arbitrary kernel memory corruption.

The moment when the cdev is destroyed is somewhere in future, while
structures that the cdev reference are freed synchronously.

I tried to put some safety measures into destroy_dev_sched(9), namely
CDP_SCHED_DTR flag that somewhat reduces the possibility of usermode
(Continue reading)

Permalink | Reply |
headers
Steven Haber | 19 Jun 2012 20:24

RE: Geom / destroy_dev() deadlock

> On Mon, Jun 11, 2012 at 03:27:39PM -0700, Steven Haber wrote:
> > I do not understand what you are proposing. Could you, please, show
> > > the patch ?
> > 
> > ---
> >  src/sys/geom/geom_dev.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/sys/geom/geom_dev.c b/src/sys/geom/geom_dev.c
> > index 38251e1..787235a 100644
> > --- a/src/sys/geom/geom_dev.c
> > +++ b/src/sys/geom/geom_dev.c
> >  <at>  <at>  -497,7 +497,7  <at>  <at>  g_dev_orphan(struct g_consumer *cp)
> >  
> >         /* Destroy the struct cdev *so we get no more requests */
> >         unit = dev2unit(dev);
> > -       destroy_dev(dev);
> > +       destroy_dev_sched(dev);
> >         free_unr(unithdr, unit);
> >  
> >         /* Wait for the cows to come home */
>
> From: Konstantin Belousov
> Sent: Monday, June 11, 2012 3:53 PM
> To: Steven Haber
> Cc: freebsd-geom <at> freebsd.org
> Subject: Re: Geom / destroy_dev() deadlock
>
> Did you noted the comment above the block you changing ?
> The patch would cause races allowing arbitrary kernel memory
(Continue reading)

Permalink | Reply |
headers
Konstantin Belousov | 20 Jun 2012 09:56
Picon

Re: Geom / destroy_dev() deadlock

On Tue, Jun 19, 2012 at 11:24:03AM -0700, Steven Haber wrote:
> > On Mon, Jun 11, 2012 at 03:27:39PM -0700, Steven Haber wrote:
> > > I do not understand what you are proposing. Could you, please, show
> > > > the patch ?
> > > 
> > > ---
> > >  src/sys/geom/geom_dev.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/sys/geom/geom_dev.c b/src/sys/geom/geom_dev.c
> > > index 38251e1..787235a 100644
> > > --- a/src/sys/geom/geom_dev.c
> > > +++ b/src/sys/geom/geom_dev.c
> > >  <at>  <at>  -497,7 +497,7  <at>  <at>  g_dev_orphan(struct g_consumer *cp)
> > >  
> > >         /* Destroy the struct cdev *so we get no more requests */
> > >         unit = dev2unit(dev);
> > > -       destroy_dev(dev);
> > > +       destroy_dev_sched(dev);
> > >         free_unr(unithdr, unit);
> > >  
> > >         /* Wait for the cows to come home */
> >
> > From: Konstantin Belousov
> > Sent: Monday, June 11, 2012 3:53 PM
> > To: Steven Haber
> > Cc: freebsd-geom <at> freebsd.org
> > Subject: Re: Geom / destroy_dev() deadlock
> >
> > Did you noted the comment above the block you changing ?
(Continue reading)

Permalink | Reply |

Gmane