linux-kernel - Re: [PATCH 5/5] staging: vme: make match() driver specific to improve non-VME64x support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 13 Aug 2011 04:50:14 -0400
From:	"Emilio G. Cota" <cota@...ap.org>
To:	Manohar Vanga <manohar.vanga@...n.ch>
Cc:	martyn.welch@...com, gregkh@...e.de, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] staging: vme: make match() driver specific to
 improve non-VME64x support

On Fri, Aug 12, 2011 at 12:30:51 +0200, Manohar Vanga wrote:
> +++ b/drivers/staging/vme/vme.c
(snip)
> +static int __vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
>  {
> -	int i;
> -	struct vme_dev *vdev;
> -
> +	struct vme_bridge *bridge;
> +	int err = 0;
>  
> -	for (i = 0; i < VME_SLOTS_MAX; i++) {
> -		vdev = &bridge->dev[i];
> -		device_unregister(&vdev->dev);
> +	mutex_lock(&vme_buses_lock);
> +	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> +		/*
> +		 * We increase the refcount of the bridge module here to
> +		 * prevent it from being removed during driver registration
> +		 */
> +		if (!vme_bridge_get(bridge->num))
> +			continue;

hmm have you tested this? It should deadlock, because as in
patch 3 vme_bridge_get() acquires vme_buses_lock.

An alternative is to call here try_module_get() directly on
bridge->owner, which would succeed in preventing it from being
removed (the lock is held 

> +		mutex_unlock(&vme_buses_lock);
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		mutex_lock(&vme_buses_lock);
> +		vme_bridge_put(bridge);

This, interestingly, wouldn't deadlock, because we pass the bridge
directly. See my second message to patch 3.

> +		if (err)
> +			break;
>  	}
> -	vme_remove_bus(bridge);
> +	mutex_unlock(&vme_buses_lock);
> +	return err;
>  }

The whole loop is admittedly complex. IIRC in my original patch
module_get/put were called here directly, and vme_buses_lock
was unlocked before calling __vme_register_driver_bus()
to avoid a deadlock, because within that function the .probe
methods of the driver would likely call vme_bridge_get().

Now that we don't export them, the loop could be simplified to:

> +	mutex_lock(&vme_buses_lock);
> +	list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> +		err = __vme_register_driver_bus(drv, bridge, ndevs);
> +		if (err)
> +			break;
>  	}
> +	mutex_unlock(&vme_buses_lock);

This cannot race with a bridge being removed. Let's see how:
If the bridge driver is sane, it will call vme_unregister_bridge()
in its .release method. In there vme_remove_bus is called, and
the thread will try to acquire vme_buses_lock, which is already
held by above loop. Coming back to the loop, the try_get_module
call in vme_bus_probe will fail, because the bridge module
is being removed, and as a result all the devices under that
bridge won't be installed--this is what we wanted.

When the loop finishes we unlock vme_buses_lock and the
removal of the bus completes.

That said, I would ONLY take the simplified loop if a comment was
added to explain the above race. And I'd add that comment
near vme_bus_get/put, because if those are exported one
day, the above loop would need be changed accordingly.

		Emilio

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/