* [PATCH v2] drm/i915/hwmon: Get rid of devm
@ 2024-04-15 22:36 Ashutosh Dixit
2024-04-15 23:35 ` Armin Wolf
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ashutosh Dixit @ 2024-04-15 22:36 UTC (permalink / raw)
To: intel-gfx
Cc: Badal Nilawar, Andi Shyti, Ville Syrjälä, linux-hwmon,
dri-devel
When both hwmon and hwmon drvdata (on which hwmon depends) are device
managed resources, the expectation, on device unbind, is that hwmon will be
released before drvdata. However, in i915 there are two separate code
paths, which both release either drvdata or hwmon and either can be
released before the other. These code paths (for device unbind) are as
follows (see also the bug referenced below):
Call Trace:
release_nodes+0x11/0x70
devres_release_group+0xb2/0x110
component_unbind_all+0x8d/0xa0
component_del+0xa5/0x140
intel_pxp_tee_component_fini+0x29/0x40 [i915]
intel_pxp_fini+0x33/0x80 [i915]
i915_driver_remove+0x4c/0x120 [i915]
i915_pci_remove+0x19/0x30 [i915]
pci_device_remove+0x32/0xa0
device_release_driver_internal+0x19c/0x200
unbind_store+0x9c/0xb0
and
Call Trace:
release_nodes+0x11/0x70
devres_release_all+0x8a/0xc0
device_unbind_cleanup+0x9/0x70
device_release_driver_internal+0x1c1/0x200
unbind_store+0x9c/0xb0
This means that in i915, if use devm, we cannot gurantee that hwmon will
always be released before drvdata. Which means that we have a uaf if hwmon
sysfs is accessed when drvdata has been released but hwmon hasn't.
The only way out of this seems to be do get rid of devm_ and release/free
everything explicitly during device unbind.
v2: Change commit message and other minor code changes
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++--------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 8c3f443c8347..46c24b1ee6df 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915)
if (!IS_DGFX(i915))
return;
- hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+ hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
if (!hwmon)
return;
@@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
hwm_get_preregistration_info(i915);
/* hwmon_dev points to device hwmon<i> */
- hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
- ddat,
- &hwm_chip_info,
- hwm_groups);
+ hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
+ ddat,
+ &hwm_chip_info,
+ hwm_groups);
if (IS_ERR(hwmon_dev)) {
i915->hwmon = NULL;
return;
@@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
continue;
- hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
- ddat_gt,
- &hwm_gt_chip_info,
- NULL);
+ hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
+ ddat_gt,
+ &hwm_gt_chip_info,
+ NULL);
if (!IS_ERR(hwmon_dev))
ddat_gt->hwmon_dev = hwmon_dev;
}
@@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
void i915_hwmon_unregister(struct drm_i915_private *i915)
{
- fetch_and_zero(&i915->hwmon);
+ struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
+ struct hwm_drvdata *ddat = &hwmon->ddat;
+ struct intel_gt *gt;
+ int i;
+
+ if (!hwmon)
+ return;
+
+ for_each_gt(gt, i915, i) {
+ struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
+
+ if (ddat_gt->hwmon_dev) {
+ hwmon_device_unregister(ddat_gt->hwmon_dev);
+ ddat_gt->hwmon_dev = NULL;
+ }
+ }
+
+ if (ddat->hwmon_dev)
+ hwmon_device_unregister(ddat->hwmon_dev);
+
+ mutex_destroy(&hwmon->hwmon_lock);
+ kfree(hwmon);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915/hwmon: Get rid of devm
2024-04-15 22:36 [PATCH v2] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
@ 2024-04-15 23:35 ` Armin Wolf
2024-04-16 4:05 ` Dixit, Ashutosh
2024-04-16 0:44 ` ✓ Fi.CI.BAT: success for drm/i915/hwmon: Get rid of devm (rev2) Patchwork
2024-04-16 18:55 ` [PATCH v2] drm/i915/hwmon: Get rid of devm Rodrigo Vivi
2 siblings, 1 reply; 8+ messages in thread
From: Armin Wolf @ 2024-04-15 23:35 UTC (permalink / raw)
To: Ashutosh Dixit, intel-gfx
Cc: Badal Nilawar, Andi Shyti, Ville Syrjälä, linux-hwmon,
dri-devel
Am 16.04.24 um 00:36 schrieb Ashutosh Dixit:
> When both hwmon and hwmon drvdata (on which hwmon depends) are device
> managed resources, the expectation, on device unbind, is that hwmon will be
> released before drvdata. However, in i915 there are two separate code
> paths, which both release either drvdata or hwmon and either can be
> released before the other. These code paths (for device unbind) are as
> follows (see also the bug referenced below):
>
> Call Trace:
> release_nodes+0x11/0x70
> devres_release_group+0xb2/0x110
> component_unbind_all+0x8d/0xa0
> component_del+0xa5/0x140
> intel_pxp_tee_component_fini+0x29/0x40 [i915]
> intel_pxp_fini+0x33/0x80 [i915]
> i915_driver_remove+0x4c/0x120 [i915]
> i915_pci_remove+0x19/0x30 [i915]
> pci_device_remove+0x32/0xa0
> device_release_driver_internal+0x19c/0x200
> unbind_store+0x9c/0xb0
>
> and
>
> Call Trace:
> release_nodes+0x11/0x70
> devres_release_all+0x8a/0xc0
> device_unbind_cleanup+0x9/0x70
> device_release_driver_internal+0x1c1/0x200
> unbind_store+0x9c/0xb0
>
> This means that in i915, if use devm, we cannot gurantee that hwmon will
> always be released before drvdata. Which means that we have a uaf if hwmon
> sysfs is accessed when drvdata has been released but hwmon hasn't.
>
> The only way out of this seems to be do get rid of devm_ and release/free
> everything explicitly during device unbind.
>
> v2: Change commit message and other minor code changes
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
> drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 8c3f443c8347..46c24b1ee6df 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> if (!IS_DGFX(i915))
> return;
>
> - hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
> if (!hwmon)
> return;
>
> @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> hwm_get_preregistration_info(i915);
>
> /* hwmon_dev points to device hwmon<i> */
> - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> - ddat,
> - &hwm_chip_info,
> - hwm_groups);
> + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> + ddat,
> + &hwm_chip_info,
> + hwm_groups);
> if (IS_ERR(hwmon_dev)) {
> i915->hwmon = NULL;
Hi,
you need to free hwmon here, since it is not managed by devres anymore.
> return;
> @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
> continue;
>
> - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
> - ddat_gt,
> - &hwm_gt_chip_info,
> - NULL);
> + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
> + ddat_gt,
> + &hwm_gt_chip_info,
> + NULL);
> if (!IS_ERR(hwmon_dev))
> ddat_gt->hwmon_dev = hwmon_dev;
> }
> @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
>
> void i915_hwmon_unregister(struct drm_i915_private *i915)
> {
> - fetch_and_zero(&i915->hwmon);
> + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
Why is fetch_and_zero() necessary here?
Thanks,
Armin Wolf
> + struct hwm_drvdata *ddat = &hwmon->ddat;
> + struct intel_gt *gt;
> + int i;
> +
> + if (!hwmon)
> + return;
> +
> + for_each_gt(gt, i915, i) {
> + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
> +
> + if (ddat_gt->hwmon_dev) {
> + hwmon_device_unregister(ddat_gt->hwmon_dev);
> + ddat_gt->hwmon_dev = NULL;
> + }
> + }
> +
> + if (ddat->hwmon_dev)
> + hwmon_device_unregister(ddat->hwmon_dev);
> +
> + mutex_destroy(&hwmon->hwmon_lock);
> + kfree(hwmon);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/hwmon: Get rid of devm (rev2)
2024-04-15 22:36 [PATCH v2] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
2024-04-15 23:35 ` Armin Wolf
@ 2024-04-16 0:44 ` Patchwork
2024-04-16 18:55 ` [PATCH v2] drm/i915/hwmon: Get rid of devm Rodrigo Vivi
2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2024-04-16 0:44 UTC (permalink / raw)
To: Dixit, Ashutosh; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 8656 bytes --]
== Series Details ==
Series: drm/i915/hwmon: Get rid of devm (rev2)
URL : https://patchwork.freedesktop.org/series/132400/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_14582 -> Patchwork_132400v2
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/index.html
Participating hosts (38 -> 31)
------------------------------
Additional (1): bat-mtlp-8
Missing (8): bat-kbl-2 bat-mtlp-6 fi-apl-guc fi-snb-2520m fi-glk-j4005 fi-cfl-8109u fi-elk-e7500 bat-arls-3
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_132400v2:
### IGT changes ###
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24@pipe-a-dp-1:
- {bat-rpls-4}: [PASS][1] -> [DMESG-WARN][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14582/bat-rpls-4/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24@pipe-a-dp-1.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-rpls-4/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24@pipe-a-dp-1.html
Known issues
------------
Here are the changes found in Patchwork_132400v2 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- bat-mtlp-8: NOTRUN -> [SKIP][3] ([i915#9318])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@debugfs_test@basic-hwmon.html
* igt@gem_lmem_swapping@verify-random:
- bat-mtlp-8: NOTRUN -> [SKIP][4] ([i915#4613]) +3 other tests skip
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@gem_lmem_swapping@verify-random.html
* igt@gem_mmap@basic:
- bat-mtlp-8: NOTRUN -> [SKIP][5] ([i915#4083])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@gem_mmap@basic.html
* igt@gem_mmap_gtt@basic:
- bat-mtlp-8: NOTRUN -> [SKIP][6] ([i915#4077]) +2 other tests skip
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@gem_mmap_gtt@basic.html
* igt@gem_render_tiled_blits@basic:
- bat-mtlp-8: NOTRUN -> [SKIP][7] ([i915#4079]) +1 other test skip
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@gem_render_tiled_blits@basic.html
* igt@i915_pm_rps@basic-api:
- bat-mtlp-8: NOTRUN -> [SKIP][8] ([i915#6621])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@i915_pm_rps@basic-api.html
* igt@i915_selftest@live@gem:
- bat-atsm-1: NOTRUN -> [ABORT][9] ([i915#10182] / [i915#10564])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-atsm-1/igt@i915_selftest@live@gem.html
* igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
- bat-mtlp-8: NOTRUN -> [SKIP][10] ([i915#5190])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
* igt@kms_addfb_basic@basic-y-tiled-legacy:
- bat-mtlp-8: NOTRUN -> [SKIP][11] ([i915#4212]) +8 other tests skip
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- bat-mtlp-8: NOTRUN -> [SKIP][12] ([i915#4213]) +1 other test skip
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
* igt@kms_dsc@dsc-basic:
- bat-mtlp-8: NOTRUN -> [SKIP][13] ([i915#3555] / [i915#3840] / [i915#9159])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_dsc@dsc-basic.html
* igt@kms_force_connector_basic@force-load-detect:
- bat-mtlp-8: NOTRUN -> [SKIP][14]
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_force_connector_basic@prune-stale-modes:
- bat-mtlp-8: NOTRUN -> [SKIP][15] ([i915#5274])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html
* igt@kms_psr@psr-primary-mmap-gtt@edp-1:
- bat-mtlp-8: NOTRUN -> [SKIP][16] ([i915#4077] / [i915#9688])
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_psr@psr-primary-mmap-gtt@edp-1.html
* igt@kms_setmode@basic-clone-single-crtc:
- bat-mtlp-8: NOTRUN -> [SKIP][17] ([i915#3555] / [i915#8809])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-fence-mmap:
- bat-mtlp-8: NOTRUN -> [SKIP][18] ([i915#3708] / [i915#4077]) +1 other test skip
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@prime_vgem@basic-fence-mmap.html
* igt@prime_vgem@basic-fence-read:
- bat-mtlp-8: NOTRUN -> [SKIP][19] ([i915#3708]) +1 other test skip
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html
* igt@prime_vgem@basic-write:
- bat-mtlp-8: NOTRUN -> [SKIP][20] ([i915#10216] / [i915#3708])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-mtlp-8/igt@prime_vgem@basic-write.html
#### Possible fixes ####
* igt@i915_selftest@live@execlists:
- bat-adls-6: [TIMEOUT][21] ([i915#10795]) -> [PASS][22]
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14582/bat-adls-6/igt@i915_selftest@live@execlists.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-adls-6/igt@i915_selftest@live@execlists.html
* igt@i915_selftest@live@gt_timelines:
- bat-atsm-1: [INCOMPLETE][23] ([i915#10461]) -> [PASS][24]
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14582/bat-atsm-1/igt@i915_selftest@live@gt_timelines.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/bat-atsm-1/igt@i915_selftest@live@gt_timelines.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[i915#10182]: https://gitlab.freedesktop.org/drm/intel/issues/10182
[i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
[i915#10435]: https://gitlab.freedesktop.org/drm/intel/issues/10435
[i915#10461]: https://gitlab.freedesktop.org/drm/intel/issues/10461
[i915#10564]: https://gitlab.freedesktop.org/drm/intel/issues/10564
[i915#10795]: https://gitlab.freedesktop.org/drm/intel/issues/10795
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
[i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
[i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
[i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688
Build changes
-------------
* Linux: CI_DRM_14582 -> Patchwork_132400v2
CI-20190529: 20190529
CI_DRM_14582: 7bc330055f5c2924b42e389887691fea3f401a45 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7806: 849cd963ce7e8222dcf17cc872d355181fd2c2a2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_132400v2: 7bc330055f5c2924b42e389887691fea3f401a45 @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
da6f6c509896 drm/i915/hwmon: Get rid of devm
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v2/index.html
[-- Attachment #2: Type: text/html, Size: 10063 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915/hwmon: Get rid of devm
2024-04-15 23:35 ` Armin Wolf
@ 2024-04-16 4:05 ` Dixit, Ashutosh
2024-04-16 7:44 ` Jani Nikula
0 siblings, 1 reply; 8+ messages in thread
From: Dixit, Ashutosh @ 2024-04-16 4:05 UTC (permalink / raw)
To: Armin Wolf
Cc: intel-gfx, Badal Nilawar, Andi Shyti, Ville Syrjälä,
linux-hwmon, dri-devel
On Mon, 15 Apr 2024 16:35:02 -0700, Armin Wolf wrote:
>
Hi Armin,
> Am 16.04.24 um 00:36 schrieb Ashutosh Dixit:
> > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> > hwm_get_preregistration_info(i915);
> >
> > /* hwmon_dev points to device hwmon<i> */
> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> > - ddat,
> > - &hwm_chip_info,
> > - hwm_groups);
> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> > + ddat,
> > + &hwm_chip_info,
> > + hwm_groups);
> > if (IS_ERR(hwmon_dev)) {
> > i915->hwmon = NULL;
>
> you need to free hwmon here, since it is not managed by devres anymore.
Thanks a lot for catching this, I had missed it in v2, it's fixed in v3. I
am actually reusing i915_hwmon_unregister() for error unwinding in v3.
>
> > return;
> > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
> > continue;
> >
> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
> > - ddat_gt,
> > - &hwm_gt_chip_info,
> > - NULL);
> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
> > + ddat_gt,
> > + &hwm_gt_chip_info,
> > + NULL);
> > if (!IS_ERR(hwmon_dev))
> > ddat_gt->hwmon_dev = hwmon_dev;
> > }
> > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> >
> > void i915_hwmon_unregister(struct drm_i915_private *i915)
> > {
> > - fetch_and_zero(&i915->hwmon);
> > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
>
> Why is fetch_and_zero() necessary here?
As mentioned, in v3 i915_hwmon_unregister() itself is used for error
unwinding so we need to prevent multiple device_unregister's etc. That is
the purpose of setting i915->hwmon to NULL. But even earlier, though it is
not obvious, i915_hwmon_unregister() is called multiple times. So e.g. it
will be called at device unbind as well as module unload. So once again we
prevent multiple device_unregister's by setting and checking for NULL
i915->hwmon.
>
> > + struct hwm_drvdata *ddat = &hwmon->ddat;
> > + struct intel_gt *gt;
> > + int i;
> > +
> > + if (!hwmon)
> > + return;
> > +
> > + for_each_gt(gt, i915, i) {
> > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
> > +
> > + if (ddat_gt->hwmon_dev) {
> > + hwmon_device_unregister(ddat_gt->hwmon_dev);
> > + ddat_gt->hwmon_dev = NULL;
> > + }
> > + }
> > +
> > + if (ddat->hwmon_dev)
> > + hwmon_device_unregister(ddat->hwmon_dev);
> > +
> > + mutex_destroy(&hwmon->hwmon_lock);
> > + kfree(hwmon);
> > }
Thanks.
--
Ashutosh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915/hwmon: Get rid of devm
2024-04-16 4:05 ` Dixit, Ashutosh
@ 2024-04-16 7:44 ` Jani Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2024-04-16 7:44 UTC (permalink / raw)
To: Dixit, Ashutosh, Armin Wolf
Cc: intel-gfx, Badal Nilawar, Andi Shyti, Ville Syrjälä,
linux-hwmon, dri-devel
On Mon, 15 Apr 2024, "Dixit, Ashutosh" <ashutosh.dixit@intel.com> wrote:
> On Mon, 15 Apr 2024 16:35:02 -0700, Armin Wolf wrote:
>>
>
> Hi Armin,
>
>> Am 16.04.24 um 00:36 schrieb Ashutosh Dixit:
>> > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
>> > hwm_get_preregistration_info(i915);
>> >
>> > /* hwmon_dev points to device hwmon<i> */
>> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>> > - ddat,
>> > - &hwm_chip_info,
>> > - hwm_groups);
>> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
>> > + ddat,
>> > + &hwm_chip_info,
>> > + hwm_groups);
>> > if (IS_ERR(hwmon_dev)) {
>> > i915->hwmon = NULL;
>>
>> you need to free hwmon here, since it is not managed by devres anymore.
>
> Thanks a lot for catching this, I had missed it in v2, it's fixed in v3. I
> am actually reusing i915_hwmon_unregister() for error unwinding in v3.
>
>>
>> > return;
>> > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
>> > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
>> > continue;
>> >
>> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
>> > - ddat_gt,
>> > - &hwm_gt_chip_info,
>> > - NULL);
>> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
>> > + ddat_gt,
>> > + &hwm_gt_chip_info,
>> > + NULL);
>> > if (!IS_ERR(hwmon_dev))
>> > ddat_gt->hwmon_dev = hwmon_dev;
>> > }
>> > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
>> >
>> > void i915_hwmon_unregister(struct drm_i915_private *i915)
>> > {
>> > - fetch_and_zero(&i915->hwmon);
>> > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
>>
>> Why is fetch_and_zero() necessary here?
>
> As mentioned, in v3 i915_hwmon_unregister() itself is used for error
> unwinding so we need to prevent multiple device_unregister's etc. That is
> the purpose of setting i915->hwmon to NULL. But even earlier, though it is
> not obvious, i915_hwmon_unregister() is called multiple times. So e.g. it
> will be called at device unbind as well as module unload. So once again we
> prevent multiple device_unregister's by setting and checking for NULL
> i915->hwmon.
IMO it's more obvious to set i915->hwmon to NULL separately.
BR,
Jani.
>
>>
>> > + struct hwm_drvdata *ddat = &hwmon->ddat;
>> > + struct intel_gt *gt;
>> > + int i;
>> > +
>> > + if (!hwmon)
>> > + return;
>> > +
>> > + for_each_gt(gt, i915, i) {
>> > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
>> > +
>> > + if (ddat_gt->hwmon_dev) {
>> > + hwmon_device_unregister(ddat_gt->hwmon_dev);
>> > + ddat_gt->hwmon_dev = NULL;
>> > + }
>> > + }
>> > +
>> > + if (ddat->hwmon_dev)
>> > + hwmon_device_unregister(ddat->hwmon_dev);
>> > +
>> > + mutex_destroy(&hwmon->hwmon_lock);
>> > + kfree(hwmon);
>> > }
>
> Thanks.
> --
> Ashutosh
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915/hwmon: Get rid of devm
2024-04-15 22:36 [PATCH v2] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
2024-04-15 23:35 ` Armin Wolf
2024-04-16 0:44 ` ✓ Fi.CI.BAT: success for drm/i915/hwmon: Get rid of devm (rev2) Patchwork
@ 2024-04-16 18:55 ` Rodrigo Vivi
2024-04-16 19:02 ` Dixit, Ashutosh
2 siblings, 1 reply; 8+ messages in thread
From: Rodrigo Vivi @ 2024-04-16 18:55 UTC (permalink / raw)
To: Ashutosh Dixit
Cc: intel-gfx, Badal Nilawar, Andi Shyti, Ville Syrjälä,
linux-hwmon, dri-devel
On Mon, Apr 15, 2024 at 03:36:12PM -0700, Ashutosh Dixit wrote:
> When both hwmon and hwmon drvdata (on which hwmon depends) are device
> managed resources, the expectation, on device unbind, is that hwmon will be
> released before drvdata. However, in i915 there are two separate code
> paths, which both release either drvdata or hwmon and either can be
> released before the other. These code paths (for device unbind) are as
> follows (see also the bug referenced below):
>
> Call Trace:
> release_nodes+0x11/0x70
> devres_release_group+0xb2/0x110
> component_unbind_all+0x8d/0xa0
> component_del+0xa5/0x140
> intel_pxp_tee_component_fini+0x29/0x40 [i915]
> intel_pxp_fini+0x33/0x80 [i915]
> i915_driver_remove+0x4c/0x120 [i915]
> i915_pci_remove+0x19/0x30 [i915]
> pci_device_remove+0x32/0xa0
> device_release_driver_internal+0x19c/0x200
> unbind_store+0x9c/0xb0
>
> and
>
> Call Trace:
> release_nodes+0x11/0x70
> devres_release_all+0x8a/0xc0
> device_unbind_cleanup+0x9/0x70
> device_release_driver_internal+0x1c1/0x200
> unbind_store+0x9c/0xb0
>
> This means that in i915, if use devm, we cannot gurantee that hwmon will
> always be released before drvdata. Which means that we have a uaf if hwmon
> sysfs is accessed when drvdata has been released but hwmon hasn't.
>
> The only way out of this seems to be do get rid of devm_ and release/free
> everything explicitly during device unbind.
>
> v2: Change commit message and other minor code changes
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
> drivers/gpu/drm/i915/i915_hwmon.c | 41 +++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 8c3f443c8347..46c24b1ee6df 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -792,7 +792,7 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> if (!IS_DGFX(i915))
> return;
>
> - hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
> if (!hwmon)
> return;
>
> @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> hwm_get_preregistration_info(i915);
>
> /* hwmon_dev points to device hwmon<i> */
> - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> - ddat,
> - &hwm_chip_info,
> - hwm_groups);
> + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> + ddat,
> + &hwm_chip_info,
> + hwm_groups);
> if (IS_ERR(hwmon_dev)) {
> i915->hwmon = NULL;
> return;
> @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> if (!hwm_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
> continue;
>
> - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
> - ddat_gt,
> - &hwm_gt_chip_info,
> - NULL);
> + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
> + ddat_gt,
> + &hwm_gt_chip_info,
> + NULL);
> if (!IS_ERR(hwmon_dev))
> ddat_gt->hwmon_dev = hwmon_dev;
> }
> @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
>
> void i915_hwmon_unregister(struct drm_i915_private *i915)
> {
> - fetch_and_zero(&i915->hwmon);
> + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
> + struct hwm_drvdata *ddat = &hwmon->ddat;
> + struct intel_gt *gt;
> + int i;
> +
> + if (!hwmon)
> + return;
"that's too late", we are going to hear from static analyzer tools.
beter to move ddat = &hwmon->ddat; after this return.
with that,
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> +
> + for_each_gt(gt, i915, i) {
> + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
> +
> + if (ddat_gt->hwmon_dev) {
> + hwmon_device_unregister(ddat_gt->hwmon_dev);
> + ddat_gt->hwmon_dev = NULL;
> + }
> + }
> +
> + if (ddat->hwmon_dev)
> + hwmon_device_unregister(ddat->hwmon_dev);
> +
> + mutex_destroy(&hwmon->hwmon_lock);
> + kfree(hwmon);
> }
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915/hwmon: Get rid of devm
2024-04-16 18:55 ` [PATCH v2] drm/i915/hwmon: Get rid of devm Rodrigo Vivi
@ 2024-04-16 19:02 ` Dixit, Ashutosh
2024-04-16 19:15 ` Rodrigo Vivi
0 siblings, 1 reply; 8+ messages in thread
From: Dixit, Ashutosh @ 2024-04-16 19:02 UTC (permalink / raw)
To: Rodrigo Vivi
Cc: intel-gfx, Badal Nilawar, Andi Shyti, Ville Syrjälä,
linux-hwmon, dri-devel
On Tue, 16 Apr 2024 11:55:20 -0700, Rodrigo Vivi wrote:
>
Hi Rodrigo,
> > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> >
> > void i915_hwmon_unregister(struct drm_i915_private *i915)
> > {
> > - fetch_and_zero(&i915->hwmon);
> > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
> > + struct hwm_drvdata *ddat = &hwmon->ddat;
> > + struct intel_gt *gt;
> > + int i;
> > +
> > + if (!hwmon)
> > + return;
>
> "that's too late", we are going to hear from static analyzer tools.
>
> beter to move ddat = &hwmon->ddat; after this return.
Yeah, I worried a lot about it :/ But then finally decided (and verified)
that we are never actually dereferencing the (possibly NULL) pointer.
But not sure about static analyzer tools, maybe you are right, I'll move
it.
> with that,
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Thanks a lot :)
Ashutosh
>
> > +
> > + for_each_gt(gt, i915, i) {
> > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
> > +
> > + if (ddat_gt->hwmon_dev) {
> > + hwmon_device_unregister(ddat_gt->hwmon_dev);
> > + ddat_gt->hwmon_dev = NULL;
> > + }
> > + }
> > +
> > + if (ddat->hwmon_dev)
> > + hwmon_device_unregister(ddat->hwmon_dev);
> > +
> > + mutex_destroy(&hwmon->hwmon_lock);
> > + kfree(hwmon);
> > }
> > --
> > 2.41.0
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm/i915/hwmon: Get rid of devm
2024-04-16 19:02 ` Dixit, Ashutosh
@ 2024-04-16 19:15 ` Rodrigo Vivi
0 siblings, 0 replies; 8+ messages in thread
From: Rodrigo Vivi @ 2024-04-16 19:15 UTC (permalink / raw)
To: Dixit, Ashutosh
Cc: intel-gfx, Badal Nilawar, Andi Shyti, Ville Syrjälä,
linux-hwmon, dri-devel
On Tue, Apr 16, 2024 at 12:02:10PM -0700, Dixit, Ashutosh wrote:
> On Tue, 16 Apr 2024 11:55:20 -0700, Rodrigo Vivi wrote:
> >
>
> Hi Rodrigo,
>
> > > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> > >
> > > void i915_hwmon_unregister(struct drm_i915_private *i915)
> > > {
> > > - fetch_and_zero(&i915->hwmon);
> > > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
> > > + struct hwm_drvdata *ddat = &hwmon->ddat;
> > > + struct intel_gt *gt;
> > > + int i;
> > > +
> > > + if (!hwmon)
> > > + return;
> >
> > "that's too late", we are going to hear from static analyzer tools.
> >
> > beter to move ddat = &hwmon->ddat; after this return.
>
> Yeah, I worried a lot about it :/ But then finally decided (and verified)
> that we are never actually dereferencing the (possibly NULL) pointer.
yeap, another acceptable approach is to simply remove this check entirely.
>
> But not sure about static analyzer tools, maybe you are right, I'll move
> it.
>
> > with that,
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Thanks a lot :)
>
> Ashutosh
>
> >
> > > +
> > > + for_each_gt(gt, i915, i) {
> > > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
> > > +
> > > + if (ddat_gt->hwmon_dev) {
> > > + hwmon_device_unregister(ddat_gt->hwmon_dev);
> > > + ddat_gt->hwmon_dev = NULL;
> > > + }
> > > + }
> > > +
> > > + if (ddat->hwmon_dev)
> > > + hwmon_device_unregister(ddat->hwmon_dev);
> > > +
> > > + mutex_destroy(&hwmon->hwmon_lock);
> > > + kfree(hwmon);
> > > }
> > > --
> > > 2.41.0
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-16 19:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-15 22:36 [PATCH v2] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
2024-04-15 23:35 ` Armin Wolf
2024-04-16 4:05 ` Dixit, Ashutosh
2024-04-16 7:44 ` Jani Nikula
2024-04-16 0:44 ` ✓ Fi.CI.BAT: success for drm/i915/hwmon: Get rid of devm (rev2) Patchwork
2024-04-16 18:55 ` [PATCH v2] drm/i915/hwmon: Get rid of devm Rodrigo Vivi
2024-04-16 19:02 ` Dixit, Ashutosh
2024-04-16 19:15 ` Rodrigo Vivi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox