| Message ID | 20200518024510.14115-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce vendor ops in vfio-pci | expand |
On Sun, 17 May 2020 22:45:10 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote: > vendor modules call macro module_vfio_pci_register_vendor_handler to > generate module_init and module_exit. > It is necessary to ensure that vendor modules always call > vfio_pci_register_vendor_driver() on driver loading and > vfio_pci_unregister_vendor_driver on driver unloading, > because > (1) at compiling time, there's only a dependency of vendor modules on > vfio_pci. > (2) at runtime, > - vendor modules add refs of vfio_pci on a successful calling of > vfio_pci_register_vendor_driver() and deref of vfio_pci on a > successful calling of vfio_pci_unregister_vendor_driver(). > - vfio_pci only adds refs of vendor module on a successful probe of vendor > driver. > vfio_pci derefs vendor module when unbinding from a device. > > So, after vfio_pci is unbound from a device, the vendor module to that > device is free to get unloaded. However, if that vendor module does not > call vfio_pci_unregister_vendor_driver() in its module_exit, vfio_pci may > hold a stale pointer to vendor module. > > Cc: Kevin Tian <kevin.tian@intel.com> > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > --- > include/linux/vfio.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 3e53deb012b6..f3746608c2d9 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -223,4 +223,31 @@ struct vfio_pci_vendor_driver_ops { > }; > int __vfio_pci_register_vendor_driver(struct vfio_pci_vendor_driver_ops *ops); > void vfio_pci_unregister_vendor_driver(struct vfio_device_ops *device_ops); > + > +#define vfio_pci_register_vendor_driver(__name, __probe, __remove, \ > + __device_ops) \ > +static struct vfio_pci_vendor_driver_ops __ops ## _node = { \ > + .owner = THIS_MODULE, \ > + .name = __name, \ > + .probe = __probe, \ > + .remove = __remove, \ > + .device_ops = __device_ops, \ > +}; \ > +__vfio_pci_register_vendor_driver(&__ops ## _node) > + > +#define module_vfio_pci_register_vendor_handler(name, probe, remove, \ > + device_ops) \ > +static int __init device_ops ## _module_init(void) \ > +{ \ > + vfio_pci_register_vendor_driver(name, probe, remove, \ > + device_ops); \ What if this function fails (e.g. with -ENOMEM)? > + return 0; \ > +}; \ > +static void __exit device_ops ## _module_exit(void) \ > +{ \ > + vfio_pci_unregister_vendor_driver(device_ops); \ > +}; \ > +module_init(device_ops ## _module_init); \ > +module_exit(device_ops ## _module_exit) > + > #endif /* VFIO_H */
On Thu, Jun 04, 2020 at 05:01:06PM +0200, Cornelia Huck wrote: > On Sun, 17 May 2020 22:45:10 -0400 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > vendor modules call macro module_vfio_pci_register_vendor_handler to > > generate module_init and module_exit. > > It is necessary to ensure that vendor modules always call > > vfio_pci_register_vendor_driver() on driver loading and > > vfio_pci_unregister_vendor_driver on driver unloading, > > because > > (1) at compiling time, there's only a dependency of vendor modules on > > vfio_pci. > > (2) at runtime, > > - vendor modules add refs of vfio_pci on a successful calling of > > vfio_pci_register_vendor_driver() and deref of vfio_pci on a > > successful calling of vfio_pci_unregister_vendor_driver(). > > - vfio_pci only adds refs of vendor module on a successful probe of vendor > > driver. > > vfio_pci derefs vendor module when unbinding from a device. > > > > So, after vfio_pci is unbound from a device, the vendor module to that > > device is free to get unloaded. However, if that vendor module does not > > call vfio_pci_unregister_vendor_driver() in its module_exit, vfio_pci may > > hold a stale pointer to vendor module. > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > --- > > include/linux/vfio.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index 3e53deb012b6..f3746608c2d9 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -223,4 +223,31 @@ struct vfio_pci_vendor_driver_ops { > > }; > > int __vfio_pci_register_vendor_driver(struct vfio_pci_vendor_driver_ops *ops); > > void vfio_pci_unregister_vendor_driver(struct vfio_device_ops *device_ops); > > + > > +#define vfio_pci_register_vendor_driver(__name, __probe, __remove, \ > > + __device_ops) \ > > +static struct vfio_pci_vendor_driver_ops __ops ## _node = { \ > > + .owner = THIS_MODULE, \ > > + .name = __name, \ > > + .probe = __probe, \ > > + .remove = __remove, \ > > + .device_ops = __device_ops, \ > > +}; \ > > +__vfio_pci_register_vendor_driver(&__ops ## _node) > > + > > +#define module_vfio_pci_register_vendor_handler(name, probe, remove, \ > > + device_ops) \ > > +static int __init device_ops ## _module_init(void) \ > > +{ \ > > + vfio_pci_register_vendor_driver(name, probe, remove, \ > > + device_ops); \ > > What if this function fails (e.g. with -ENOMEM)? > right. I need to return error in that case. Thanks for pointing it out! Yan > > + return 0; \ > > +}; \ > > +static void __exit device_ops ## _module_exit(void) \ > > +{ \ > > + vfio_pci_unregister_vendor_driver(device_ops); \ > > +}; \ > > +module_init(device_ops ## _module_init); \ > > +module_exit(device_ops ## _module_exit) > > + > > #endif /* VFIO_H */ >
diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 3e53deb012b6..f3746608c2d9 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -223,4 +223,31 @@ struct vfio_pci_vendor_driver_ops { }; int __vfio_pci_register_vendor_driver(struct vfio_pci_vendor_driver_ops *ops); void vfio_pci_unregister_vendor_driver(struct vfio_device_ops *device_ops); + +#define vfio_pci_register_vendor_driver(__name, __probe, __remove, \ + __device_ops) \ +static struct vfio_pci_vendor_driver_ops __ops ## _node = { \ + .owner = THIS_MODULE, \ + .name = __name, \ + .probe = __probe, \ + .remove = __remove, \ + .device_ops = __device_ops, \ +}; \ +__vfio_pci_register_vendor_driver(&__ops ## _node) + +#define module_vfio_pci_register_vendor_handler(name, probe, remove, \ + device_ops) \ +static int __init device_ops ## _module_init(void) \ +{ \ + vfio_pci_register_vendor_driver(name, probe, remove, \ + device_ops); \ + return 0; \ +}; \ +static void __exit device_ops ## _module_exit(void) \ +{ \ + vfio_pci_unregister_vendor_driver(device_ops); \ +}; \ +module_init(device_ops ## _module_init); \ +module_exit(device_ops ## _module_exit) + #endif /* VFIO_H */
vendor modules call macro module_vfio_pci_register_vendor_handler to generate module_init and module_exit. It is necessary to ensure that vendor modules always call vfio_pci_register_vendor_driver() on driver loading and vfio_pci_unregister_vendor_driver on driver unloading, because (1) at compiling time, there's only a dependency of vendor modules on vfio_pci. (2) at runtime, - vendor modules add refs of vfio_pci on a successful calling of vfio_pci_register_vendor_driver() and deref of vfio_pci on a successful calling of vfio_pci_unregister_vendor_driver(). - vfio_pci only adds refs of vendor module on a successful probe of vendor driver. vfio_pci derefs vendor module when unbinding from a device. So, after vfio_pci is unbound from a device, the vendor module to that device is free to get unloaded. However, if that vendor module does not call vfio_pci_unregister_vendor_driver() in its module_exit, vfio_pci may hold a stale pointer to vendor module. Cc: Kevin Tian <kevin.tian@intel.com> Suggested-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- include/linux/vfio.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)