[RFC PATCH 6.6.y 0/4] misc: ti-st: close transport removal races

[RFC PATCH 6.6.y 0/4] misc: ti-st: close transport removal races

[Hongyan Xu]

The TI shared transport is published before probe completes and remains
published while remove tears its core down.  The exported protocol entry
points can retain the raw core pointer across that teardown.  An attached
N_TI_WL line discipline and its write-wakeup work provide another path to
the same freed core.

This series publishes only fully initialized transports, accounts users of
the published pointer, withdraws and drains that pointer on remove, and
synchronously closes an attached TTY before freeing the core.

This is an RFC for the 6.6 stable tree because the driver is still present
there but was removed from mainline by commit 78fe66360ed6 ("misc: ti-st:
st_kim: remove the driver").  There is consequently no current mainline
code location through which these fixes can first be merged.

The patches are based on linux-6.6.y commit da47cbc25466.

Hongyan Xu (4):
  misc: ti-st: publish the transport only after probe succeeds
  misc: ti-st: account published transport users
  misc: ti-st: drain published users before transport removal
  misc: ti-st: synchronize TTY teardown with transport removal

 drivers/misc/ti-st/st_core.c | 59 +++++++++++++++++++++++---------
 drivers/misc/ti-st/st_kim.c  | 65 ++++++++++++++++++++++--------------
 include/linux/ti_wilink_st.h |  8 +++++
 3 files changed, 91 insertions(+), 41 deletions(-)

-- 
2.50.1.windows.1

[RFC PATCH 6.6.y 1/4] misc: ti-st: publish the transport only after probe succeeds

[Hongyan Xu]

kim_probe() stores the platform device in st_kim_devices[] before the
transport core and the remaining resources are initialized.  If a later
probe step fails, st_kim_ref() can follow that published device to freed
driver data.

Publish the device only after probe has completed, and clear drvdata when
rolling probe back.

Fixes: 53618cc1e51e ("Staging: sources for ST core")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 drivers/misc/ti-st/st_kim.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
index fe682e0..ec265a6 100644
--- a/drivers/misc/ti-st/st_kim.c
+++ b/drivers/misc/ti-st/st_kim.c
@@ -699,15 +699,7 @@ static int kim_probe(struct platform_device *pdev)
 {
 	struct kim_data_s	*kim_gdata;
 	struct ti_st_plat_data	*pdata = pdev->dev.platform_data;
-	int err;
-
-	if ((pdev->id != -1) && (pdev->id < MAX_ST_DEVICES)) {
-		/* multiple devices could exist */
-		st_kim_devices[pdev->id] = pdev;
-	} else {
-		/* platform's sure about existence of 1 device */
-		st_kim_devices[0] = pdev;
-	}
+	int err, id = 0;
 
 	kim_gdata = kzalloc(sizeof(struct kim_data_s), GFP_KERNEL);
 	if (!kim_gdata) {
@@ -762,12 +754,16 @@ static int kim_probe(struct platform_device *pdev)
 				kim_gdata, &version_fops);
 	debugfs_create_file("protocols", S_IRUGO, kim_debugfs_dir,
 				kim_gdata, &list_fops);
+	if (pdev->id >= 0 && pdev->id < MAX_ST_DEVICES)
+		id = pdev->id;
+	st_kim_devices[id] = pdev;
 	return 0;
 
 err_sysfs_group:
 	st_core_exit(kim_gdata->core_data);
 
 err_core_init:
+	platform_set_drvdata(pdev, NULL);
 	kfree(kim_gdata);
 
 	return err;
-- 
2.50.1.windows.1

[RFC PATCH 6.6.y 2/4] misc: ti-st: account published transport users

[Hongyan Xu]

The shared transport entry points obtain a raw core pointer through
st_kim_ref().  Removal needs to stop new lookups and wait for existing
users before freeing that core.

Serialize publication and lookup, count successful lookups, and use a
scope cleanup to release the count on every return path.  This prepares
the removal path to drain the users without a large error-path rewrite.

Fixes: 53618cc1e51e ("Staging: sources for ST core")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 drivers/misc/ti-st/st_core.c | 15 +++++++++++----
 drivers/misc/ti-st/st_kim.c  | 29 +++++++++++++++++++++++------
 include/linux/ti_wilink_st.h |  8 ++++++++
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index b878431..43d2587 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -7,6 +7,7 @@
  */
 
 #define pr_fmt(fmt)	"(stc): " fmt
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/tty.h>
@@ -24,6 +25,8 @@
  */
 static void (*st_recv)(void *disc_data, const u8 *ptr, size_t count);
 
+DEFINE_FREE(st_kim, struct st_data_s *, st_kim_put(_T));
+
 /********************************************************************/
 static void add_channel_to_table(struct st_data_s *st_gdata,
 		struct st_proto_s *new_proto)
@@ -522,7 +525,7 @@ void kim_st_list_protocols(struct st_data_s *st_gdata, void *buf)
  */
 long st_register(struct st_proto_s *new_proto)
 {
-	struct st_data_s	*st_gdata;
+	struct st_data_s *st_gdata __free(st_kim) = NULL;
 	long err = 0;
 	unsigned long flags = 0;
 
@@ -640,7 +643,7 @@ long st_unregister(struct st_proto_s *proto)
 {
 	long err = 0;
 	unsigned long flags = 0;
-	struct st_data_s	*st_gdata;
+	struct st_data_s *st_gdata __free(st_kim) = NULL;
 
 	pr_debug("%s: %d ", __func__, proto->chnl_id);
 
@@ -688,7 +691,7 @@ long st_unregister(struct st_proto_s *proto)
  */
 long st_write(struct sk_buff *skb)
 {
-	struct st_data_s *st_gdata;
+	struct st_data_s *st_gdata __free(st_kim) = NULL;
 	long len;
 
 	st_kim_ref(&st_gdata, 0);
@@ -719,10 +722,12 @@ EXPORT_SYMBOL_GPL(st_unregister);
  */
 static int st_tty_open(struct tty_struct *tty)
 {
-	struct st_data_s *st_gdata;
+	struct st_data_s *st_gdata __free(st_kim) = NULL;
 	pr_info("%s ", __func__);
 
 	st_kim_ref(&st_gdata, 0);
+	if (!st_gdata)
+		return -ENODEV;
 	st_gdata->tty = tty;
 	tty->disc_data = st_gdata;
 
@@ -878,6 +883,8 @@ int st_core_init(struct st_data_s **core_data)
 
 	/* Locking used in st_int_enqueue() to avoid multiple execution */
 	spin_lock_init(&st_gdata->lock);
+	atomic_set(&st_gdata->active_users, 0);
+	init_waitqueue_head(&st_gdata->users_wait);
 
 	err = st_ll_init(st_gdata);
 	if (err) {
diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
index ec265a6..11eadc1 100644
--- a/drivers/misc/ti-st/st_kim.c
+++ b/drivers/misc/ti-st/st_kim.c
@@ -23,9 +23,11 @@
 #include <linux/skbuff.h>
 #include <linux/ti_wilink_st.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 
 #define MAX_ST_DEVICES	3	/* Imagine 1 on each UART for now */
 static struct platform_device *st_kim_devices[MAX_ST_DEVICES];
+static DEFINE_MUTEX(st_kim_mutex);
 
 /**********************************************************************/
 /* internal functions */
@@ -671,18 +673,31 @@ void st_kim_ref(struct st_data_s **core_data, int id)
 {
 	struct platform_device	*pdev;
 	struct kim_data_s	*kim_gdata;
+
+	*core_data = NULL;
+	mutex_lock(&st_kim_mutex);
 	/* get kim_gdata reference from platform device */
 	pdev = st_get_plat_device(id);
 	if (!pdev)
-		goto err;
+		goto out;
 	kim_gdata = platform_get_drvdata(pdev);
-	if (!kim_gdata)
-		goto err;
+	if (!kim_gdata || !kim_gdata->core_data ||
+	    kim_gdata->core_data->removing)
+		goto out;
 
+	atomic_inc(&kim_gdata->core_data->active_users);
 	*core_data = kim_gdata->core_data;
-	return;
-err:
-	*core_data = NULL;
+out:
+	mutex_unlock(&st_kim_mutex);
+}
+
+void st_kim_put(struct st_data_s *st_gdata)
+{
+	if (!st_gdata)
+		return;
+
+	if (atomic_dec_and_test(&st_gdata->active_users))
+		wake_up(&st_gdata->users_wait);
 }
 
 DEFINE_SHOW_ATTRIBUTE(version);
@@ -756,7 +771,9 @@ static int kim_probe(struct platform_device *pdev)
 				kim_gdata, &list_fops);
 	if (pdev->id >= 0 && pdev->id < MAX_ST_DEVICES)
 		id = pdev->id;
+	mutex_lock(&st_kim_mutex);
 	st_kim_devices[id] = pdev;
+	mutex_unlock(&st_kim_mutex);
 	return 0;
 
 err_sysfs_group:
diff --git a/include/linux/ti_wilink_st.h b/include/linux/ti_wilink_st.h
index 10642d4..f91300d 100644
--- a/include/linux/ti_wilink_st.h
+++ b/include/linux/ti_wilink_st.h
@@ -13,6 +13,7 @@
 #define TI_WILINK_ST_H
 
 #include <linux/skbuff.h>
+#include <linux/wait.h>
 
 /**
  * enum proto-type - The protocol on WiLink chips which share a
@@ -125,6 +126,9 @@ extern long st_unregister(struct st_proto_s *);
  * @ll_state: the various PM states the chip can be, the states are notified
  *	to us, when the chip sends relevant PM packets(SLEEP_IND, WAKE_IND).
  * @kim_data: reference to the parent encapsulating structure.
+ * @active_users: number of published core users being drained by removal.
+ * @users_wait: wait queue used by removal to drain published core users.
+ * @removing: set once the core is no longer available to new users.
  *
  */
 struct st_data_s {
@@ -144,6 +148,9 @@ struct st_data_s {
 	unsigned char	protos_registered;
 	unsigned long ll_state;
 	void *kim_data;
+	atomic_t active_users;
+	wait_queue_head_t users_wait;
+	bool removing;
 	struct tty_struct *tty;
 	struct work_struct work_write_wakeup;
 };
@@ -179,6 +186,7 @@ void st_core_exit(struct st_data_s *);
 
 /* ask for reference from KIM */
 void st_kim_ref(struct st_data_s **, int);
+void st_kim_put(struct st_data_s *st_gdata);
 
 #define GPS_STUB_TEST
 #ifdef GPS_STUB_TEST
-- 
2.50.1.windows.1

[RFC PATCH 6.6.y 3/4] misc: ti-st: drain published users before transport removal

[Hongyan Xu]

kim_remove() leaves the transport published while it tears down GPIO,
sysfs state and the core.  A protocol registration or write can therefore
retain the core pointer across kfree().

Under the publication mutex, mark the core as removing and clear its
global slot.  Unregister the line discipline and wait for all previously
accounted users before releasing the core and its parent data.  Keep the
GPIO and platform data valid until those users have drained.

Fixes: 53618cc1e51e ("Staging: sources for ST core")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 drivers/misc/ti-st/st_core.c | 25 ++++++++++++++-----------
 drivers/misc/ti-st/st_kim.c  | 22 ++++++++++++----------
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index 43d2587..3aed6c3 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -906,20 +906,23 @@ err_unreg_ldisc:
 void st_core_exit(struct st_data_s *st_gdata)
 {
 	long err;
+
+	if (!st_gdata)
+		return;
+
+	tty_unregister_ldisc(&st_ldisc_ops);
+	wait_event(st_gdata->users_wait,
+		   !atomic_read(&st_gdata->active_users));
+
 	/* internal module cleanup */
 	err = st_ll_deinit(st_gdata);
 	if (err)
 		pr_err("error during deinit of ST LL %ld", err);
 
-	if (st_gdata != NULL) {
-		/* Free ST Tx Qs and skbs */
-		skb_queue_purge(&st_gdata->txq);
-		skb_queue_purge(&st_gdata->tx_waitq);
-		kfree_skb(st_gdata->rx_skb);
-		kfree_skb(st_gdata->tx_skb);
-		/* TTY ldisc cleanup */
-		tty_unregister_ldisc(&st_ldisc_ops);
-		/* free the global data pointer */
-		kfree(st_gdata);
-	}
+	/* Free ST Tx Qs and skbs */
+	skb_queue_purge(&st_gdata->txq);
+	skb_queue_purge(&st_gdata->tx_waitq);
+	kfree_skb(st_gdata->rx_skb);
+	kfree_skb(st_gdata->tx_skb);
+	kfree(st_gdata);
 }
diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
index 11eadc1..ab84fe6 100644
--- a/drivers/misc/ti-st/st_kim.c
+++ b/drivers/misc/ti-st/st_kim.c
@@ -788,28 +788,30 @@ err_core_init:
 
 static int kim_remove(struct platform_device *pdev)
 {
-	/* free the GPIOs requested */
 	struct ti_st_plat_data	*pdata = pdev->dev.platform_data;
 	struct kim_data_s	*kim_gdata;
+	int id = 0;
 
 	kim_gdata = platform_get_drvdata(pdev);
-
-	/*
-	 * Free the Bluetooth/FM/GPIO
-	 * nShutdown gpio from the system
-	 */
-	gpio_free(pdata->nshutdown_gpio);
-	pr_info("nshutdown GPIO Freed");
+	if (pdev->id >= 0 && pdev->id < MAX_ST_DEVICES)
+		id = pdev->id;
+	mutex_lock(&st_kim_mutex);
+	kim_gdata->core_data->removing = true;
+	st_kim_devices[id] = NULL;
+	mutex_unlock(&st_kim_mutex);
 
 	debugfs_remove_recursive(kim_debugfs_dir);
 	sysfs_remove_group(&pdev->dev.kobj, &uim_attr_grp);
 	pr_info("sysfs entries removed");
 
-	kim_gdata->kim_pdev = NULL;
 	st_core_exit(kim_gdata->core_data);
+	kim_gdata->core_data = NULL;
+	kim_gdata->kim_pdev = NULL;
+	gpio_free(pdata->nshutdown_gpio);
+	pr_info("nshutdown GPIO Freed");
 
+	platform_set_drvdata(pdev, NULL);
 	kfree(kim_gdata);
-	kim_gdata = NULL;
 	return 0;
 }
 
-- 
2.50.1.windows.1

[RFC PATCH 6.6.y 4/4] misc: ti-st: synchronize TTY teardown with transport removal

[Hongyan Xu]

Unregistering the line discipline does not close an instance which is
already attached to a TTY.  Its callbacks and work_write_wakeup can keep
using the transport after st_core_exit() frees it.

Protect the attached TTY pointer with the core lock.  On removal, take a
TTY reference and synchronously hang it up, then cancel the wakeup work
before releasing the core.  Also cancel the work in normal close and clear
disc_data only after the close cleanup is complete.

Fixes: 53618cc1e51e ("Staging: sources for ST core")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 drivers/misc/ti-st/st_core.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index 3aed6c3..7610656 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -723,13 +723,18 @@ EXPORT_SYMBOL_GPL(st_unregister);
 static int st_tty_open(struct tty_struct *tty)
 {
 	struct st_data_s *st_gdata __free(st_kim) = NULL;
+	unsigned long flags;
+
 	pr_info("%s ", __func__);
 
 	st_kim_ref(&st_gdata, 0);
 	if (!st_gdata)
 		return -ENODEV;
+
+	spin_lock_irqsave(&st_gdata->lock, flags);
 	st_gdata->tty = tty;
 	tty->disc_data = st_gdata;
+	spin_unlock_irqrestore(&st_gdata->lock, flags);
 
 	/* don't do an wakeup for now */
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
@@ -757,6 +762,7 @@ static void st_tty_close(struct tty_struct *tty)
 	struct	st_data_s *st_gdata = tty->disc_data;
 
 	pr_info("%s ", __func__);
+	cancel_work_sync(&st_gdata->work_write_wakeup);
 
 	/*
 	 * TODO:
@@ -777,7 +783,6 @@ static void st_tty_close(struct tty_struct *tty)
 	 * N_TI_WL ldisc is un-installed
 	 */
 	st_kim_complete(st_gdata->kim_data);
-	st_gdata->tty = NULL;
 	/* Flush any pending characters in the driver and discipline. */
 	tty_ldisc_flush(tty);
 	tty_driver_flush_buffer(tty);
@@ -791,6 +796,8 @@ static void st_tty_close(struct tty_struct *tty)
 	st_gdata->rx_state = ST_W4_PACKET_TYPE;
 	kfree_skb(st_gdata->rx_skb);
 	st_gdata->rx_skb = NULL;
+	st_gdata->tty = NULL;
+	tty->disc_data = NULL;
 	spin_unlock_irqrestore(&st_gdata->lock, flags);
 
 	pr_debug("%s: done ", __func__);
@@ -905,6 +912,8 @@ err_unreg_ldisc:
 
 void st_core_exit(struct st_data_s *st_gdata)
 {
+	struct tty_struct *tty;
+	unsigned long flags;
 	long err;
 
 	if (!st_gdata)
@@ -914,6 +923,14 @@ void st_core_exit(struct st_data_s *st_gdata)
 	wait_event(st_gdata->users_wait,
 		   !atomic_read(&st_gdata->active_users));
 
+	spin_lock_irqsave(&st_gdata->lock, flags);
+	tty = tty_kref_get(st_gdata->tty);
+	spin_unlock_irqrestore(&st_gdata->lock, flags);
+	if (tty)
+		tty_vhangup(tty);
+	cancel_work_sync(&st_gdata->work_write_wakeup);
+	tty_kref_put(tty);
+
 	/* internal module cleanup */
 	err = st_ll_deinit(st_gdata);
 	if (err)
-- 
2.50.1.windows.1