On Wed, Aug 24, 2011 at 10:57:18PM +0000, KY Srinivasan wrote: > > > Like other bus specific devices in the kernel (pci devices, virtio > > > devices,), > > > class specific vmbus devices - struct storvsc_device and struct > > > netvsc_device > > > embed a pointer to the underlying struct hv_device. > > > > And when you save that pointer, you ARE incrementing the reference count > > properly, right? If not, you just caused a bug. > > Why do you say that. This assignment is done in the probe function where the > driver core is invoking the driver specific probe function.
But if you save a pointer, you HAVE to increment the reference count. > In the driver specific probe function, I allocate class specific > device state and embed the bus specific device pointer. So, I would > think the driver core is taking the appropriate reference count. What > I am doing is exactly what other PCI and virtio drivers are doing. For > instance, in virtio_blk.c , virtblk_probe() function, the > virtio_device pointer is stashed away in the virtio_blk structure in > exactly the same way I am doing. I suspect the assumption here is that > if probe succeeded, then the remove() function would be called to let > the driver cleanup the state. Yes, but that's a bug, the pointer reference count should be incremented. Look at drivers/usb/usb-skeleton.c for a well-documented way to handle a driver that works with a bus that has devices that could go away at any point in time. It handles the reference count, and the locking correctly, and has been audited by lots of people. > > > Furthermore, a pointer to the class specific device structure is > > > stashed in the struct hv_device (the ext pointer). > > > This is identical what is done in the virtio blk device - look at the > > > priv element in struct virtio_device. > > > > Yes, but the "base" structure of virtio_device does not contain a lock > > that the users of that priv pointer are using to control access to data > > _within_ the priv pointer, right? > > True. As I noted in an earlier email, the current hyper-v driver code has > logic > to deal with the race conditions I have described. It is just that the > current > implementation is completely bogus - look at for instance the function > must_get_stor_device() in storvsc.c. This function is invoked in the channel > callback code path to process messages coming from the host. I added this lock > to close the window in getting the ext pointer. Clearly the lock protecting > the > ext pointer must be in a structure whose persistence is guaranteed and that > is the reason > I put the lock in the struct hv_device. But no, that's not the way to do it, put it in the structure that has the data you are trying to protect. greg k-h _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
