[PATCH 3/3] leds: trigger: netdev: serialize mode/interval stores with trigger lock

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>

netdev_led_attr_store() and interval_store() update the shared
trigger_data state and call set_baseline_state() without holding
trigger_data->lock, while netdev_trig_notify() and device_name_store()
do the same work under that lock. Because the store paths do not take
the lock, it provides no mutual exclusion against them.

kernfs only serializes writes to the same attribute file, so writes to
two different files run concurrently. netdev_led_attr_store() does a
non-atomic read-modify-write of ->mode, so one update can be lost:

  CPU0 (echo 1 > link_10)            CPU1 (echo 1 > link_100)
  -------------------------          --------------------------
  mode = trigger_data->mode; // 0
                                     mode = trigger_data->mode; // 0
  set_bit(LINK_10, &mode);
                                     set_bit(LINK_100, &mode);
  trigger_data->mode = mode; // LINK_10
                                     trigger_data->mode = mode; // LINK_100
                                     /* LINK_10 update lost */

The store path also races with the notifier, which updates the link
state under the lock. set_baseline_state() then programs the LED from a
half-updated snapshot, and both CPUs drive the same led_cdev at once:

  CPU0 (echo 1 > link)               CPU1 (NETDEV_CHANGE)
  -------------------------          --------------------------
                                     mutex_lock(&trigger_data->lock);
                                     carrier_link_up = false;
                                     link_speed = SPEED_UNKNOWN;
  trigger_data->mode = mode;
  set_baseline_state();
    /* reads carrier_link_up == false,
       link_speed == SPEED_UNKNOWN,
       torn intermediate state */
                                     get_device_state(); // refill
                                     set_baseline_state();
                                     mutex_unlock(&trigger_data->lock);

->hw_control is likewise written non-atomically by both the store path
and netdev_trig_notify(), and set_baseline_state() branches on it.

Hold trigger_data->lock around the read-modify-write of ->mode, the
->hw_control update and set_baseline_state() in both stores, matching
the locking already used by device_name_store() and
netdev_trig_notify(). In netdev_led_attr_store() use guard(mutex) so
the lock is released on every return path; the invalid-combination check
is performed on the locked snapshot so a rejected write does not disturb
the running configuration. cancel_delayed_work_sync() is called under
the lock; this is safe because netdev_trig_work() never acquires
trigger_data->lock.

Fixes: d5e01266e7f5 ("leds: trigger: netdev: add additional specific link speed mode")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
---
 drivers/leds/trigger/ledtrig-netdev.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 89b48c522c227..6556f6d28547f 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -389,7 +389,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
-	unsigned long state, mode = trigger_data->mode;
+	unsigned long state, mode;
 	int ret;
 	int bit;
 
@@ -421,6 +421,16 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return -EINVAL;
 	}
 
+	/*
+	 * Serialize the read-modify-write of ->mode and the dependent
+	 * ->hw_control update and set_baseline_state() against concurrent
+	 * attribute stores and netdev_trig_notify().  netdev_trig_work() must
+	 * never take this lock, otherwise the cancel_delayed_work_sync() below
+	 * would deadlock.
+	 */
+	guard(mutex)(&trigger_data->lock);
+
+	mode = trigger_data->mode;
 	if (state)
 		set_bit(bit, &mode);
 	else
@@ -510,10 +520,14 @@ static ssize_t interval_store(struct device *dev,
 
 	/* impose some basic bounds on the timer interval */
 	if (value >= 5 && value <= 10000) {
+		mutex_lock(&trigger_data->lock);
+
 		cancel_delayed_work_sync(&trigger_data->work);
 
 		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
 		set_baseline_state(trigger_data);	/* resets timer */
+
+		mutex_unlock(&trigger_data->lock);
 	}
 
 	return size;
-- 
2.55.0