Re: [PATCH v4] fpga: region: add owner module and take its refcount

From: Xu Yilun
Date: Thu Apr 11 2024 - 04:56:48 EST

On Wed, Apr 10, 2024 at 11:49:34AM +0200, Marco Pagani wrote:
>
>
> On 2024-04-09 06:34, Xu Yilun wrote:
> > On Wed, Apr 03, 2024 at 04:46:09PM +0200, Marco Pagani wrote:
> >> The current implementation of the fpga region assumes that the low-level
> >> module registers a driver for the parent device and uses its owner pointer
> >> to take the module's refcount. This approach is problematic since it can
> >> lead to a null pointer dereference while attempting to get the region
> >> during programming if the parent device does not have a driver.
> >>
> >> To address this problem, add a module owner pointer to the fpga_region
> >> struct and use it to take the module's refcount. Modify the functions for
> >> registering a region to take an additional owner module parameter and
> >> rename them to avoid conflicts. Use the old function names for helper
> >> macros that automatically set the module that registers the region as the
> >> owner. This ensures compatibility with existing low-level control modules
> >> and reduces the chances of registering a region without setting the owner.
> >>
> >> Also, update the documentation to keep it consistent with the new interface
> >> for registering an fpga region.
> >>
> >> Fixes: 0fa20cdfcc1f ("fpga: fpga-region: device tree control for FPGA")
> >> Suggested-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> Suggested-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> >> Reviewed-by: Russ Weight <russ.weight@xxxxxxxxx>
> >> Signed-off-by: Marco Pagani <marpagan@xxxxxxxxxx>
> >> ---
> >>
> >> v4:
> >> - Split out the swap between put_device() and mutex_unlock() while
> >> releasing the region to avoid potential use after release issues
> >> v3:
> >> - Add reviewed-by Russ Weight
> >> v2:
> >> - Fixed typo in the documentation sets -> set
> >> - Renamed owner pointer get_br_owner -> br_owner
> >> ---
> >> Documentation/driver-api/fpga/fpga-region.rst | 13 ++++++----
> >> drivers/fpga/fpga-region.c | 24 +++++++++++--------
> >> include/linux/fpga/fpga-region.h | 13 +++++++---
> >> 3 files changed, 32 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/Documentation/driver-api/fpga/fpga-region.rst b/Documentation/driver-api/fpga/fpga-region.rst
> >> index dc55d60a0b4a..77190a5ef330 100644
> >> --- a/Documentation/driver-api/fpga/fpga-region.rst
> >> +++ b/Documentation/driver-api/fpga/fpga-region.rst
> >> @@ -46,13 +46,16 @@ API to add a new FPGA region
> >> ----------------------------
> >>
> >> * struct fpga_region - The FPGA region struct
> >> -* struct fpga_region_info - Parameter structure for fpga_region_register_full()
> >> -* fpga_region_register_full() - Create and register an FPGA region using the
> >> +* struct fpga_region_info - Parameter structure for __fpga_region_register_full()
> >> +* __fpga_region_register_full() - Create and register an FPGA region using the
> >> fpga_region_info structure to provide the full flexibility of options
> >> -* fpga_region_register() - Create and register an FPGA region using standard
> >> +* __fpga_region_register() - Create and register an FPGA region using standard
> >> arguments
> >> * fpga_region_unregister() - Unregister an FPGA region
> >>
> >> +Helper macros ``fpga_region_register()`` and ``fpga_region_register_full()``
> >> +automatically set the module that registers the FPGA region as the owner.
> >> +
> >> The FPGA region's probe function will need to get a reference to the FPGA
> >> Manager it will be using to do the programming. This usually would happen
> >> during the region's probe function.
> >> @@ -82,10 +85,10 @@ following APIs to handle building or tearing down that list.
> >> :functions: fpga_region_info
> >>
> >> .. kernel-doc:: drivers/fpga/fpga-region.c
> >> - :functions: fpga_region_register_full
> >> + :functions: __fpga_region_register
> >>
> >> .. kernel-doc:: drivers/fpga/fpga-region.c
> >> - :functions: fpga_region_register
> >> + :functions: __fpga_region_register_full
> >
> > Why you swap the order? You didn't do that for fpga-mgr.
>
> Ouch, it's a typo.
>
> >>
> >> .. kernel-doc:: drivers/fpga/fpga-region.c
> >> :functions: fpga_region_unregister
> >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> >> index b364a929425c..d50ab1509989 100644
> >> --- a/drivers/fpga/fpga-region.c
> >> +++ b/drivers/fpga/fpga-region.c
> >> @@ -53,7 +53,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
> >> }
> >>
> >> get_device(dev);
> >> - if (!try_module_get(dev->parent->driver->owner)) {
> >> + if (!try_module_get(region->br_owner)) {
> >> put_device(dev);
> >> mutex_unlock(&region->mutex);
> >> return ERR_PTR(-ENODEV);
> >> @@ -75,7 +75,7 @@ static void fpga_region_put(struct fpga_region *region)
> >>
> >> dev_dbg(dev, "put\n");
> >>
> >> - module_put(dev->parent->driver->owner);
> >> + module_put(region->br_owner);
> >> put_device(dev);
> >> mutex_unlock(&region->mutex);
> >> }
> >> @@ -181,14 +181,16 @@ static struct attribute *fpga_region_attrs[] = {
> >> ATTRIBUTE_GROUPS(fpga_region);
> >>
> >> /**
> >> - * fpga_region_register_full - create and register an FPGA Region device
> >> + * __fpga_region_register_full - create and register an FPGA Region device
> >> * @parent: device parent
> >> * @info: parameters for FPGA Region
> >> + * @owner: owner module containing the get_bridges function
> >> *
> >> * Return: struct fpga_region or ERR_PTR()
> >> */
> >> struct fpga_region *
> >> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
> >> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
> >> + struct module *owner)
> >> {
> >> struct fpga_region *region;
> >> int id, ret = 0;
> >> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> >> region->compat_id = info->compat_id;
> >> region->priv = info->priv;
> >> region->get_bridges = info->get_bridges;
> >> + region->br_owner = owner;
> >>
> >> mutex_init(&region->mutex);
> >> INIT_LIST_HEAD(&region->bridge_list);
> >> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> >>
> >> return ERR_PTR(ret);
> >> }
> >> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
> >>
> >> /**
> >> - * fpga_region_register - create and register an FPGA Region device
> >> + * __fpga_region_register - create and register an FPGA Region device
> >> * @parent: device parent
> >> * @mgr: manager that programs this region
> >> * @get_bridges: optional function to get bridges to a list
> >> + * @owner: owner module containing get_bridges function
> >> *
> >> * This simple version of the register function should be sufficient for most users.
> >> * The fpga_region_register_full() function is available for users that need to
> >> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >> * Return: struct fpga_region or ERR_PTR()
> >> */
> >> struct fpga_region *
> >> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >> - int (*get_bridges)(struct fpga_region *))
> >> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >> + int (*get_bridges)(struct fpga_region *), struct module *owner)
> >> {
> >> struct fpga_region_info info = { 0 };
> >>
> >> info.mgr = mgr;
> >> info.get_bridges = get_bridges;
> >>
> >> - return fpga_region_register_full(parent, &info);
> >> + return __fpga_region_register_full(parent, &info, owner);
> >> }
> >> -EXPORT_SYMBOL_GPL(fpga_region_register);
> >> +EXPORT_SYMBOL_GPL(__fpga_region_register);
> >>
> >> /**
> >> * fpga_region_unregister - unregister an FPGA region
> >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> >> index 9d4d32909340..d175babc3d68 100644
> >> --- a/include/linux/fpga/fpga-region.h
> >> +++ b/include/linux/fpga/fpga-region.h
> >> @@ -36,6 +36,7 @@ struct fpga_region_info {
> >> * @mgr: FPGA manager
> >> * @info: FPGA image info
> >> * @compat_id: FPGA region id for compatibility check.
> >> + * @br_owner: module containing the get_bridges function
> >
> > I'm a little confused that you call it br_owner, just because there is
> > only one get_bridge() callback provided by low-level module. If we
> > further have another callback, the name & all the doc would be a problem.
> > And It is really the owner of the region module, not the bridge module.
> >
> > Maybe just name it owner, or ops_owner?
>
> Right, it makes sense to me. How about rg_ops_owner for symmetry with
> the manager and bridge?

I think ops_owner is better. First it is a member for region related
structure, no need to emphasize the 'region' again. Second I didn't see
any 'rg' in fpga core files, I think it is less understandable to
everyone.

'br' is more widely used, so it's OK.

Thanks,
Yilun.

>
>
> Thanks,
> Marco
>
>