Re: A patch to test_power module

Re: A patch to test_power module

[Anton Vorontsov]

On Sun, Jan 27, 2013 at 10:45:43PM +0200, Andrey Gelman wrote:
>     Hello !
>     This patch fixes 2 issues in test_power module:
>     1. the mapping of ac_online parameter to its string value "on / off"
>     was swapped.
>     2. more important ...
>     You could not insmod test_power with module parameters, without
>     causing kernel crash.
>     The reason is that in test_power, setting module parameter value has
>     side effect - power_supply_changed event.
>     As the module parameters are processed prior to calling module_init,
>     power_supply_changed event is generated on behalf module that is not
>     yet initialized, causing some NULL pointer dereference.
>     
>     My solution is to test whether the module has been initialized,
>     prior to calling power_supply_changed.

Hi!

The patch looks good, but it is missing Signed-off-by tag. Plus, you
should also Cc: linux-kernel@vger.kernel.org on submissions (I've added it
now).

Plus, please don't use html in emails. :)

Thanks a lot!

Anton

> From ccd5b3e916477a3c2a51bbf10a3be8e01f3c85e4 Mon Sep 17 00:00:00 2001
> From: Andrey Gelman <andreyg@compulab.co.il>
> Date: Sun, 27 Jan 2013 22:16:33 +0200
> Subject: [PATCH] test_power: fix a bug in setting module parameter values
> 
> When the kernel loads a module, module params are processed
> prior to calling module_init. As a result, setting module param
> value should not have side effects, at least as long as the
> module has not been initialized.
> ---
>  drivers/power/test_power.c |   31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/power/test_power.c b/drivers/power/test_power.c
> index b99a452..753a74f 100644
> --- a/drivers/power/test_power.c
> +++ b/drivers/power/test_power.c
> @@ -30,6 +30,8 @@ static int battery_technology		= POWER_SUPPLY_TECHNOLOGY_LION;
>  static int battery_capacity		= 50;
>  static int battery_voltage		= 3300;
>  
> +static bool module_initialized	= false;
> +
>  static int test_power_get_ac_property(struct power_supply *psy,
>  				      enum power_supply_property psp,
>  				      union power_supply_propval *val)
> @@ -185,6 +187,7 @@ static int __init test_power_init(void)
>  		}
>  	}
>  
> +	module_initialized = true;
>  	return 0;
>  failed:
>  	while (--i >= 0)
> @@ -209,6 +212,8 @@ static void __exit test_power_exit(void)
>  
>  	for (i = 0; i < ARRAY_SIZE(test_power_supplies); i++)
>  		power_supply_unregister(&test_power_supplies[i]);
> +
> +	module_initialized = false;
>  }
>  module_exit(test_power_exit);
>  
> @@ -221,8 +226,8 @@ struct battery_property_map {
>  };
>  
>  static struct battery_property_map map_ac_online[] = {
> -	{ 0,  "on"  },
> -	{ 1,  "off" },
> +	{ 0,  "off"  },
> +	{ 1,  "on" },
>  	{ -1, NULL  },
>  };
>  
> @@ -295,10 +300,16 @@ static const char *map_get_key(struct battery_property_map *map, int value,
>  	return def_key;
>  }
>  
> +static inline void signal_power_supply_changed(struct power_supply *psy)
> +{
> +	if (module_initialized)
> +		power_supply_changed(psy);
> +}
> +
>  static int param_set_ac_online(const char *key, const struct kernel_param *kp)
>  {
>  	ac_online = map_get_value(map_ac_online, key, ac_online);
> -	power_supply_changed(&test_power_supplies[0]);
> +	signal_power_supply_changed(&test_power_supplies[0]);
>  	return 0;
>  }
>  
> @@ -311,7 +322,7 @@ static int param_get_ac_online(char *buffer, const struct kernel_param *kp)
>  static int param_set_usb_online(const char *key, const struct kernel_param *kp)
>  {
>  	usb_online = map_get_value(map_ac_online, key, usb_online);
> -	power_supply_changed(&test_power_supplies[2]);
> +	signal_power_supply_changed(&test_power_supplies[2]);
>  	return 0;
>  }
>  
> @@ -325,7 +336,7 @@ static int param_set_battery_status(const char *key,
>  					const struct kernel_param *kp)
>  {
>  	battery_status = map_get_value(map_status, key, battery_status);
> -	power_supply_changed(&test_power_supplies[1]);
> +	signal_power_supply_changed(&test_power_supplies[1]);
>  	return 0;
>  }
>  
> @@ -339,7 +350,7 @@ static int param_set_battery_health(const char *key,
>  					const struct kernel_param *kp)
>  {
>  	battery_health = map_get_value(map_health, key, battery_health);
> -	power_supply_changed(&test_power_supplies[1]);
> +	signal_power_supply_changed(&test_power_supplies[1]);
>  	return 0;
>  }
>  
> @@ -353,7 +364,7 @@ static int param_set_battery_present(const char *key,
>  					const struct kernel_param *kp)
>  {
>  	battery_present = map_get_value(map_present, key, battery_present);
> -	power_supply_changed(&test_power_supplies[0]);
> +	signal_power_supply_changed(&test_power_supplies[0]);
>  	return 0;
>  }
>  
> @@ -369,7 +380,7 @@ static int param_set_battery_technology(const char *key,
>  {
>  	battery_technology = map_get_value(map_technology, key,
>  						battery_technology);
> -	power_supply_changed(&test_power_supplies[1]);
> +	signal_power_supply_changed(&test_power_supplies[1]);
>  	return 0;
>  }
>  
> @@ -390,7 +401,7 @@ static int param_set_battery_capacity(const char *key,
>  		return -EINVAL;
>  
>  	battery_capacity = tmp;
> -	power_supply_changed(&test_power_supplies[1]);
> +	signal_power_supply_changed(&test_power_supplies[1]);
>  	return 0;
>  }
>  
> @@ -405,7 +416,7 @@ static int param_set_battery_voltage(const char *key,
>  		return -EINVAL;
>  
>  	battery_voltage = tmp;
> -	power_supply_changed(&test_power_supplies[1]);
> +	signal_power_supply_changed(&test_power_supplies[1]);
>  	return 0;
>  }
>  
> -- 
> 1.7.9.5

Re: A patch to test_power module

[Andrey Gelman]

[-- Attachment #1: Type: text/plain, Size: 5849 bytes --]

On 02/03/2013 05:39 AM, Anton Vorontsov wrote:
> On Sun, Jan 27, 2013 at 10:45:43PM +0200, Andrey Gelman wrote:
>>      Hello !
>>      This patch fixes 2 issues in test_power module:
>>      1. the mapping of ac_online parameter to its string value "on / off"
>>      was swapped.
>>      2. more important ...
>>      You could not insmod test_power with module parameters, without
>>      causing kernel crash.
>>      The reason is that in test_power, setting module parameter value has
>>      side effect - power_supply_changed event.
>>      As the module parameters are processed prior to calling module_init,
>>      power_supply_changed event is generated on behalf module that is not
>>      yet initialized, causing some NULL pointer dereference.
>>
>>      My solution is to test whether the module has been initialized,
>>      prior to calling power_supply_changed.
>
> Hi!
>
> The patch looks good, but it is missing Signed-off-by tag. Plus, you
> should also Cc: linux-kernel@vger.kernel.org on submissions (I've added it
> now).
>
> Plus, please don't use html in emails. :)
>
> Thanks a lot!
>
> Anton
>
>>  From ccd5b3e916477a3c2a51bbf10a3be8e01f3c85e4 Mon Sep 17 00:00:00 2001
>> From: Andrey Gelman <andreyg@compulab.co.il>
>> Date: Sun, 27 Jan 2013 22:16:33 +0200
>> Subject: [PATCH] test_power: fix a bug in setting module parameter values
>>
>> When the kernel loads a module, module params are processed
>> prior to calling module_init. As a result, setting module param
>> value should not have side effects, at least as long as the
>> module has not been initialized.
>> ---
>>   drivers/power/test_power.c |   31 +++++++++++++++++++++----------
>>   1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/power/test_power.c b/drivers/power/test_power.c
>> index b99a452..753a74f 100644
>> --- a/drivers/power/test_power.c
>> +++ b/drivers/power/test_power.c
>> @@ -30,6 +30,8 @@ static int battery_technology		= POWER_SUPPLY_TECHNOLOGY_LION;
>>   static int battery_capacity		= 50;
>>   static int battery_voltage		= 3300;
>>
>> +static bool module_initialized	= false;
>> +
>>   static int test_power_get_ac_property(struct power_supply *psy,
>>   				      enum power_supply_property psp,
>>   				      union power_supply_propval *val)
>> @@ -185,6 +187,7 @@ static int __init test_power_init(void)
>>   		}
>>   	}
>>
>> +	module_initialized = true;
>>   	return 0;
>>   failed:
>>   	while (--i >= 0)
>> @@ -209,6 +212,8 @@ static void __exit test_power_exit(void)
>>
>>   	for (i = 0; i < ARRAY_SIZE(test_power_supplies); i++)
>>   		power_supply_unregister(&test_power_supplies[i]);
>> +
>> +	module_initialized = false;
>>   }
>>   module_exit(test_power_exit);
>>
>> @@ -221,8 +226,8 @@ struct battery_property_map {
>>   };
>>
>>   static struct battery_property_map map_ac_online[] = {
>> -	{ 0,  "on"  },
>> -	{ 1,  "off" },
>> +	{ 0,  "off"  },
>> +	{ 1,  "on" },
>>   	{ -1, NULL  },
>>   };
>>
>> @@ -295,10 +300,16 @@ static const char *map_get_key(struct battery_property_map *map, int value,
>>   	return def_key;
>>   }
>>
>> +static inline void signal_power_supply_changed(struct power_supply *psy)
>> +{
>> +	if (module_initialized)
>> +		power_supply_changed(psy);
>> +}
>> +
>>   static int param_set_ac_online(const char *key, const struct kernel_param *kp)
>>   {
>>   	ac_online = map_get_value(map_ac_online, key, ac_online);
>> -	power_supply_changed(&test_power_supplies[0]);
>> +	signal_power_supply_changed(&test_power_supplies[0]);
>>   	return 0;
>>   }
>>
>> @@ -311,7 +322,7 @@ static int param_get_ac_online(char *buffer, const struct kernel_param *kp)
>>   static int param_set_usb_online(const char *key, const struct kernel_param *kp)
>>   {
>>   	usb_online = map_get_value(map_ac_online, key, usb_online);
>> -	power_supply_changed(&test_power_supplies[2]);
>> +	signal_power_supply_changed(&test_power_supplies[2]);
>>   	return 0;
>>   }
>>
>> @@ -325,7 +336,7 @@ static int param_set_battery_status(const char *key,
>>   					const struct kernel_param *kp)
>>   {
>>   	battery_status = map_get_value(map_status, key, battery_status);
>> -	power_supply_changed(&test_power_supplies[1]);
>> +	signal_power_supply_changed(&test_power_supplies[1]);
>>   	return 0;
>>   }
>>
>> @@ -339,7 +350,7 @@ static int param_set_battery_health(const char *key,
>>   					const struct kernel_param *kp)
>>   {
>>   	battery_health = map_get_value(map_health, key, battery_health);
>> -	power_supply_changed(&test_power_supplies[1]);
>> +	signal_power_supply_changed(&test_power_supplies[1]);
>>   	return 0;
>>   }
>>
>> @@ -353,7 +364,7 @@ static int param_set_battery_present(const char *key,
>>   					const struct kernel_param *kp)
>>   {
>>   	battery_present = map_get_value(map_present, key, battery_present);
>> -	power_supply_changed(&test_power_supplies[0]);
>> +	signal_power_supply_changed(&test_power_supplies[0]);
>>   	return 0;
>>   }
>>
>> @@ -369,7 +380,7 @@ static int param_set_battery_technology(const char *key,
>>   {
>>   	battery_technology = map_get_value(map_technology, key,
>>   						battery_technology);
>> -	power_supply_changed(&test_power_supplies[1]);
>> +	signal_power_supply_changed(&test_power_supplies[1]);
>>   	return 0;
>>   }
>>
>> @@ -390,7 +401,7 @@ static int param_set_battery_capacity(const char *key,
>>   		return -EINVAL;
>>
>>   	battery_capacity = tmp;
>> -	power_supply_changed(&test_power_supplies[1]);
>> +	signal_power_supply_changed(&test_power_supplies[1]);
>>   	return 0;
>>   }
>>
>> @@ -405,7 +416,7 @@ static int param_set_battery_voltage(const char *key,
>>   		return -EINVAL;
>>
>>   	battery_voltage = tmp;
>> -	power_supply_changed(&test_power_supplies[1]);
>> +	signal_power_supply_changed(&test_power_supplies[1]);
>>   	return 0;
>>   }
>>
>> --
>> 1.7.9.5

Fixed ... I think.
Thank you,
Andrey


[-- Attachment #2: test_power.patch --]
[-- Type: text/x-patch, Size: 4309 bytes --]

>From cc873c2cddd8c7457ad807d71ebada42754638dd Mon Sep 17 00:00:00 2001
From: Andrey Gelman <andreyg@compulab.co.il>
Date: Sun, 27 Jan 2013 22:16:33 +0200
Subject: [PATCH] test_power: fix a bug in setting module parameter values

When the kernel loads a module, module params are processed
prior to calling module_init. As a result, setting module param
value should not have side effects, at least as long as the
module has not been initialized.

Signed-off-by: Andrey Gelman <andreyg@compulab.co.il>
---
 drivers/power/test_power.c |   31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/power/test_power.c b/drivers/power/test_power.c
index b99a452..753a74f 100644
--- a/drivers/power/test_power.c
+++ b/drivers/power/test_power.c
@@ -30,6 +30,8 @@ static int battery_technology		= POWER_SUPPLY_TECHNOLOGY_LION;
 static int battery_capacity		= 50;
 static int battery_voltage		= 3300;
 
+static bool module_initialized	= false;
+
 static int test_power_get_ac_property(struct power_supply *psy,
 				      enum power_supply_property psp,
 				      union power_supply_propval *val)
@@ -185,6 +187,7 @@ static int __init test_power_init(void)
 		}
 	}
 
