LinuxLists.cc - [PATCH v2] fpga: region: add owner module and take its refcount

2024-03-27 16:01:57

Subject: [PATCH v2] fpga: region: add owner module and take its refcount

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 <[email protected]>
Suggested-by: Xu Yilun <[email protected]>
Signed-off-by: Marco Pagani <[email protected]>
---

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

.. 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..1beb7415c2dc 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,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->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->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
* @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

2024-03-27 19:34:22

by Russ Weight

[permalink] [raw]

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

On Wed, Mar 27, 2024 at 05:00:20PM +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 <[email protected]>
> Suggested-by: Xu Yilun <[email protected]>

Reviewed-by: Russ Weight <[email protected]>

> Signed-off-by: Marco Pagani <[email protected]>
> ---
>
> 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 | 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..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
>
> .. 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..1beb7415c2dc 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,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->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->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
> * @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
>

2024-04-01 09:39:06

by Xu Yilun

[permalink] [raw]

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

2024-04-03 13:39:03

by Marco Pagani

[permalink] [raw]

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

On 2024-04-01 11:34, Xu Yilun wrote:
> On Wed, Mar 27, 2024 at 05:00:20PM +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.
>
> Please try not to mix different changes in one patch, especially for
> a "bug fix" as you said.

You are right. I'll split out the change and eventually send it as a
separate patch.

> And I do have concern about the fix, see below.
>
> [...]
>
>> @@ -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,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->br_owner);
>> mutex_unlock(&region->mutex);
>
> If there is concern the region would be freed after put_device(), then
> why still keep the sequence in fpga_region_get()?

Ouch, sorry, I forgot to make the change also in fpga_region_get().

> And is it possible region is freed before get_device() in
> fpga_region_get()?

If the user follows the usual pattern (i.e., waiting for
fpga_region_program_fpga() to complete before calling
fpga_region_unregister()) there should be no problem. However, I think
releasing the device before unlocking the mutex contained in the context
associated with the device makes the code brittle and more prone to
problems.

> Or we should clearly document how/when to use these functions?

I think it is not necessary to change the documentation since the
in-kernel programming API will not be affected by the change.

Thanks,
Marco

2024-04-09 04:13:33

by Xu Yilun

[permalink] [raw]

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

On Wed, Apr 03, 2024 at 03:34:22PM +0200, Marco Pagani wrote:
>
>
> On 2024-04-01 11:34, Xu Yilun wrote:
> > On Wed, Mar 27, 2024 at 05:00:20PM +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.
> >
> > Please try not to mix different changes in one patch, especially for
> > a "bug fix" as you said.
>
> You are right. I'll split out the change and eventually send it as a
> separate patch.
>
> > And I do have concern about the fix, see below.
> >
> > [...]
> >
> >> @@ -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,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->br_owner);
> >> mutex_unlock(&region->mutex);
> >
> > If there is concern the region would be freed after put_device(), then
> > why still keep the sequence in fpga_region_get()?
>
> Ouch, sorry, I forgot to make the change also in fpga_region_get().
>
> > And is it possible region is freed before get_device() in
> > fpga_region_get()?
>
> If the user follows the usual pattern (i.e., waiting for

I can see the only safe way is fpga_region_program_fpga() or fpga_region_get()
should be included in:

region = fpga_region_class_find();
...
put_device(&region->dev);

That is to say, fpga_region_get() should not be called when there is no
region dev reference hold beforehand. In this case, no use after release
risk. That's why I was thinking about some documentation.

Another concern is we'd better keep the get/put operations symmetrical
for easy maintaining, as long as it doesn't cause problem.

Thanks,
Yilun

> fpga_region_program_fpga() to complete before calling
> fpga_region_unregister()) there should be no problem. However, I think
> releasing the device before unlocking the mutex contained in the context
> associated with the device makes the code brittle and more prone to
> problems.
>
> > Or we should clearly document how/when to use these functions?
>
> I think it is not necessary to change the documentation since the
> in-kernel programming API will not be affected by the change.
>
> Thanks,
> Marco
>

2024-04-10 09:42:42

by Marco Pagani

[permalink] [raw]

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

