diff mbox series

[v2] drm/i915/hwmon: Get rid of devm

Message ID 20240415223612.738535-1-ashutosh.dixit@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915/hwmon: Get rid of devm | expand

Commit Message

Dixit, Ashutosh April 15, 2024, 10:36 p.m. UTC
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(-)

Comments

Armin Wolf April 15, 2024, 11:35 p.m. UTC | #1
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);
>   }
Dixit, Ashutosh April 16, 2024, 4:05 a.m. UTC | #2
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
Jani Nikula April 16, 2024, 7:44 a.m. UTC | #3
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
Rodrigo Vivi April 16, 2024, 6:55 p.m. UTC | #4
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
>
Dixit, Ashutosh April 16, 2024, 7:02 p.m. UTC | #5
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
> >
Rodrigo Vivi April 16, 2024, 7:15 p.m. UTC | #6
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
> > >
diff mbox series

Patch

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);
 }