Received-SPF: pass (google.com: domain of linux-kernel+bounces-136234-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 9 Apr 2024 12:34:04 +0800
From: Xu Yilun <yilun.xu@linux.intel.com>
To: Marco Pagani <marpagan@redhat.com>
Cc: Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
	Xu Yilun <yilun.xu@intel.com>, Tom Rix <trix@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Alan Tull <atull@opensource.altera.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Russ Weight <russ.weight@linux.dev>, linux-fpga@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] fpga: region: add owner module and take its refcount
Message-ID: <ZhTFPLWGkh0oRhL/@yilunxu-OptiPlex-7050>
References: <20240403144611.65507-1-marpagan@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240403144611.65507-1-marpagan@redhat.com>

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@linuxfoundation.org>
> Suggested-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Russ Weight <russ.weight@linux.dev>
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
> 
> 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.

>  
>  .. 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?

Thanks,
Yilun

>   * @priv: private data
>   * @get_bridges: optional function to get bridges to a list
>   */
> @@ -46,6 +47,7 @@ struct fpga_region {
>  	struct fpga_manager *mgr;
>  	struct fpga_image_info *info;
>  	struct fpga_compat_id *compat_id;
> +	struct module *br_owner;
>  	void *priv;
>  	int (*get_bridges)(struct fpga_region *region);
>  };
> @@ -58,12 +60,17 @@ fpga_region_class_find(struct device *start, const void *data,
>  
>  int fpga_region_program_fpga(struct fpga_region *region);
>  
> +#define fpga_region_register_full(parent, info) \
> +	__fpga_region_register_full(parent, info, THIS_MODULE)
>  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);
>  
> +#define fpga_region_register(parent, mgr, get_bridges) \
> +	__fpga_region_register(parent, mgr, get_bridges, THIS_MODULE)
>  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);
>  void fpga_region_unregister(struct fpga_region *region);
>  
>  #endif /* _FPGA_REGION_H */
> 
> base-commit: b1a91ca25f15b6d7b311de4465854a5981dee3d3
> -- 
> 2.44.0
> 
>