| Message ID | 20231218202809.84253-2-marpagan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: improve protection against low-level control module unloading | expand |
On Mon, Dec 18, 2023 at 09:28:08PM +0100, Marco Pagani wrote: > Add a module owner field to the fpga_manager_ops struct and use it to > take the low-level control module's refcount instead of relying on the > parent device's driver pointer. Low-level control modules should > statically set the owner field to THIS_MODULE. To detect when the owner Don't be so strict, people could pass in any owner module they think it is correct. > module pointer becomes stale, set the mops pointer to null during > fpga_mgr_unregister() (called by the low-level module exit function) and No need the side note, people could call fpga_mgr_unregister() at any time they think it is correct. > test it before taking the module's refcount. Use a mutex to avoid a > crash that can happen if __fpga_mgr_get() gets suspended between testing > the mops pointer and taking the low-level refcount and then resumes when > the low-level module has already been freed. > > Thanks to Xu Yilun for suggesting the locking pattern. I appreciate that but don't put it in changelog. A Suggested-by is appropriate. > > Other changes: move put_device() from __fpga_mgr_get() to fpga_mgr_get() Opportunistically move put_device() ... The point is, try to imply the *Other* changes are simple and relative to the main change, or we should split the patch. Sorry but seeing the actual change, please split. The whole change is small and the put_device() part contributes 40% code ... Thanks, Yilun > and of_fpga_mgr_get() to improve code clarity. > > Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get") > Signed-off-by: Marco Pagani <marpagan@redhat.com> > --- > drivers/fpga/fpga-mgr.c | 50 ++++++++++++++++++++++++----------- > include/linux/fpga/fpga-mgr.h | 4 +++ > 2 files changed, 38 insertions(+), 16 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index 06651389c592..a32b7d40080d 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = { > }; > ATTRIBUTE_GROUPS(fpga_mgr); > > -static struct fpga_manager *__fpga_mgr_get(struct device *dev) > +static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev) > { > struct fpga_manager *mgr; > > - mgr = to_fpga_manager(dev); > + mgr = to_fpga_manager(mgr_dev); > > - if (!try_module_get(dev->parent->driver->owner)) > - goto err_dev; > + mutex_lock(&mgr->mops_mutex); > > - return mgr; > + if (!mgr->mops || !try_module_get(mgr->mops->owner)) > + mgr = ERR_PTR(-ENODEV); > > -err_dev: > - put_device(dev); > - return ERR_PTR(-ENODEV); > + mutex_unlock(&mgr->mops_mutex); > + > + return mgr; > } > > static int fpga_mgr_dev_match(struct device *dev, const void *data) > @@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data) > */ > struct fpga_manager *fpga_mgr_get(struct device *dev) > { > - struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, > - fpga_mgr_dev_match); > + struct fpga_manager *mgr; > + struct device *mgr_dev; > + > + mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match); > if (!mgr_dev) > return ERR_PTR(-ENODEV); > > - return __fpga_mgr_get(mgr_dev); > + mgr = __fpga_mgr_get(mgr_dev); > + if (IS_ERR(mgr)) > + put_device(mgr_dev); > + > + return mgr; > } > EXPORT_SYMBOL_GPL(fpga_mgr_get); > > @@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get); > */ > struct fpga_manager *of_fpga_mgr_get(struct device_node *node) > { > - struct device *dev; > + struct fpga_manager *mgr; > + struct device *mgr_dev; > > - dev = class_find_device_by_of_node(&fpga_mgr_class, node); > - if (!dev) > + mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node); > + if (!mgr_dev) > return ERR_PTR(-ENODEV); > > - return __fpga_mgr_get(dev); > + mgr = __fpga_mgr_get(mgr_dev); > + if (IS_ERR(mgr)) > + put_device(mgr_dev); > + > + return mgr; > } > EXPORT_SYMBOL_GPL(of_fpga_mgr_get); > > @@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get); > */ > void fpga_mgr_put(struct fpga_manager *mgr) > { > - module_put(mgr->dev.parent->driver->owner); > + module_put(mgr->mops->owner); > put_device(&mgr->dev); > } > EXPORT_SYMBOL_GPL(fpga_mgr_put); > @@ -803,6 +814,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in > } > > mutex_init(&mgr->ref_mutex); > + mutex_init(&mgr->mops_mutex); > > mgr->name = info->name; > mgr->mops = info->mops; > @@ -888,6 +900,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr) > */ > fpga_mgr_fpga_remove(mgr); > > + mutex_lock(&mgr->mops_mutex); > + > + mgr->mops = NULL; > + > + mutex_unlock(&mgr->mops_mutex); > + > device_unregister(&mgr->dev); > } > EXPORT_SYMBOL_GPL(fpga_mgr_unregister); > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index 54f63459efd6..b4d9413cb444 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -162,6 +162,7 @@ struct fpga_manager_info { > * @write_complete: set FPGA to operating state after writing is done > * @fpga_remove: optional: Set FPGA into a specific state during driver remove > * @groups: optional attribute groups. > + * @owner: owner module containing the ops. > * > * fpga_manager_ops are the low level functions implemented by a specific > * fpga manager driver. The optional ones are tested for NULL before being > @@ -184,6 +185,7 @@ struct fpga_manager_ops { > struct fpga_image_info *info); > void (*fpga_remove)(struct fpga_manager *mgr); > const struct attribute_group **groups; > + struct module *owner; > }; > > /* FPGA manager status: Partial/Full Reconfiguration errors */ > @@ -201,6 +203,7 @@ struct fpga_manager_ops { > * @state: state of fpga manager > * @compat_id: FPGA manager id for compatibility check. > * @mops: pointer to struct of fpga manager ops > + * @mops_mutex: protects mops from low-level module removal > * @priv: low level driver private date > */ > struct fpga_manager { > @@ -209,6 +212,7 @@ struct fpga_manager { > struct mutex ref_mutex; > enum fpga_mgr_states state; > struct fpga_compat_id *compat_id; > + struct mutex mops_mutex; > const struct fpga_manager_ops *mops; > void *priv; > }; > -- > 2.43.0 > >
On 2023-12-25 07:58, Xu Yilun wrote: > On Mon, Dec 18, 2023 at 09:28:08PM +0100, Marco Pagani wrote: >> Add a module owner field to the fpga_manager_ops struct and use it to >> take the low-level control module's refcount instead of relying on the >> parent device's driver pointer. Low-level control modules should >> statically set the owner field to THIS_MODULE. To detect when the owner > > Don't be so strict, people could pass in any owner module they think it > is correct. > I'm planning to use a helper macro to set the owner field at registration, as suggested by Greg K-H. So, I would change this sentence anyway. >> module pointer becomes stale, set the mops pointer to null during >> fpga_mgr_unregister() (called by the low-level module exit function) and > > No need the side note, people could call fpga_mgr_unregister() at any > time they think it is correct. > Okay, I'll drop this line. >> test it before taking the module's refcount. Use a mutex to avoid a >> crash that can happen if __fpga_mgr_get() gets suspended between testing >> the mops pointer and taking the low-level refcount and then resumes when >> the low-level module has already been freed. >> >> Thanks to Xu Yilun for suggesting the locking pattern. > > I appreciate that but don't put it in changelog. A Suggested-by is > appropriate. > Okay. >> >> Other changes: move put_device() from __fpga_mgr_get() to fpga_mgr_get() > > Opportunistically move put_device() ... > > The point is, try to imply the *Other* changes are simple and relative to > the main change, or we should split the patch. > > Sorry but seeing the actual change, please split. The whole change is > small and the put_device() part contributes 40% code ... > Make sense. I would prefer to rephrase the message and do everything in a single patch since moving put_device() is a small change related to the mechanism for getting the manager. Thanks, Marco >> and of_fpga_mgr_get() to improve code clarity. >> >> Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get") >> Signed-off-by: Marco Pagani <marpagan@redhat.com> >> --- >> drivers/fpga/fpga-mgr.c | 50 ++++++++++++++++++++++++----------- >> include/linux/fpga/fpga-mgr.h | 4 +++ >> 2 files changed, 38 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c >> index 06651389c592..a32b7d40080d 100644 >> --- a/drivers/fpga/fpga-mgr.c >> +++ b/drivers/fpga/fpga-mgr.c >> @@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = { >> }; >> ATTRIBUTE_GROUPS(fpga_mgr); >> >> -static struct fpga_manager *__fpga_mgr_get(struct device *dev) >> +static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev) >> { >> struct fpga_manager *mgr; >> >> - mgr = to_fpga_manager(dev); >> + mgr = to_fpga_manager(mgr_dev); >> >> - if (!try_module_get(dev->parent->driver->owner)) >> - goto err_dev; >> + mutex_lock(&mgr->mops_mutex); >> >> - return mgr; >> + if (!mgr->mops || !try_module_get(mgr->mops->owner)) >> + mgr = ERR_PTR(-ENODEV); >> >> -err_dev: >> - put_device(dev); >> - return ERR_PTR(-ENODEV); >> + mutex_unlock(&mgr->mops_mutex); >> + >> + return mgr; >> } >> >> static int fpga_mgr_dev_match(struct device *dev, const void *data) >> @@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data) >> */ >> struct fpga_manager *fpga_mgr_get(struct device *dev) >> { >> - struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, >> - fpga_mgr_dev_match); >> + struct fpga_manager *mgr; >> + struct device *mgr_dev; >> + >> + mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match); >> if (!mgr_dev) >> return ERR_PTR(-ENODEV); >> >> - return __fpga_mgr_get(mgr_dev); >> + mgr = __fpga_mgr_get(mgr_dev); >> + if (IS_ERR(mgr)) >> + put_device(mgr_dev); >> + >> + return mgr; >> } >> EXPORT_SYMBOL_GPL(fpga_mgr_get); >> >> @@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get); >> */ >> struct fpga_manager *of_fpga_mgr_get(struct device_node *node) >> { >> - struct device *dev; >> + struct fpga_manager *mgr; >> + struct device *mgr_dev; >> >> - dev = class_find_device_by_of_node(&fpga_mgr_class, node); >> - if (!dev) >> + mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node); >> + if (!mgr_dev) >> return ERR_PTR(-ENODEV); >> >> - return __fpga_mgr_get(dev); >> + mgr = __fpga_mgr_get(mgr_dev); >> + if (IS_ERR(mgr)) >> + put_device(mgr_dev); >> + >> + return mgr; >> } >> EXPORT_SYMBOL_GPL(of_fpga_mgr_get); >> >> @@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get); >> */ >> void fpga_mgr_put(struct fpga_manager *mgr) >> { >> - module_put(mgr->dev.parent->driver->owner); >> + module_put(mgr->mops->owner); >> put_device(&mgr->dev); >> } >> EXPORT_SYMBOL_GPL(fpga_mgr_put); >> @@ -803,6 +814,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in >> } >> >> mutex_init(&mgr->ref_mutex); >> + mutex_init(&mgr->mops_mutex); >> >> mgr->name = info->name; >> mgr->mops = info->mops; >> @@ -888,6 +900,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr) >> */ >> fpga_mgr_fpga_remove(mgr); >> >> + mutex_lock(&mgr->mops_mutex); >> + >> + mgr->mops = NULL; >> + >> + mutex_unlock(&mgr->mops_mutex); >> + >> device_unregister(&mgr->dev); >> } >> EXPORT_SYMBOL_GPL(fpga_mgr_unregister); >> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h >> index 54f63459efd6..b4d9413cb444 100644 >> --- a/include/linux/fpga/fpga-mgr.h >> +++ b/include/linux/fpga/fpga-mgr.h >> @@ -162,6 +162,7 @@ struct fpga_manager_info { >> * @write_complete: set FPGA to operating state after writing is done >> * @fpga_remove: optional: Set FPGA into a specific state during driver remove >> * @groups: optional attribute groups. >> + * @owner: owner module containing the ops. >> * >> * fpga_manager_ops are the low level functions implemented by a specific >> * fpga manager driver. The optional ones are tested for NULL before being >> @@ -184,6 +185,7 @@ struct fpga_manager_ops { >> struct fpga_image_info *info); >> void (*fpga_remove)(struct fpga_manager *mgr); >> const struct attribute_group **groups; >> + struct module *owner; >> }; >> >> /* FPGA manager status: Partial/Full Reconfiguration errors */ >> @@ -201,6 +203,7 @@ struct fpga_manager_ops { >> * @state: state of fpga manager >> * @compat_id: FPGA manager id for compatibility check. >> * @mops: pointer to struct of fpga manager ops >> + * @mops_mutex: protects mops from low-level module removal >> * @priv: low level driver private date >> */ >> struct fpga_manager { >> @@ -209,6 +212,7 @@ struct fpga_manager { >> struct mutex ref_mutex; >> enum fpga_mgr_states state; >> struct fpga_compat_id *compat_id; >> + struct mutex mops_mutex; >> const struct fpga_manager_ops *mops; >> void *priv; >> }; >> -- >> 2.43.0 >> >> >
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index 06651389c592..a32b7d40080d 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = { }; ATTRIBUTE_GROUPS(fpga_mgr); -static struct fpga_manager *__fpga_mgr_get(struct device *dev) +static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev) { struct fpga_manager *mgr; - mgr = to_fpga_manager(dev); + mgr = to_fpga_manager(mgr_dev); - if (!try_module_get(dev->parent->driver->owner)) - goto err_dev; + mutex_lock(&mgr->mops_mutex); - return mgr; + if (!mgr->mops || !try_module_get(mgr->mops->owner)) + mgr = ERR_PTR(-ENODEV); -err_dev: - put_device(dev); - return ERR_PTR(-ENODEV); + mutex_unlock(&mgr->mops_mutex); + + return mgr; } static int fpga_mgr_dev_match(struct device *dev, const void *data) @@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data) */ struct fpga_manager *fpga_mgr_get(struct device *dev) { - struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, - fpga_mgr_dev_match); + struct fpga_manager *mgr; + struct device *mgr_dev; + + mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match); if (!mgr_dev) return ERR_PTR(-ENODEV); - return __fpga_mgr_get(mgr_dev); + mgr = __fpga_mgr_get(mgr_dev); + if (IS_ERR(mgr)) + put_device(mgr_dev); + + return mgr; } EXPORT_SYMBOL_GPL(fpga_mgr_get); @@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get); */ struct fpga_manager *of_fpga_mgr_get(struct device_node *node) { - struct device *dev; + struct fpga_manager *mgr; + struct device *mgr_dev; - dev = class_find_device_by_of_node(&fpga_mgr_class, node); - if (!dev) + mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node); + if (!mgr_dev) return ERR_PTR(-ENODEV); - return __fpga_mgr_get(dev); + mgr = __fpga_mgr_get(mgr_dev); + if (IS_ERR(mgr)) + put_device(mgr_dev); + + return mgr; } EXPORT_SYMBOL_GPL(of_fpga_mgr_get); @@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get); */ void fpga_mgr_put(struct fpga_manager *mgr) { - module_put(mgr->dev.parent->driver->owner); + module_put(mgr->mops->owner); put_device(&mgr->dev); } EXPORT_SYMBOL_GPL(fpga_mgr_put); @@ -803,6 +814,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in } mutex_init(&mgr->ref_mutex); + mutex_init(&mgr->mops_mutex); mgr->name = info->name; mgr->mops = info->mops; @@ -888,6 +900,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr) */ fpga_mgr_fpga_remove(mgr); + mutex_lock(&mgr->mops_mutex); + + mgr->mops = NULL; + + mutex_unlock(&mgr->mops_mutex); + device_unregister(&mgr->dev); } EXPORT_SYMBOL_GPL(fpga_mgr_unregister); diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index 54f63459efd6..b4d9413cb444 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -162,6 +162,7 @@ struct fpga_manager_info { * @write_complete: set FPGA to operating state after writing is done * @fpga_remove: optional: Set FPGA into a specific state during driver remove * @groups: optional attribute groups. + * @owner: owner module containing the ops. * * fpga_manager_ops are the low level functions implemented by a specific * fpga manager driver. The optional ones are tested for NULL before being @@ -184,6 +185,7 @@ struct fpga_manager_ops { struct fpga_image_info *info); void (*fpga_remove)(struct fpga_manager *mgr); const struct attribute_group **groups; + struct module *owner; }; /* FPGA manager status: Partial/Full Reconfiguration errors */ @@ -201,6 +203,7 @@ struct fpga_manager_ops { * @state: state of fpga manager * @compat_id: FPGA manager id for compatibility check. * @mops: pointer to struct of fpga manager ops + * @mops_mutex: protects mops from low-level module removal * @priv: low level driver private date */ struct fpga_manager { @@ -209,6 +212,7 @@ struct fpga_manager { struct mutex ref_mutex; enum fpga_mgr_states state; struct fpga_compat_id *compat_id; + struct mutex mops_mutex; const struct fpga_manager_ops *mops; void *priv; };
Add a module owner field to the fpga_manager_ops struct and use it to take the low-level control module's refcount instead of relying on the parent device's driver pointer. Low-level control modules should statically set the owner field to THIS_MODULE. To detect when the owner module pointer becomes stale, set the mops pointer to null during fpga_mgr_unregister() (called by the low-level module exit function) and test it before taking the module's refcount. Use a mutex to avoid a crash that can happen if __fpga_mgr_get() gets suspended between testing the mops pointer and taking the low-level refcount and then resumes when the low-level module has already been freed. Thanks to Xu Yilun for suggesting the locking pattern. Other changes: move put_device() from __fpga_mgr_get() to fpga_mgr_get() and of_fpga_mgr_get() to improve code clarity. Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get") Signed-off-by: Marco Pagani <marpagan@redhat.com> --- drivers/fpga/fpga-mgr.c | 50 ++++++++++++++++++++++++----------- include/linux/fpga/fpga-mgr.h | 4 +++ 2 files changed, 38 insertions(+), 16 deletions(-)