All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: "Darren Hart" <dvhart@infradead.org>,
	"Andy Shevchenko" <andy@infradead.org>,
	"Pali Rohár" <pali.rohar@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org
Subject: [PATCH v9 3/4] platform/x86/dell-laptop: Protect kbd_state against races
Date: Thu, 16 Mar 2017 11:55:34 +0100	[thread overview]
Message-ID: <20170316105535.8885-3-hdegoede@redhat.com> (raw)
In-Reply-To: <20170316105535.8885-1-hdegoede@redhat.com>

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
---
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 4de7aa0..da185fb 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -1139,6 +1139,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.
@@ -1568,9 +1569,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;
@@ -1578,9 +1581,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,
@@ -1640,9 +1646,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);
@@ -1656,18 +1664,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] == '+')
@@ -1683,22 +1697,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,
@@ -1755,12 +1776,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;
 
@@ -1780,15 +1805,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,
@@ -1823,18 +1853,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,
@@ -1919,27 +1954,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 = {
-- 
2.9.3

  parent reply	other threads:[~2017-03-16 10:55 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-16 10:55 [PATCH v9 1/4] platform/x86/dell-*: Add a generic dell-laptop notifier chain Hans de Goede
2017-03-16 10:55 ` [PATCH v9 2/4] platform/x86/dell-laptop: Refactor kbd_led_triggers_store() Hans de Goede
2017-03-16 10:55 ` Hans de Goede [this message]
2017-03-16 10:55 ` [PATCH v9 4/4] platform/x86/dell-*: Call led_classdev_notify_brightness_hw_changed on kbd brightness change Hans de Goede
2017-03-17 22:33   ` Darren Hart
2017-03-18 13:42     ` Hans de Goede
2017-03-19 15:10   ` Pali Rohár
2017-03-19 18:06     ` Hans de Goede
2017-03-19 18:11       ` Pali Rohár
2017-03-22 16:59         ` Darren Hart
2017-03-23  8:58           ` Pali Rohár
2017-03-22 16:53       ` Darren Hart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170316105535.8885-3-hdegoede@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=andy@infradead.org \
    --cc=dvhart@infradead.org \
    --cc=pali.rohar@gmail.com \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.