+	module_initialized = true;
 	return 0;
 failed:
 	while (--i >= 0)
@@ -209,6 +212,8 @@ static void __exit test_power_exit(void)
 
 	for (i = 0; i < ARRAY_SIZE(test_power_supplies); i++)
 		power_supply_unregister(&test_power_supplies[i]);
+
+	module_initialized = false;
 }
 module_exit(test_power_exit);
 
@@ -221,8 +226,8 @@ struct battery_property_map {
 };
 
 static struct battery_property_map map_ac_online[] = {
-	{ 0,  "on"  },
-	{ 1,  "off" },
+	{ 0,  "off"  },
+	{ 1,  "on" },
 	{ -1, NULL  },
 };
 
@@ -295,10 +300,16 @@ static const char *map_get_key(struct battery_property_map *map, int value,
 	return def_key;
 }
 
+static inline void signal_power_supply_changed(struct power_supply *psy)
+{
+	if (module_initialized)
+		power_supply_changed(psy);
+}
+
 static int param_set_ac_online(const char *key, const struct kernel_param *kp)
 {
 	ac_online = map_get_value(map_ac_online, key, ac_online);
-	power_supply_changed(&test_power_supplies[0]);
+	signal_power_supply_changed(&test_power_supplies[0]);
 	return 0;
 }
 
