On Fri, Nov 18, 2005 at 10:19:40AM +0100, Poul-Henning Kamp wrote:
> In message <437d6ab5.7020...@root.org>, Nate Lawson writes:
>
> >> +void
> >> +disk_gone(struct disk *dp)
> >> +{
> >> + struct g_geom *gp;
> >> + struct g_provider *pp;
> >> +
> >> + gp = dp->d_geom;
> >> + if (gp != NULL)
> >> + LIST_FOREACH(pp, &gp->provider, provider)
> >> + g_orphan_provider(pp, ENXIO);
> >> +}
> >> +
> >
> >Does there need to be locking for this list traversal? Couldn't
> >disk_gone() race in parallel with a taste event if someone plugs/unplugs
> >quickly, especially for a slow device (i.e. floppy)?
>
> Disk gone is called by the driver which owns struct disk, so nobody
> else has any business messing with that particular list.
>
> Obviously the driver needs to not stomp on itself, but Giant does
> that for CAM.Sorry for answering to ~7 years old e-mail, but the lock is actually necessary. Without holding the topology lock in disk_gone() we risk that g_orphan_provider() or g_wither_provider() is more recent version will remove provider from the list and we will use a pointer to freed provider to find next one on the list. Someone recently reported such panic to me and asked if LIST_FOREACH_SAFE() would be enough to fix the problem. Taking into account your response it seems it will be enough, but I still think we should use the topology lock here, especially now that CAM is not Giant-locked. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl
pgpEszLhp8c11.pgp
Description: PGP signature
