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: [email protected] 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 <[email protected]>
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
>From cc873c2cddd8c7457ad807d71ebada42754638dd Mon Sep 17 00:00:00 2001
From: Andrey Gelman <[email protected]>
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 <[email protected]>
---
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