@@ -311,7 +322,7 @@ static int param_get_ac_online(char *buffer, const struct kernel_param *kp)
 static int param_set_usb_online(const char *key, const struct kernel_param *kp)
 {
 	usb_online = map_get_value(map_ac_online, key, usb_online);
-	power_supply_changed(&test_power_supplies[2]);
+	signal_power_supply_changed(&test_power_supplies[2]);
 	return 0;
 }
 
@@ -325,7 +336,7 @@ static int param_set_battery_status(const char *key,
 					const struct kernel_param *kp)
 {
 	battery_status = map_get_value(map_status, key, battery_status);
-	power_supply_changed(&test_power_supplies[1]);
+	signal_power_supply_changed(&test_power_supplies[1]);
 	return 0;
 }
 
@@ -339,7 +350,7 @@ static int param_set_battery_health(const char *key,
 					const struct kernel_param *kp)
 {
 	battery_health = map_get_value(map_health, key, battery_health);
-	power_supply_changed(&test_power_supplies[1]);
+	signal_power_supply_changed(&test_power_supplies[1]);
 	return 0;
 }
 
@@ -353,7 +364,7 @@ static int param_set_battery_present(const char *key,
 					const struct kernel_param *kp)
 {
 	battery_present = map_get_value(map_present, key, battery_present);
-	power_supply_changed(&test_power_supplies[0]);
+	signal_power_supply_changed(&test_power_supplies[0]);
 	return 0;
 }
 
