[PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[Jan Beulich]

In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
but the return value from coretemp_init() was left to indicate an
error. This isn't correct, as the negative return value indicates to
the module loader that initialization failed, which isn't intended
here and results in dangling pointers.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Chen Gong <gong.chen@linux.intel.com>

---
 drivers/hwmon/coretemp.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- linux-2.6.36-rc4/drivers/hwmon/coretemp.c	2010-09-13 08:45:02.000000000 +0200
+++ 2.6.36-rc4-x86-coretemp-hotplug-init-return/drivers/hwmon/coretemp.c	2010-09-03 17:54:14.000000000 +0200
@@ -541,16 +541,19 @@ static int __init coretemp_init(void)
 				" has no thermal sensor.\n", c->x86_model);
 		}
 	}
+
+#ifndef CONFIG_HOTPLUG_CPU
 	if (list_empty(&pdev_list)) {
 		err = -ENODEV;
 		goto exit_driver_unreg;
 	}
+#endif
 
 	register_hotcpu_notifier(&coretemp_cpu_notifier);
 	return 0;
 
-exit_driver_unreg:
 #ifndef CONFIG_HOTPLUG_CPU
+exit_driver_unreg:
 	platform_driver_unregister(&coretemp_driver);
 #endif
 exit:

Re: [lm-sensors] [PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[Chen Gong]

于 9/13/2010 6:05 PM, Jan Beulich 写道:
> In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
> platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
> but the return value from coretemp_init() was left to indicate an
> error. This isn't correct, as the negative return value indicates to
> the module loader that initialization failed, which isn't intended

only !HOTPLUG_CPU can remove the driver. That is what we need to do 
because under the hotplug condition, we need to consider those "offline"
cpus. I don't understand what you are afraid. Can you give me a scinario ?

> here and results in dangling pointers.
>
> Signed-off-by: Jan Beulich<jbeulich@novell.com>
> Cc: Chen Gong<gong.chen@linux.intel.com>
>
> ---
>   drivers/hwmon/coretemp.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> --- linux-2.6.36-rc4/drivers/hwmon/coretemp.c	2010-09-13 08:45:02.000000000 +0200
> +++ 2.6.36-rc4-x86-coretemp-hotplug-init-return/drivers/hwmon/coretemp.c	2010-09-03 17:54:14.000000000 +0200
> @@ -541,16 +541,19 @@ static int __init coretemp_init(void)
>   				" has no thermal sensor.\n", c->x86_model);
>   		}
>   	}
> +
> +#ifndef CONFIG_HOTPLUG_CPU
>   	if (list_empty(&pdev_list)) {
>   		err = -ENODEV;
>   		goto exit_driver_unreg;
>   	}
> +#endif
>
>   	register_hotcpu_notifier(&coretemp_cpu_notifier);
>   	return 0;
>
> -exit_driver_unreg:
>   #ifndef CONFIG_HOTPLUG_CPU
> +exit_driver_unreg:
>   	platform_driver_unregister(&coretemp_driver);
>   #endif
>   exit:
>
>
>
>

Re: [lm-sensors] [PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[Jan Beulich]

>>> On 14.09.10 at 08:30, Chen Gong <gong.chen@linux.intel.com> wrote:
> 于 9/13/2010 6:05 PM, Jan Beulich 写道:
>> In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
>> platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
>> but the return value from coretemp_init() was left to indicate an
>> error. This isn't correct, as the negative return value indicates to
>> the module loader that initialization failed, which isn't intended
> 
> only !HOTPLUG_CPU can remove the driver. That is what we need to do 
> because under the hotplug condition, we need to consider those "offline"
> cpus. I don't understand what you are afraid. Can you give me a scinario ?

If the driver's init routine returns -ENODEV (like is the case without this
change when HOTPLUG_CPU is enabled and there is no device found),
the platform driver will remain registered, yet the driver memory will
get freed (leaving dangling pointers into the driver's former image). Or
in other words - when leaving the driver registered, the return value
of the init function must be zero.

Jan

>> here and results in dangling pointers.
>>
>> Signed-off-by: Jan Beulich<jbeulich@novell.com>
>> Cc: Chen Gong<gong.chen@linux.intel.com>
>>
>> ---
>>   drivers/hwmon/coretemp.c |    5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> --- linux-2.6.36-rc4/drivers/hwmon/coretemp.c	2010-09-13 08:45:02.000000000 
> +0200
>> +++ 
> 2.6.36-rc4-x86-coretemp-hotplug-init-return/drivers/hwmon/coretemp.c	2010-
> 09-03 17:54:14.000000000 +0200
>> @@ -541,16 +541,19 @@ static int __init coretemp_init(void)
>>   				" has no thermal sensor.\n", c->x86_model);
>>   		}
>>   	}
>> +
>> +#ifndef CONFIG_HOTPLUG_CPU
>>   	if (list_empty(&pdev_list)) {
>>   		err = -ENODEV;
>>   		goto exit_driver_unreg;
>>   	}
>> +#endif
>>
>>   	register_hotcpu_notifier(&coretemp_cpu_notifier);
>>   	return 0;
>>
>> -exit_driver_unreg:
>>   #ifndef CONFIG_HOTPLUG_CPU
>> +exit_driver_unreg:
>>   	platform_driver_unregister(&coretemp_driver);
>>   #endif
>>   exit:
>>
>>
>>
>>