On 2024-04-09 06:08, Xu Yilun wrote:
> On Wed, Apr 03, 2024 at 03:34:22PM +0200, Marco Pagani wrote:
>>
>>
>> On 2024-04-01 11:34, Xu Yilun wrote:
>>> On Wed, Mar 27, 2024 at 05:00:20PM +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.
>>>
>>> Please try not to mix different changes in one patch, especially for
>>> a "bug fix" as you said.
>>
>> You are right. I'll split out the change and eventually send it as a
>> separate patch.
>>
>>> And I do have concern about the fix, see below.
>>>
>>> [...]
>>>
>>>> @@ -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,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->br_owner);
>>>> mutex_unlock(&region->mutex);
>>>
>>> If there is concern the region would be freed after put_device(), then
>>> why still keep the sequence in fpga_region_get()?
>>
>> Ouch, sorry, I forgot to make the change also in fpga_region_get().
>>
>>> And is it possible region is freed before get_device() in
>>> fpga_region_get()?
>>
>> If the user follows the usual pattern (i.e., waiting for
>
> I can see the only safe way is fpga_region_program_fpga() or fpga_region_get()
> should be included in:
>
> region = fpga_region_class_find();
> ...
> put_device(&region->dev);
>
> That is to say, fpga_region_get() should not be called when there is no
> region dev reference hold beforehand. In this case, no use after release
> risk. That's why I was thinking about some documentation.
>
> Another concern is we'd better keep the get/put operations symmetrical
> for easy maintaining, as long as it doesn't cause problem.

Now I see your point. So, you suggest changing only the docs to clarify
that the region must be taken with fpga_region_class_find() before
programming it with fpga_region_program_fpga()?

That's fine by me. However, this made me wonder why we need to take the
region dev with get_device() in fpga_region_program_fpga()->fpga_region_get().
If we assume that the user must always call fpga_region_class_find()
before programming with fpga_region_program_fpga(), why do we need the
double get?

Thanks,
Marco

>> fpga_region_program_fpga() to complete before calling
>> fpga_region_unregister()) there should be no problem. However, I think
>> releasing the device before unlocking the mutex contained in the context
>> associated with the device makes the code brittle and more prone to
>> problems.
>>
>>> Or we should clearly document how/when to use these functions?
>>
>> I think it is not necessary to change the documentation since the
>> in-kernel programming API will not be affected by the change.
>>
[...]

2024-04-11 11:42:55

by Xu Yilun

[permalink] [raw]

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

On Wed, Apr 10, 2024 at 11:42:23AM +0200, Marco Pagani wrote:
>
>
> On 2024-04-09 06:08, Xu Yilun wrote:
> > On Wed, Apr 03, 2024 at 03:34:22PM +0200, Marco Pagani wrote:
> >>
> >>
> >> On 2024-04-01 11:34, Xu Yilun wrote:
> >>> On Wed, Mar 27, 2024 at 05:00:20PM +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.
> >>>
> >>> Please try not to mix different changes in one patch, especially for
> >>> a "bug fix" as you said.
> >>
> >> You are right. I'll split out the change and eventually send it as a
> >> separate patch.
> >>
> >>> And I do have concern about the fix, see below.
> >>>
> >>> [...]
> >>>
> >>>> @@ -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,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->br_owner);
> >>>> mutex_unlock(&region->mutex);
> >>>
> >>> If there is concern the region would be freed after put_device(), then
> >>> why still keep the sequence in fpga_region_get()?
> >>
> >> Ouch, sorry, I forgot to make the change also in fpga_region_get().
> >>
> >>> And is it possible region is freed before get_device() in
> >>> fpga_region_get()?
> >>
> >> If the user follows the usual pattern (i.e., waiting for
> >
> > I can see the only safe way is fpga_region_program_fpga() or fpga_region_get()
> > should be included in:
> >
> > region = fpga_region_class_find();
> > ...
> > put_device(&region->dev);
> >
> > That is to say, fpga_region_get() should not be called when there is no
> > region dev reference hold beforehand. In this case, no use after release
> > risk. That's why I was thinking about some documentation.
> >
> > Another concern is we'd better keep the get/put operations symmetrical
> > for easy maintaining, as long as it doesn't cause problem.
>
> Now I see your point. So, you suggest changing only the docs to clarify
> that the region must be taken with fpga_region_class_find() before
> programming it with fpga_region_program_fpga()?

Like:

The reference to the region must already been hold. E.g. by
fpga_region_class_find().

>
> That's fine by me. However, this made me wonder why we need to take the
> region dev with get_device() in fpga_region_program_fpga()->fpga_region_get().
> If we assume that the user must always call fpga_region_class_find()
> before programming with fpga_region_program_fpga(), why do we need the
> double get?

Yeah, I have the same concern when I visit this part. I don't think it
is necessary.

Thanks,
Yilun

>
> Thanks,
> Marco
>
> >> fpga_region_program_fpga() to complete before calling
> >> fpga_region_unregister()) there should be no problem. However, I think
> >> releasing the device before unlocking the mutex contained in the context
> >> associated with the device makes the code brittle and more prone to
> >> problems.
> >>
> >>> Or we should clearly document how/when to use these functions?
> >>
> >> I think it is not necessary to change the documentation since the
> >> in-kernel programming API will not be affected by the change.
> >>
> [...]
>