Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 21-02-17 15:25, Pali Rohár wrote:
On Tuesday 21 February 2017 15:18:14 Hans de Goede wrote:
Hi,

On 21-02-17 15:06, Pali Rohár wrote:
On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
The kbd led code has multiple entry points each of which modifies the
kbd_state by reading it, modifying a copy, writing the copy and on
error setting the modified copy writing back the original state.

This is racy, so add a mutex protection the read-modify-write cycle
on each of the entry points.

Is this mutex really needed? kbd_get_state and kbd_set_state are already
locked by mutex. Which situation is trying this patch fix?

Yes this is really necessary, between getting the state
and storing it writing to another sysfs attribute
may cause a change to the state which will then get
overwritten by the write of the earlier gotten state.

This is a classic read-modify-write race and as such
needs protection.

Right, to preserve all changes (by all concurrent modifications) it is
needed to do that kbd_get_state() + modify + kbd_set_state() atomically.

So this patch fix all concurrent modifications by kernel.

But does not fix race condition when both userspace and kernel want to
change keyboard brightness settings.

Yes using libsmbios is ALWAYS racy, as said in my previous mails,
people really should use the proper kernel interfaces rather then
directly poking hw from userspace, but just because one path is
racy is not a good reason to not fix the races in another path,
esp, when that other path is the preferred way to do things
and is actually the path which all modern desktop environments use.

Regards,

Hans




Regards,

Hans



Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
Changes in v8:
-New patch in v8 of this patch-set
---
drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
1 file changed, 76 insertions(+), 36 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index a2913a5..70951f3 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
static u8 kbd_previous_mode_bit;

static bool kbd_led_present;
+static DEFINE_MUTEX(kbd_led_mutex);

/*
 * NOTE: there are three ways to set the keyboard backlight level.
@@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
		}
	}

+	mutex_lock(&kbd_led_mutex);
+
	ret = kbd_get_state(&state);
	if (ret)
-		return ret;
+		goto out;

	new_state = state;
	new_state.timeout_value = value;
@@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,

	ret = kbd_set_state_safe(&new_state, &state);
	if (ret)
-		return ret;
+		goto out;

-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
}

static ssize_t kbd_led_timeout_show(struct device *dev,
@@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
	if (trigger[0] != '+' && trigger[0] != '-')
		return -EINVAL;

+	mutex_lock(&kbd_led_mutex);
+
	ret = kbd_get_state(&state);
	if (ret)
-		return ret;
+		goto out;

	if (kbd_triggers_supported)
		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
@@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
				continue;
			if (trigger[0] == '+' &&
-			    triggers_enabled && (state.triggers & BIT(i)))
-				return count;
+			    triggers_enabled && (state.triggers & BIT(i))) {
+				ret = count;
+				goto out;
+			}
			if (trigger[0] == '-' &&
-			    (!triggers_enabled || !(state.triggers & BIT(i))))
-				return count;
+			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
+				ret = count;
+				goto out;
+			}
			trigger_bit = i;
			break;
		}
	}

-	if (trigger_bit == -1)
-		return -EINVAL;
+	if (trigger_bit == -1) {
+		ret = -EINVAL;
+		goto out;
+	}

	new_state = state;
	if (trigger[0] == '+')
@@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
			new_state.triggers &= ~BIT(2);
	}
	if ((kbd_info.triggers & new_state.triggers) !=
-	    new_state.triggers)
-		return -EINVAL;
+	    new_state.triggers) {
+		ret = -EINVAL;
+		goto out;
+	}
	if (new_state.triggers && !triggers_enabled) {
		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
		kbd_set_level(&new_state, kbd_previous_level);
	} else if (new_state.triggers == 0) {
		kbd_set_level(&new_state, 0);
	}
-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-		return -EINVAL;
+	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
+		ret = -EINVAL;
+		goto out;
+	}
	ret = kbd_set_state_safe(&new_state, &state);
	if (ret)
-		return ret;
+		goto out;
	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
		kbd_previous_mode_bit = new_state.mode_bit;
-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
}

static ssize_t kbd_led_triggers_show(struct device *dev,
@@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
	if (ret)
		return ret;

+	mutex_lock(&kbd_led_mutex);
+
	ret = kbd_get_state(&state);
	if (ret)
-		return ret;
+		goto out;

-	if (enable == kbd_is_als_mode_bit(state.mode_bit))
-		return count;
+	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
+		ret = count;
+		goto out;
+	}

	new_state = state;

@@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
			new_state.mode_bit = KBD_MODE_BIT_ON;
		}
	}
-	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
-		return -EINVAL;
+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
+		ret = -EINVAL;
+		goto out;
+	}

	ret = kbd_set_state_safe(&new_state, &state);
	if (ret)
-		return ret;
+		goto out;
	kbd_previous_mode_bit = new_state.mode_bit;

-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
}

static ssize_t kbd_led_als_enabled_show(struct device *dev,
@@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
	if (ret)
		return ret;

+	mutex_lock(&kbd_led_mutex);
+
	ret = kbd_get_state(&state);
	if (ret)
-		return ret;
+		goto out;

	new_state = state;
	new_state.als_setting = setting;

	ret = kbd_set_state_safe(&new_state, &state);
	if (ret)
-		return ret;
+		goto out;

-	return count;
+	ret = count;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
}

static ssize_t kbd_led_als_setting_show(struct device *dev,
@@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
	u16 num;
	int ret;

+	mutex_lock(&kbd_led_mutex);
+
	if (kbd_get_max_level()) {
		ret = kbd_get_state(&state);
		if (ret)
-			return ret;
+			goto out;
		new_state = state;
		ret = kbd_set_level(&new_state, value);
		if (ret)
-			return ret;
-		return kbd_set_state_safe(&new_state, &state);
-	}
-
-	if (kbd_get_valid_token_counts()) {
+			goto out;
+		ret = kbd_set_state_safe(&new_state, &state);
+	} else if (kbd_get_valid_token_counts()) {
		for (num = kbd_token_bits; num != 0 && value > 0; --value)
			num &= num - 1; /* clear the first bit set */
		if (num == 0)
-			return 0;
-		return kbd_set_token_bit(ffs(num) - 1);
+			ret = 0;
+		else
+			ret = kbd_set_token_bit(ffs(num) - 1);
+	} else {
+		pr_warn("Keyboard brightness level control not supported\n");
+		ret = -ENXIO;
	}

-	pr_warn("Keyboard brightness level control not supported\n");
-	return -ENXIO;
+out:
+	mutex_unlock(&kbd_led_mutex);
+	return ret;
}

static struct led_classdev kbd_led = {





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux