Re: [PATCH] try_module_get(THIS_MODULE) is bogus

[Christoph Hellwig <hch@sgi.com>]

On Fri, Feb 21, 2003 at 06:18:43PM -0500, Christoph Hellwig wrote:
> In most cases the fix is to add an struct module * member to the operations
> vector instead and manipulate the refcounts in the callers context.
> 
> For the ALSA cases it was completly superflous (when will people get it that
> using an exported symbol will make it's module unloadable?..)

This one had a use after free bug..  Fixed version below.


--- 1.2/drivers/char/ipmi/ipmi_kcs_intf.c	Tue Feb 18 19:31:36 2003
+++ edited/drivers/char/ipmi/ipmi_kcs_intf.c	Fri Feb 21 19:45:45 2003
@@ -613,18 +613,6 @@
 	atomic_set(&kcs_info->req_events, 1);
 }
 
-static int new_user(void *send_info)
-{
-	if (!try_module_get(THIS_MODULE))
-		return -EBUSY;
-	return 0;
-}
-
-static void user_left(void *send_info)
-{
-	module_put(THIS_MODULE);
-}
-
 /* Call every 10 ms. */
 #define KCS_TIMEOUT_TIME_USEC	10000
 #define KCS_USEC_PER_JIFFY	(1000000/HZ)
@@ -718,11 +706,10 @@
 
 static struct ipmi_smi_handlers handlers =
 {
-	sender:		       sender,
-	request_events:        request_events,
-	new_user:	       new_user,
-	user_left:	       user_left,
-	set_run_to_completion: set_run_to_completion
+	.owner			= THIS_MODULE,
+	.sender			= sender,
+	.request_events		= request_events,
+	.set_run_to_completion	= set_run_to_completion,
 };
 
 static unsigned char ipmi_kcs_dev_rev;
===== drivers/char/ipmi/ipmi_msghandler.c 1.1 vs edited =====
--- 1.1/drivers/char/ipmi/ipmi_msghandler.c	Mon Jan 13 13:07:12 2003
+++ edited/drivers/char/ipmi/ipmi_msghandler.c	Fri Feb 21 19:45:45 2003
@@ -485,13 +485,14 @@
 	new_user->intf = ipmi_interfaces[if_num];
 	new_user->gets_events = 0;
 
-	rv = new_user->intf->handlers->new_user(new_user->intf->send_info);
-	if (rv)
+	if (!try_module_get(new_user->intf->handlers->owner)) {
+		rv = -ENODEV;
 		goto out_unlock;
+	}
 
-	write_lock_irqsave(&(new_user->intf->users_lock), flags);
-	list_add_tail(&(new_user->link), &(new_user->intf->users));
-	write_unlock_irqrestore(&(new_user->intf->users_lock), flags);
+	write_lock_irqsave(&new_user->intf->users_lock, flags);
+	list_add_tail(&new_user->link, &new_user->intf->users);
+	write_unlock_irqrestore(&new_user->intf->users_lock, flags);
 
  out_unlock:	
 	if (rv) {
@@ -563,12 +564,12 @@
 	unsigned long flags;
 
 	down_read(&interfaces_sem);
-	write_lock_irqsave(&(intf->users_lock), flags);
+	write_lock_irqsave(&intf->users_lock, flags);
 	rv = ipmi_destroy_user_nolock(user);
 	if (!rv)
-		intf->handlers->user_left(intf->send_info);
+		module_put(intf->handlers->owner);
 		
-	write_unlock_irqrestore(&(intf->users_lock), flags);
+	write_unlock_irqrestore(&intf->users_lock, flags);
 	up_read(&interfaces_sem);
 	return rv;
 }
===== drivers/ieee1394/hosts.c 1.13 vs edited =====
--- 1.13/drivers/ieee1394/hosts.c	Sat Feb  1 17:43:09 2003
+++ edited/drivers/ieee1394/hosts.c	Fri Feb 21 19:45:45 2003
@@ -11,6 +11,7 @@
  */
 
 #include <linux/config.h>
+#include <linux/module.h>
 #include <linux/types.h>
 #include <linux/list.h>
 #include <linux/init.h>
