kern/104079: [fdc] kldunload fdc.ko leads to panic: mutex Giant owned

[Jaakko Heinonen]

The following reply was made to PR kern/104079; it has been noted by GNATS.

From: Jaakko Heinonen <jh at saunalahti.fi>
To: bug-followup at FreeBSD.org, lynx.ripe at gmail.com
Cc:  
Subject: Re: kern/104079: [fdc] kldunload fdc.ko leads to panic: mutex
	Giant owned
Date: Tue, 11 Nov 2008 18:08:30 +0200

 On module unload fd_detach() is called with Giant held. The problem is
 that fd_detach() tries to acquire the GEOM topology lock (sx lock).
 This is not allowed under Giant.
 
 There are also other bugs related to loading/unloading the fdc module:
 
 * Return value from g_modevent() is ignored. fdc_modevent() always
   returns success even if g_modevent() fails.
 * g_wither_geom() races against module unload. Withering is not
   guaranteed to finish in finite time. If withering doesn't finish
   before g_modevent() call the module unload fails. Combined with
   g_modevent() return value ignorance the result is a panic in g_event
   thread.
   (Looks like g_wither_geom() race against module unload is a common
   problem with many geom classes. atapicd is affected for sure.)
 * When loading the module fdc_attach() must be completed before
   fd_attach(). However fd_attach() runs before fdc_attach().
 
 Following patch works around some of the problems. We can do withering
 as geom event to avoid calling g_wither_geom() under Giant. The makefile
 hack is to change the order of fd_attach() and fdc_attach(). If someone
 knows better way to change ordering I am happy to hear.
 (DRIVER_MODULE() macro doesn't take order (SI_ORDER_*) as argument.)
 
 The patch doesn't address g_wither_geom() race (except checking
 g_modevent() return value may prevent a panic in some cases).
 
 %%%
 Index: sys/modules/fdc/Makefile
 ===================================================================
 --- sys/modules/fdc/Makefile	(revision 184512)
 +++ sys/modules/fdc/Makefile	(working copy)
 @@ -7,11 +7,12 @@ KMOD=	fdc
  SRCS=	fdc.c fdc_cbus.c
  .else
  .PATH:  ${.CURDIR}/../../dev/fdc
 -SRCS=	fdc.c fdc_isa.c fdc_pccard.c
 +SRCS=	fdc_isa.c fdc_pccard.c
  .if ${MACHINE} == "i386" || ${MACHINE} == "amd64"
  CFLAGS+= -I${.CURDIR}/../../contrib/dev/acpica
  SRCS+=	opt_acpi.h acpi_if.h fdc_acpi.c
  .endif
 +SRCS+=	fdc.c
  .endif
  
  SRCS+=	opt_fdc.h bus_if.h card_if.h device_if.h \
 Index: sys/dev/fdc/fdc.c
 ===================================================================
 --- sys/dev/fdc/fdc.c	(revision 184512)
 +++ sys/dev/fdc/fdc.c	(working copy)
 @@ -2012,15 +2012,22 @@ fd_attach(device_t dev)
  	return (0);
  }
  
 +static void
 +fd_detach_geom(void *arg, int flag)
 +{
 +	struct	fd_data *fd = arg;
 +
 +	g_topology_assert();
 +	g_wither_geom(fd->fd_geom, ENXIO);
 +}
 +
  static int
  fd_detach(device_t dev)
  {
  	struct	fd_data *fd;
  
  	fd = device_get_softc(dev);
 -	g_topology_lock();
 -	g_wither_geom(fd->fd_geom, ENXIO);
 -	g_topology_unlock();
 +	g_waitfor_event(fd_detach_geom, fd, M_WAITOK, NULL);
  	while (device_get_state(dev) == DS_BUSY)
  		tsleep(fd, PZERO, "fdd", hz/10);
  	callout_drain(&fd->toffhandle);
 @@ -2049,8 +2056,7 @@ static int
  fdc_modevent(module_t mod, int type, void *data)
  {
  
 -	g_modevent(NULL, type, &g_fd_class);
 -	return (0);
 +	return (g_modevent(NULL, type, &g_fd_class));
  }
  
  DRIVER_MODULE(fd, fdc, fd_driver, fd_devclass, fdc_modevent, 0);
 %%%
 
 -- 
 Jaakko