* [PATCH v4] drm/i915/hwmon: Get rid of devm
@ 2024-04-17 5:16 Ashutosh Dixit
2024-04-17 8:09 ` ✗ Fi.CI.BAT: failure for drm/i915/hwmon: Get rid of devm (rev5) Patchwork
2024-04-17 8:28 ` [PATCH v4] drm/i915/hwmon: Get rid of devm Andi Shyti
0 siblings, 2 replies; 4+ messages in thread
From: Ashutosh Dixit @ 2024-04-17 5:16 UTC (permalink / raw
To: intel-gfx
Cc: Badal Nilawar, Andi Shyti, Ville Syrjälä, Rodrigo Vivi,
Jani Nikula, 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
v3: Cleanup from i915_hwmon_register on error (Armin Wolf)
v4: Eliminate potential static analyzer warning (Rodrigo)
Eliminate fetch_and_zero (Jani)
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10366
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
drivers/gpu/drm/i915/i915_hwmon.c | 52 +++++++++++++++++++++----------
1 file changed, 36 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index b758fd110c20..1551a40a675e 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -793,7 +793,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;
@@ -819,14 +819,12 @@ 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);
- if (IS_ERR(hwmon_dev)) {
- i915->hwmon = NULL;
- return;
- }
+ hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
+ ddat,
+ &hwm_chip_info,
+ hwm_groups);
+ if (IS_ERR(hwmon_dev))
+ goto err;
ddat->hwmon_dev = hwmon_dev;
@@ -839,16 +837,38 @@ 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);
- if (!IS_ERR(hwmon_dev))
- ddat_gt->hwmon_dev = hwmon_dev;
+ hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
+ ddat_gt,
+ &hwm_gt_chip_info,
+ NULL);
+ if (IS_ERR(hwmon_dev))
+ goto err;
+
+ ddat_gt->hwmon_dev = hwmon_dev;
}
+ return;
+err:
+ i915_hwmon_unregister(i915);
}
void i915_hwmon_unregister(struct drm_i915_private *i915)
{
- fetch_and_zero(&i915->hwmon);
+ struct i915_hwmon *hwmon = i915->hwmon;
+ struct intel_gt *gt;
+ int i;
+
+ if (!hwmon)
+ return;
+
+ for_each_gt(gt, i915, i)
+ if (hwmon->ddat_gt[i].hwmon_dev)
+ hwmon_device_unregister(hwmon->ddat_gt[i].hwmon_dev);
+
+ if (hwmon->ddat.hwmon_dev)
+ hwmon_device_unregister(hwmon->ddat.hwmon_dev);
+
+ mutex_destroy(&hwmon->hwmon_lock);
+
+ kfree(i915->hwmon);
+ i915->hwmon = NULL;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915/hwmon: Get rid of devm (rev5)
2024-04-17 5:16 [PATCH v4] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
@ 2024-04-17 8:09 ` Patchwork
2024-04-17 8:28 ` [PATCH v4] drm/i915/hwmon: Get rid of devm Andi Shyti
1 sibling, 0 replies; 4+ messages in thread
From: Patchwork @ 2024-04-17 8:09 UTC (permalink / raw
To: Ashutosh Dixit; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 6850 bytes --]
== Series Details ==
Series: drm/i915/hwmon: Get rid of devm (rev5)
URL : https://patchwork.freedesktop.org/series/132400/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_14592 -> Patchwork_132400v5
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_132400v5 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_132400v5, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/index.html
Participating hosts (40 -> 36)
------------------------------
Additional (1): bat-atsm-1
Missing (5): fi-bsw-n3050 fi-snb-2520m fi-elk-e7500 bat-jsl-1 bat-arls-3
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_132400v5:
### IGT changes ###
#### Possible regressions ####
* igt@i915_pm_rpm@module-reload:
- bat-arls-2: [PASS][1] -> [ABORT][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14592/bat-arls-2/igt@i915_pm_rpm@module-reload.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-arls-2/igt@i915_pm_rpm@module-reload.html
Known issues
------------
Here are the changes found in Patchwork_132400v5 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_huc_copy@huc-copy:
- bat-atsm-1: NOTRUN -> [FAIL][3] ([i915#10563])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@gem_huc_copy@huc-copy.html
* igt@gem_mmap@basic:
- bat-atsm-1: NOTRUN -> [SKIP][4] ([i915#4083])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@gem_mmap@basic.html
* igt@gem_tiled_pread_basic:
- bat-atsm-1: NOTRUN -> [SKIP][5] ([i915#4079]) +1 other test skip
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@gem_tiled_pread_basic.html
* igt@i915_pm_rps@basic-api:
- bat-atsm-1: NOTRUN -> [SKIP][6] ([i915#6621])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@i915_pm_rps@basic-api.html
* igt@i915_selftest@live@gem:
- bat-atsm-1: NOTRUN -> [ABORT][7] ([i915#10182] / [i915#10564])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@i915_selftest@live@gem.html
* igt@kms_addfb_basic@size-max:
- bat-atsm-1: NOTRUN -> [SKIP][8] ([i915#6077]) +37 other tests skip
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@kms_addfb_basic@size-max.html
* igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
- bat-atsm-1: NOTRUN -> [SKIP][9] ([i915#6078]) +22 other tests skip
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
* igt@kms_force_connector_basic@prune-stale-modes:
- bat-atsm-1: NOTRUN -> [SKIP][10] ([i915#6093]) +4 other tests skip
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@kms_force_connector_basic@prune-stale-modes.html
* igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24:
- bat-atsm-1: NOTRUN -> [SKIP][11] ([i915#1836]) +6 other tests skip
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24.html
* igt@kms_prop_blob@basic:
- bat-atsm-1: NOTRUN -> [SKIP][12] ([i915#7357])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@kms_prop_blob@basic.html
* igt@kms_setmode@basic-clone-single-crtc:
- bat-atsm-1: NOTRUN -> [SKIP][13] ([i915#6094])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-fence-mmap:
- bat-atsm-1: NOTRUN -> [SKIP][14] ([i915#4077]) +4 other tests skip
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@prime_vgem@basic-fence-mmap.html
* igt@prime_vgem@basic-write:
- bat-atsm-1: NOTRUN -> [SKIP][15] +2 other tests skip
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-atsm-1/igt@prime_vgem@basic-write.html
#### Possible fixes ####
* igt@i915_module_load@load:
- bat-dg2-9: [DMESG-WARN][16] ([i915#10014]) -> [PASS][17]
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14592/bat-dg2-9/igt@i915_module_load@load.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/bat-dg2-9/igt@i915_module_load@load.html
* igt@i915_selftest@live@hugepages:
- fi-apl-guc: [ABORT][18] ([i915#10461]) -> [PASS][19]
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14592/fi-apl-guc/igt@i915_selftest@live@hugepages.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/fi-apl-guc/igt@i915_selftest@live@hugepages.html
[i915#10014]: https://gitlab.freedesktop.org/drm/intel/issues/10014
[i915#10182]: https://gitlab.freedesktop.org/drm/intel/issues/10182
[i915#10461]: https://gitlab.freedesktop.org/drm/intel/issues/10461
[i915#10563]: https://gitlab.freedesktop.org/drm/intel/issues/10563
[i915#10564]: https://gitlab.freedesktop.org/drm/intel/issues/10564
[i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
[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#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
[i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
[i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
[i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
Build changes
-------------
* Linux: CI_DRM_14592 -> Patchwork_132400v5
CI-20190529: 20190529
CI_DRM_14592: c577180c5a61220ef8db9a4a3cd4de66bf58eed4 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7809: 3a71f659700859cab49b8e05a198ba18a5cbd24a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_132400v5: c577180c5a61220ef8db9a4a3cd4de66bf58eed4 @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
e506a0d262f5 drm/i915/hwmon: Get rid of devm
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_132400v5/index.html
[-- Attachment #2: Type: text/html, Size: 7875 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] drm/i915/hwmon: Get rid of devm
2024-04-17 5:16 [PATCH v4] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
2024-04-17 8:09 ` ✗ Fi.CI.BAT: failure for drm/i915/hwmon: Get rid of devm (rev5) Patchwork
@ 2024-04-17 8:28 ` Andi Shyti
2024-04-17 15:01 ` Dixit, Ashutosh
1 sibling, 1 reply; 4+ messages in thread
From: Andi Shyti @ 2024-04-17 8:28 UTC (permalink / raw
To: Ashutosh Dixit
Cc: intel-gfx, Badal Nilawar, Ville Syrjälä, Rodrigo Vivi,
Jani Nikula, linux-hwmon, dri-devel
Hi Ashutosh,
> @@ -839,16 +837,38 @@ 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);
> - if (!IS_ERR(hwmon_dev))
> - ddat_gt->hwmon_dev = hwmon_dev;
> + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
> + ddat_gt,
> + &hwm_gt_chip_info,
> + NULL);
> + if (IS_ERR(hwmon_dev))
> + goto err;
here the logic is changing, though. Before we were not leaving if
hwmon_device_register_with_info() was returning error.
Is this wanted? And why isn't it described in the log?
Thanks,
Andi
> +
> + ddat_gt->hwmon_dev = hwmon_dev;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] drm/i915/hwmon: Get rid of devm
2024-04-17 8:28 ` [PATCH v4] drm/i915/hwmon: Get rid of devm Andi Shyti
@ 2024-04-17 15:01 ` Dixit, Ashutosh
0 siblings, 0 replies; 4+ messages in thread
From: Dixit, Ashutosh @ 2024-04-17 15:01 UTC (permalink / raw
To: Andi Shyti
Cc: intel-gfx, Badal Nilawar, Ville Syrjälä, Rodrigo Vivi,
Jani Nikula, linux-hwmon, dri-devel
On Wed, 17 Apr 2024 01:28:48 -0700, Andi Shyti wrote:
>
Hi Andi,
> > @@ -839,16 +837,38 @@ 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);
> > - if (!IS_ERR(hwmon_dev))
> > - ddat_gt->hwmon_dev = hwmon_dev;
> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
> > + ddat_gt,
> > + &hwm_gt_chip_info,
> > + NULL);
> > + if (IS_ERR(hwmon_dev))
> > + goto err;
>
> here the logic is changing, though. Before we were not leaving if
> hwmon_device_register_with_info() was returning error.
>
> Is this wanted? And why isn't it described in the log?
Not sure if the previous logic was intentional or not, anyway I have
restored it in v5 (where I once again forgot to add "PATCH v5" to the
Subject but v5 is there in the version log :/).
Thanks.
--
Ashutosh
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-17 15:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-17 5:16 [PATCH v4] drm/i915/hwmon: Get rid of devm Ashutosh Dixit
2024-04-17 8:09 ` ✗ Fi.CI.BAT: failure for drm/i915/hwmon: Get rid of devm (rev5) Patchwork
2024-04-17 8:28 ` [PATCH v4] drm/i915/hwmon: Get rid of devm Andi Shyti
2024-04-17 15:01 ` Dixit, Ashutosh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.