@@ -69,8 +70,10 @@
         spin_lock_irqsave(&hosts_lock, flags);
         list_for_each(lh, &hosts) {
                 if (host == list_entry(lh, struct hpsb_host, host_list)) {
-			if (host->driver->devctl(host, MODIFY_USAGE, 1)) {
-				host->driver->devctl(host, MODIFY_USAGE, 1);
+			if (try_module_get(host->driver->owner)) {
+				/* we're doing this twice and don't seem
+				   to undo it..  --hch */
+				(void)try_module_get(host->driver->owner);
 				host->refcount++;
 				retval = 1;
 			}
@@ -95,7 +98,7 @@
 {
         unsigned long flags;
 
-        host->driver->devctl(host, MODIFY_USAGE, 0);
+	module_put(host->driver->owner);
 
         spin_lock_irqsave(&hosts_lock, flags);
         host->refcount--;
===== drivers/ieee1394/hosts.h 1.11 vs edited =====
--- 1.11/drivers/ieee1394/hosts.h	Fri Jan 31 01:48:22 2003
+++ edited/drivers/ieee1394/hosts.h	Fri Feb 21 19:45:45 2003
@@ -92,12 +92,6 @@
          * Return void. */
         CANCEL_REQUESTS,
 
-        /* Decrease host usage count if arg == 0, increase otherwise.  Return
-         * 1 for success, 0 for failure.  Increase usage may fail if the driver
-         * is in the process of shutting itself down.  Decrease usage can not
-         * fail. */
-        MODIFY_USAGE,
-
         /* Start or stop receiving isochronous channel in arg.  Return void.
          * This acts as an optimization hint, hosts are not required not to
          * listen on unrequested channels. */
@@ -147,6 +141,7 @@
 };
 
 struct hpsb_host_driver {
+	struct module *owner;
 	const char *name;
 
         /* This function must store a pointer to the configuration ROM into the
===== drivers/ieee1394/ohci1394.c 1.19 vs edited =====
--- 1.19/drivers/ieee1394/ohci1394.c	Sun Feb  2 08:01:36 2003
+++ edited/drivers/ieee1394/ohci1394.c	Fri Feb 21 19:45:45 2003
@@ -966,16 +966,6 @@
 		dma_trm_reset(&ohci->at_resp_context);
 		break;
 
-	case MODIFY_USAGE:
-		if (arg) {
-			if (try_module_get(THIS_MODULE))
-				retval = 1;
-		} else {
-			module_put(THIS_MODULE);
-			retval = 1;
-		}
-                break;
-
 	case ISO_LISTEN_CHANNEL:
         {
 		u64 mask;
@@ -3202,6 +3192,7 @@
 }
 
 static struct hpsb_host_driver ohci1394_driver = {
+	.owner =		THIS_MODULE,
 	.name =			OHCI1394_DRIVER_NAME,
 	.get_rom =		ohci_get_rom,
 	.transmit_packet =	ohci_transmit,
===== drivers/ieee1394/pcilynx.c 1.24 vs edited =====
--- 1.24/drivers/ieee1394/pcilynx.c	Tue Feb 11 00:00:08 2003
+++ edited/drivers/ieee1394/pcilynx.c	Fri Feb 21 19:45:45 2003
@@ -801,17 +801,6 @@
 
                 break;
 
-        case MODIFY_USAGE:
-		if (arg) {
-			if (try_module_get(THIS_MODULE))
-				retval = 1;
-		} else {
-			module_put(THIS_MODULE);
-			retval = 1;
-		}
-
-                break;
-
         case ISO_LISTEN_CHANNEL:
                 spin_lock_irqsave(&lynx->iso_rcv.lock, flags);
                 
@@ -1904,6 +1893,7 @@
 };
 
 static struct hpsb_host_driver lynx_driver = {
+	.owner =	   THIS_MODULE,
 	.name =		   PCILYNX_DRIVER_NAME,
         .get_rom =         get_lynx_rom,
         .transmit_packet = lynx_transmit,
===== drivers/pci/hotplug.c 1.10 vs edited =====
--- 1.10/drivers/pci/hotplug.c	Thu Feb  6 10:58:43 2003
+++ edited/drivers/pci/hotplug.c	Fri Feb 21 19:43:17 2003
@@ -173,44 +173,6 @@
 EXPORT_SYMBOL(pci_visit_dev);
 
 /**
- * pci_is_dev_in_use - query devices' usage
- * @dev: PCI device to query
- *
- * Queries whether a given PCI device is in use by a driver or not.
- * Returns 1 if the device is in use, 0 if it is not.
- */
-int pci_is_dev_in_use(struct pci_dev *dev)
-{
-	/* 
-	 * dev->driver will be set if the device is in use by a new-style 
-	 * driver -- otherwise, check the device's regions to see if any
-	 * driver has claimed them.
-	 */
-
-	int i;
-	int inuse = 0;
-
-	if (dev->driver) {
-		/* Assume driver feels responsible */
-		return 1;
-	}
-
-	for (i = 0; !dev->driver && !inuse && (i < 6); i++) {
-		if (!pci_resource_start(dev, i))
-			continue;
-		if (pci_resource_flags(dev, i) & IORESOURCE_IO) {
-			inuse = check_region(pci_resource_start(dev, i),
-					     pci_resource_len(dev, i));
-		} else if (pci_resource_flags(dev, i) & IORESOURCE_MEM) {
-			inuse = check_mem_region(pci_resource_start(dev, i),
-						 pci_resource_len(dev, i));
-		}
-	}
-	return inuse;
-}
-EXPORT_SYMBOL(pci_is_dev_in_use);
-
-/**
  * pci_remove_device_safe - remove an unused hotplug device
  * @dev: the device to remove
  *
@@ -221,9 +183,8 @@
  */
 int pci_remove_device_safe(struct pci_dev *dev)
 {
-	if (pci_is_dev_in_use(dev)) {
+	if (pci_dev_driver(dev))
 		return -EBUSY;
-	}
 	pci_remove_device(dev);
 	return 0;
 }
===== include/linux/ipmi_smi.h 1.1 vs edited =====
--- 1.1/include/linux/ipmi_smi.h	Tue Nov 26 23:06:25 2002
+++ edited/include/linux/ipmi_smi.h	Fri Feb 21 19:45:47 2003
@@ -78,6 +78,8 @@
 
 struct ipmi_smi_handlers
 {
+	struct module *owner;
+
 	/* Called to enqueue an SMI message to be sent.  This
 	   operation is not allowed to fail.  If an error occurs, it
 	   should report back the error in a received message.  It may
@@ -92,15 +94,6 @@
 	/* Called by the upper layer to request that we try to get
 	   events from the BMC we are attached to. */
 	void (*request_events)(void *send_info);
-
-	/* Called when someone is using the interface, so the module can
-	   adjust it's use count.  Return zero if successful, or an
-	   errno if not. */
-	int (*new_user)(void *send_info);
-
-	/* Called when someone is no longer using the interface, so the
-	   module can adjust it's use count. */
-	void (*user_left)(void *send_info);
 
 	/* Called when the interface should go into "run to
 	   completion" mode.  If this call sets the value to true, the
===== include/linux/sunrpc/auth.h 1.7 vs edited =====
--- 1.7/include/linux/sunrpc/auth.h	Sun Jan 12 22:40:31 2003
+++ edited/include/linux/sunrpc/auth.h	Fri Feb 21 19:45:47 2003
@@ -84,6 +84,7 @@
  * Client authentication ops
  */
 struct rpc_authops {
+	struct module		*owner;
 	rpc_authflavor_t	au_flavor;	/* flavor (RPC_AUTH_*) */
 #ifdef RPC_DEBUG
 	char *			au_name;
+++ edited/net/sunrpc/auth.c	Fri Feb 21 22:12:30 2003
@@ -8,6 +8,7 @@
 
 #include <linux/types.h>
 #include <linux/sched.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/socket.h>
@@ -65,6 +66,8 @@
 
 	if (flavor >= RPC_AUTH_MAXFLAVOR || !(ops = auth_flavors[flavor]))
 		return NULL;
+	if (!try_module_get(ops->owner))
+		return NULL;
 	clnt->cl_auth = ops->create(clnt, pseudoflavor);
 	return clnt->cl_auth;
 }
@@ -73,6 +76,8 @@
 rpcauth_destroy(struct rpc_auth *auth)
 {
 	auth->au_ops->destroy(auth);
+	module_put(auth->au_ops->owner);
+	kfree(auth);
 }
 
 static spinlock_t rpc_credcache_lock = SPIN_LOCK_UNLOCKED;
===== net/sunrpc/auth_null.c 1.9 vs edited =====
--- 1.9/net/sunrpc/auth_null.c	Sun Jan 12 22:40:13 2003
+++ edited/net/sunrpc/auth_null.c	Fri Feb 21 22:12:51 2003
@@ -8,6 +8,7 @@
 
 #include <linux/types.h>
 #include <linux/socket.h>
+#include <linux/module.h>
 #include <linux/in.h>
 #include <linux/utsname.h>
 #include <linux/sunrpc/clnt.h>
@@ -41,7 +42,6 @@
 {
 	dprintk("RPC: destroying NULL authenticator %p\n", auth);
 	rpcauth_free_credcache(auth);
-	kfree(auth);
 }
 
 /*
@@ -125,6 +125,7 @@
 }
 
 struct rpc_authops	authnull_ops = {
+	.owner		= THIS_MODULE,
 	.au_flavor	= RPC_AUTH_NULL,
 #ifdef RPC_DEBUG
 	.au_name	= "NULL",
===== net/sunrpc/auth_unix.c 1.9 vs edited =====
--- 1.9/net/sunrpc/auth_unix.c	Sun Jan 12 22:40:13 2003
+++ edited/net/sunrpc/auth_unix.c	Fri Feb 21 22:12:35 2003
@@ -8,6 +8,7 @@
 
 #include <linux/types.h>
 #include <linux/sched.h>
+#include <linux/module.h>
 #include <linux/socket.h>
 #include <linux/in.h>
 #include <linux/sunrpc/clnt.h>
@@ -60,7 +61,6 @@
 {
 	dprintk("RPC: destroying UNIX authenticator %p\n", auth);
 	rpcauth_free_credcache(auth);
-	kfree(auth);
 }
 
 static struct rpc_cred *
@@ -219,6 +219,7 @@
 }
 
 struct rpc_authops	authunix_ops = {
+	.owner		= THIS_MODULE,
 	.au_flavor	= RPC_AUTH_UNIX,
 #ifdef RPC_DEBUG
 	.au_name	= "UNIX",
===== net/sunrpc/auth_gss/auth_gss.c 1.2 vs edited =====
--- 1.2/net/sunrpc/auth_gss/auth_gss.c	Sun Jan 12 22:40:31 2003
+++ edited/net/sunrpc/auth_gss/auth_gss.c	Fri Feb 21 22:13:20 2003
@@ -438,8 +438,6 @@
 	struct rpc_auth * auth;
 
 	dprintk("RPC: creating GSS authenticator for client %p\n",clnt);
-	if (!try_module_get(THIS_MODULE))
-		return NULL;
 	if (!(gss_auth = kmalloc(sizeof(*gss_auth), GFP_KERNEL)))
 		goto out_dec;
 	gss_auth->mech = gss_pseudoflavor_to_mech(flavor);
@@ -470,7 +468,6 @@
 err_free:
 	kfree(gss_auth);
 out_dec:
-	module_put(THIS_MODULE);
 	return NULL;
 }
 
@@ -485,9 +482,6 @@
 	rpc_unlink(gss_auth->path);
 
 	rpcauth_free_credcache(auth);
-
-	kfree(auth);
-	module_put(THIS_MODULE);
 }
 
 /* gss_destroy_cred (and gss_destroy_ctx) are used to clean up after failure
@@ -691,6 +685,7 @@
 }
 
 static struct rpc_authops authgss_ops = {
+	.owner		= THIS_MODULE,
 	.au_flavor	= RPC_AUTH_GSS,
 #ifdef RPC_DEBUG
 	.au_name	= "RPCSEC_GSS",
===== sound/pci/rme9652/hammerfall_mem.c 1.11 vs edited =====
--- 1.11/sound/pci/rme9652/hammerfall_mem.c	Mon Jan 27 18:30:54 2003
+++ edited/sound/pci/rme9652/hammerfall_mem.c	Fri Feb 21 19:45:47 2003
@@ -150,8 +150,6 @@
 	for (i = 0; i < NBUFS; i++) {
 		rbuf = &hammerfall_buffers[i];
 		if (rbuf->flags == HAMMERFALL_BUF_ALLOCATED) {
-			if (! try_module_get(THIS_MODULE))
-				return NULL;
 			rbuf->flags |= HAMMERFALL_BUF_USED;
 			rbuf->pci = pcidev;
 			*dmaaddr = rbuf->addr;
@@ -171,7 +169,6 @@
 		rbuf = &hammerfall_buffers[i];
 		if (rbuf->buf == addr && rbuf->pci == pcidev) {
 			rbuf->flags &= ~HAMMERFALL_BUF_USED;
-			module_put(THIS_MODULE);
 			return;
 		}
 	}