[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-fpga
Subject:    Re: [PATCH] fpga: region: add owner module and take its refcount
From:       Marco Pagani <marpagan () redhat ! com>
Date:       2024-03-26 18:58:53
Message-ID: befc8ccf-0661-4c6f-b262-3dab3c34e0be () redhat ! com
[Download RAW message or body]



On 2024-03-26 18:28, Russ Weight wrote:
> 
> On Fri, Mar 22, 2024 at 06:19:30PM +0100, 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.
> > 
> > Other changes: unlock the mutex before calling put_device() in
> > fpga_region_put() to avoid potential use after release issues.
> > 
> > Fixes: 0fa20cdfcc1f ("fpga: fpga-region: device tree control for FPGA")
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Suggested-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Marco Pagani <marpagan@redhat.com>
> > ---
> > Documentation/driver-api/fpga/fpga-region.rst | 13 ++++++----
> > drivers/fpga/fpga-region.c                    | 26 +++++++++++--------
> > include/linux/fpga/fpga-region.h              | 13 +++++++---
> > 3 files changed, 33 insertions(+), 19 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/fpga/fpga-region.rst \
> > b/Documentation/driver-api/fpga/fpga-region.rst index dc55d60a0b4a..3aff5199b6d8 \
> >                 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 sets the module that registers the FPGA region as the owner.
> 
> s/sets/set/

Nice catch.

> 
> > +
> > 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
> > 
> > .. 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..f8bbda024d59 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->get_br_owner)) {
> > 		put_device(dev);
> > 		mutex_unlock(&region->mutex);
> > 		return ERR_PTR(-ENODEV);
> > @@ -75,9 +75,9 @@ static void fpga_region_put(struct fpga_region *region)
> > 
> > 	dev_dbg(dev, "put\n");
> > 
> > -	module_put(dev->parent->driver->owner);
> > -	put_device(dev);
> > +	module_put(region->get_br_owner);
> > 	mutex_unlock(&region->mutex);
> > +	put_device(dev);
> > }
> > 
> > /**
> > @@ -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->get_br_owner = owner;
> 
> get_* implies a function. Maybe this would be better called br_owner?

You are right. I will change it to br_owner in v2.

> 
> - Russ
> 

[ ... ]
Thanks,
Marco


[prev in list] [next in list] [prev in thread] [next in thread]