Re: [lm-sensors] [PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[Chen Gong]

于 9/14/2010 2:47 PM, Jan Beulich 写道:
>>>> On 14.09.10 at 08:30, Chen Gong<gong.chen@linux.intel.com>  wrote:
>> 于 9/13/2010 6:05 PM, Jan Beulich 写道:
>>> In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
>>> platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
>>> but the return value from coretemp_init() was left to indicate an
>>> error. This isn't correct, as the negative return value indicates to
>>> the module loader that initialization failed, which isn't intended
>>
>> only !HOTPLUG_CPU can remove the driver. That is what we need to do
>> because under the hotplug condition, we need to consider those "offline"
>> cpus. I don't understand what you are afraid. Can you give me a scinario ?
>
> If the driver's init routine returns -ENODEV (like is the case without this
> change when HOTPLUG_CPU is enabled and there is no device found),
> the platform driver will remain registered, yet the driver memory will
> get freed (leaving dangling pointers into the driver's former image). Or

Under this condition, I can't fine who frees the memory of the platform
driver

> in other words - when leaving the driver registered, the return value
> of the init function must be zero.

Is it mandotary or just some kind of convention ?

>
> Jan
>
>>> here and results in dangling pointers.
>>>
>>> Signed-off-by: Jan Beulich<jbeulich@novell.com>
>>> Cc: Chen Gong<gong.chen@linux.intel.com>
>>>
>>> ---
>>>    drivers/hwmon/coretemp.c |    5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> --- linux-2.6.36-rc4/drivers/hwmon/coretemp.c	2010-09-13 08:45:02.000000000
>> +0200
>>> +++
>> 2.6.36-rc4-x86-coretemp-hotplug-init-return/drivers/hwmon/coretemp.c	2010-
>> 09-03 17:54:14.000000000 +0200
>>> @@ -541,16 +541,19 @@ static int __init coretemp_init(void)
>>>    				" has no thermal sensor.\n", c->x86_model);
>>>    		}
>>>    	}
>>> +
>>> +#ifndef CONFIG_HOTPLUG_CPU
>>>    	if (list_empty(&pdev_list)) {
>>>    		err = -ENODEV;
>>>    		goto exit_driver_unreg;
>>>    	}
>>> +#endif
>>>
>>>    	register_hotcpu_notifier(&coretemp_cpu_notifier);
>>>    	return 0;
>>>
>>> -exit_driver_unreg:
>>>    #ifndef CONFIG_HOTPLUG_CPU
>>> +exit_driver_unreg:
>>>    	platform_driver_unregister(&coretemp_driver);
>>>    #endif
>>>    exit:
>>>
>>>
>>>
>>>

Re: [lm-sensors] [PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[Jan Beulich]

>>> On 14.09.10 at 09:46, Chen Gong <gong.chen@linux.intel.com> wrote:
> 于 9/14/2010 2:47 PM, Jan Beulich 写道:
>>>>> On 14.09.10 at 08:30, Chen Gong<gong.chen@linux.intel.com>  wrote:
>>> 于 9/13/2010 6:05 PM, Jan Beulich 写道:
>>>> In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
>>>> platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
>>>> but the return value from coretemp_init() was left to indicate an
>>>> error. This isn't correct, as the negative return value indicates to
>>>> the module loader that initialization failed, which isn't intended
>>>
>>> only !HOTPLUG_CPU can remove the driver. That is what we need to do
>>> because under the hotplug condition, we need to consider those "offline"
>>> cpus. I don't understand what you are afraid. Can you give me a scinario ?
>>
>> If the driver's init routine returns -ENODEV (like is the case without this
>> change when HOTPLUG_CPU is enabled and there is no device found),
>> the platform driver will remain registered, yet the driver memory will
>> get freed (leaving dangling pointers into the driver's former image). Or
> 
> Under this condition, I can't fine who frees the memory of the platform
> driver

I'm not talking about the control structure, but about the driver's
code and data.

>> in other words - when leaving the driver registered, the return value
>> of the init function must be zero.
> 
> Is it mandotary or just some kind of convention ?

Mandatory.

Jan

Re: [PATCH] x86/hwmon: fix module init for hotplug-but-no-device-found case

[Guenter Roeck]

Jan Beulich <JBeulich <at> novell.com> writes:

> 
> In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
> platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
> but the return value from coretemp_init() was left to indicate an
> error. This isn't correct, as the negative return value indicates to
> the module loader that initialization failed, which isn't intended
> here and results in dangling pointers.
> 
> Signed-off-by: Jan Beulich <jbeulich <at> novell.com>
> Cc: Chen Gong <gong.chen <at> linux.intel.com>
> 
I have not seen an Acked-by or Nacked-by for this patch or any other patch from
this series from Jan. 

Discussions indicate that the patches are ok. Unless I hear an explicit Nack,
I'll review the patches and apply them if I think they make sense.

Guenter

Re: x86/hwmon: fix module init for hotplug-but-no-device-found case

[Guenter Roeck]

On Mon, Sep 13, 2010 at 10:05:51AM -0000, Jan Beulich wrote:
> In commit 0dca94baeab4a1a514841b0a4c8e3a51dfb4d5ae the call to
> platform_driver_unregister() was made conditional upon !HOTPLUG_CPU,
> but the return value from coretemp_init() was left to indicate an
> error. This isn't correct, as the negative return value indicates to
> the module loader that initialization failed, which isn't intended
> here and results in dangling pointers.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Chen Gong <gong.chen@linux.intel.com>
> 
Looks like my previous mail got lost or stuck in our e-mail system.
Trying again. Sorry if this message is received twice.

I have not seen an Acked-by for this set of patches, nor a Nack. Since 
(most of) the patches fix real bugs, I applied the ones which do into
my staging tree, and the others into my -next tree.

Guenter