On Mon, Jun 13, 2016 at 11:00:44 -0400, George Wilson wrote:
> Is the use-after-free issue in openzfs or just Nexenta's gate? Just want to
> make sure we have a bug filed if it's in openzfs.
Just Nexenta's gate. E.g., for the l2arc case it looks like:
} else if (spa->spa_l2cache.sav_vdevs != NULL &&
nvlist_lookup_nvlist_array(spa->spa_l2cache.sav_config,
ZPOOL_CONFIG_L2CACHE, &l2cache, &nl2cache) == 0 &&
(nv = spa_nvlist_lookup_by_guid(l2cache, nl2cache, guid)) != NULL) {
/*
* Cache devices can always be removed.
*/
if (vd == NULL)
vd = spa_lookup_by_guid(spa, guid, B_TRUE);
<Nexenta code deref'ing vd ommitted for brevity>
spa_vdev_remove_aux(spa->spa_l2cache.sav_config,
ZPOOL_CONFIG_L2CACHE, l2cache, nl2cache, nv);
spa_load_l2cache(spa);
spa->spa_l2cache.sav_sync = B_TRUE;
spa_event_notify(spa, vd, ESC_ZFS_VDEV_REMOVE_AUX);
} else if (vd != NULL && vd->vdev_islog) {
The problem here is that spa_load_l2cache() can free the vd that we looked up -
but we end up passing it to the spa_event_notify() call just the same.
We're fixing it in our gate, but I want to clean it up a bit in openzfs to
avoid this recurring/other bad things happening. The spare case it a bit more
complicated, but the same basic idea applies. (On top of the event getting
generated even when EBUSY.)
Jeff.
> Thanks,
> George
>
> On Mon, Jun 13, 2016 at 10:58 AM, Josef 'Jeff' Sipek <jef...@josefsipek.net>
> wrote:
>
> > On Mon, Jun 13, 2016 at 10:48:14 -0400, George Wilson wrote:
> > > Jeff,
> > >
> > > Seems reasonable to me. If we wanted to provide more information about
> > the
> > > vdev being removed then you could make a call to spa_lookup_by_guid(spa,
> > > guid, B_TRUE) just prior to the call to spa_vdev_remove_aux(). You would
> > > only do this is vd == NULL but that could be another way to enhance this
> > > logic.
> >
> > Amusingly enough, the reason I ended up looking at this is exactly because
> > we have that kind of lookup in Nexenta's gate. It turns out that using the
> > vd looked up with aux == B_TRUE leads to a use-after-free. :) Yeah, I
> > could
> > try to stash things elsewhere (e.g., make a copy of the vd), but IMO it's
> > not really worth it.
> >
> > Jeff.
> >
> > >
> > > Thanks,
> > > George
> > >
> > > On Fri, Jun 10, 2016 at 5:14 PM, Alan Somers <asom...@freebsd.org>
> > wrote:
> > >
> > > > On Fri, Jun 10, 2016 at 2:53 PM, Josef 'Jeff' Sipek
> > > > <jef...@josefsipek.net> wrote:
> > > > > I've been looking at the code that 6922 introduced and to me it looks
> > > > like
> > > > > the change is too complicated/misleading and slightly buggy.
> > > > >
> > > > > l2cache case
> > > > > ------------
> > > > >
> > > > > At the beginning of the function, we try to convert a guid to a
> > vdev_t
> > > > > pointer by calling spa_lookup_by_guid with aux == B_FALSE. It is my
> > > > > understanding that this will always result in vd == NULL if the guid
> > is
> > > > for
> > > > > an L2ARC device. That means that when we make it into the sav_vdevs
> > !=
> > > > NULL
> > > > > else-if, the sole user of vd will always be NULL.
> > > > >
> > > > > This brings up the next question. How useful are these events since
> > > > they don't
> > > > > include the removed vdev's guid and path?
> > > > >
> > > > > spares case
> > > > > -----------
> > > > >
> > > > > This is similar to the l2cache case, but a bit more complicated.
> > Unlike
> > > > the
> > > > > l2cache case, this case can fail with EBUSY. However, in the event
> > of a
> > > > > failure, the code still generates the event. This is easy to fix by
> > > > moving
> > > > > the event generation higher up.
> > > > >
> > > > > Here, we certainly want to use vd, since vd can be either NULL or
> > > > non-NULL.
> > > > > It is NULL (and unspare is 0) in the case of 'zpool remove' of an
> > unused
> > > > > spare device, but non-NULL (and unspare is 1) in the case of 'zpool
> > > > detach'
> > > > > of a removed device that a spare took over from.
> > > > >
> > > > >
> > > > > So, with all this said, unless I'm wrong about something, I'd like
> > review
> > > > > for the combined change:
> > > > >
> > > > > diff --git a/usr/src/uts/common/fs/zfs/spa.c
> > > > b/usr/src/uts/common/fs/zfs/spa.c
> > > > > index 07abe37..0265887 100644
> > > > > --- a/usr/src/uts/common/fs/zfs/spa.c
> > > > > +++ b/usr/src/uts/common/fs/zfs/spa.c
> > > > > @@ -5473,10 +5473,10 @@ spa_vdev_remove(spa_t *spa, uint64_t guid,
> > > > boolean_t unspare)
> > > > > ZPOOL_CONFIG_SPARES, spares, nspares,
> > nv);
> > > > > spa_load_spares(spa);
> > > > > spa->spa_spares.sav_sync = B_TRUE;
> > > > > + spa_event_notify(spa, vd,
> > > > ESC_ZFS_VDEV_REMOVE_AUX);
> > > > > } else {
> > > > > error = SET_ERROR(EBUSY);
> > > > > }
> > > > > - spa_event_notify(spa, vd, ESC_ZFS_VDEV_REMOVE_AUX);
> > > > > } else if (spa->spa_l2cache.sav_vdevs != NULL &&
> > > > > nvlist_lookup_nvlist_array(spa->spa_l2cache.sav_config,
> > > > > ZPOOL_CONFIG_L2CACHE, &l2cache, &nl2cache) == 0 &&
> > > > > @@ -5488,7 +5488,7 @@ spa_vdev_remove(spa_t *spa, uint64_t guid,
> > > > boolean_t unspare)
> > > > > ZPOOL_CONFIG_L2CACHE, l2cache, nl2cache, nv);
> > > > > spa_load_l2cache(spa);
> > > > > spa->spa_l2cache.sav_sync = B_TRUE;
> > > > > - spa_event_notify(spa, vd, ESC_ZFS_VDEV_REMOVE_AUX);
> > > > > + spa_event_notify(spa, NULL, ESC_ZFS_VDEV_REMOVE_AUX);
> > > > > } else if (vd != NULL && vd->vdev_islog) {
> > > > > ASSERT(!locked);
> > > > > ASSERT(vd == vd->vdev_top);
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jeff.
> > > > >
> > > > > --
> > > > > I already backed up the [server] once, I can do it again.
> > > > > - a sysadmin threatening to do more frequent backups
> > > >
> > > > From inspection your proposal looks good to me, but I won't have time
> > > > to test it until next week.
> > > >
> > > > -Alan
> > > >
> >
> > --
> > Bad pun of the week: The formula 1 control computer suffered from a race
> > condition
> >
--
mainframe, n.:
An obsolete device still used by thousands of obsolete companies serving
billions of obsolete customers and making huge obsolete profits for their
obsolete shareholders. And this year's run twice as fast as last year's.
-------------------------------------------
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription:
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com