@@ -369,7 +380,7 @@ static int param_set_battery_technology(const char *key,
 {
 	battery_technology = map_get_value(map_technology, key,
 						battery_technology);
-	power_supply_changed(&test_power_supplies[1]);
+	signal_power_supply_changed(&test_power_supplies[1]);
 	return 0;
 }
 
@@ -390,7 +401,7 @@ static int param_set_battery_capacity(const char *key,
 		return -EINVAL;
 
 	battery_capacity = tmp;
-	power_supply_changed(&test_power_supplies[1]);
+	signal_power_supply_changed(&test_power_supplies[1]);
 	return 0;
 }
 
@@ -405,7 +416,7 @@ static int param_set_battery_voltage(const char *key,
 		return -EINVAL;
 
 	battery_voltage = tmp;
-	power_supply_changed(&test_power_supplies[1]);
+	signal_power_supply_changed(&test_power_supplies[1]);
 	return 0;
 }
 
-- 
1.7.9.5

Re: A patch to test_power module

[Anton Vorontsov]

On Sun, Feb 03, 2013 at 01:17:16PM +0200, Andrey Gelman wrote:
[...]
> >The patch looks good, but it is missing Signed-off-by tag. Plus, you
> >should also Cc: linux-kernel@vger.kernel.org on submissions (I've added it
> >now).
> >
> >Plus, please don't use html in emails. :)
[...]
> Fixed ... I think.

Thanks a lot for the patch, Andrey! It is now in battery-2.6.git tree.

Anton