[PATCH v2 0/4] platform/x86/amd/hsmp: Fix potential issues during async probing of ACPI driver

[PATCH v2 0/4] platform/x86/amd/hsmp: Fix potential issues during async probing of ACPI driver

[Suma Hegde]

This patch series aims to resolve potential issues that may occur
when asynchronous probing is enabled in HSMP ACPI driver.

* The first patch introduces a mutex to safeguard shared data across
  multiple ACPI probes and removals.

* The second patch implements reference counting to ensure that
  hsmp_pdev->sock allocation is freed, and the miscellaneous device
  is deregistered only after all socket removal calls are completed.

* The third patch relocates the initialization of hsmp_pdev.num_sockets to
  hsmp.c.

* The fourth patch adds a check for sock->dev in hsmp_send_message() to
  prevent access to the hsmp_pdev->sock structure when it is not yet
  initialized or has been removed, which could occur during partial
  probing or removal.

Suma Hegde (4):
  platform/x86/amd/hsmp: Use mutex to synchronize ACPI based probe
  platform/x86/amd/hsmp: Add reference counter to track ACPI probe and
    removal
  platform/x86/amd/hsmp: Move initialization of num_sockets to
    hsmp_common_init()
  platform/x86/amd/hsmp: Ensure initialization of the sock struct in
    hsmp_send_message()

 drivers/platform/x86/amd/hsmp/acpi.c | 34 +++++++++++++++-------------
 drivers/platform/x86/amd/hsmp/hsmp.c | 33 +++++++++++++++++++++++++++
 drivers/platform/x86/amd/hsmp/hsmp.h |  2 +-
 drivers/platform/x86/amd/hsmp/plat.c | 18 ++++-----------
 4 files changed, 56 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] platform/x86/amd/hsmp: Use mutex to synchronize ACPI based probe

[Suma Hegde]

The hsmp_acpi_probe() function is called for each socket. When
asynchronous probing is enabled, it can lead to race conditions
due to multiple probes accessing shared data (hsmp_pdev->sock
and the hsmp_pdev->is_probed variable), which may cause potential
issues.

To resolve these issues, a guard mutex has been introduced to ensure
synchronized execution.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
Changes since v1:
1. Guard the remove() also
2. Update commit description

 drivers/platform/x86/amd/hsmp/acpi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index d974c2289f5a..b981ae3157ea 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -13,6 +13,7 @@
 
 #include <linux/acpi.h>
 #include <linux/array_size.h>
+#include <linux/cleanup.h>
 #include <linux/bits.h>
 #include <linux/bitfield.h>
 #include <linux/device.h>
@@ -20,6 +21,7 @@
 #include <linux/ioport.h>
 #include <linux/kstrtox.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/sysfs.h>
 #include <linux/uuid.h>
@@ -44,6 +46,8 @@ struct hsmp_sys_attr {
 	u32 msg_id;
 };
 
+static DEFINE_MUTEX(hsmp_lock);
+
 static int amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
 			      u32 *value, bool write)
 {
@@ -584,6 +588,7 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 	hsmp_pdev = get_hsmp_pdev();
 	if (!hsmp_pdev)
 		return -ENOMEM;
+	guard(mutex)(&hsmp_lock);
 
 	if (!hsmp_pdev->is_probed) {
 		hsmp_pdev->num_sockets = amd_num_nodes();
@@ -620,6 +625,7 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 
 static void hsmp_acpi_remove(struct platform_device *pdev)
 {
+	guard(mutex)(&hsmp_lock);
 	/*
 	 * We register only one misc_device even on multi-socket system.
 	 * So, deregister should happen only once.
-- 
2.25.1

Re: [PATCH v2 1/4] platform/x86/amd/hsmp: Use mutex to synchronize ACPI based probe

[Ilpo Järvinen]

On Wed, 23 Jul 2025, Suma Hegde wrote:

> The hsmp_acpi_probe() function is called for each socket. When
> asynchronous probing is enabled, it can lead to race conditions
> due to multiple probes accessing shared data (hsmp_pdev->sock
> and the hsmp_pdev->is_probed variable), which may cause potential
> issues.
> 
> To resolve these issues, a guard mutex has been introduced to ensure
> synchronized execution.
> 
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>

I guess Fixes tag would be in order for this and some other patches in 
the series too.

-- 
 i.

> ---
> Changes since v1:
> 1. Guard the remove() also
> 2. Update commit description
> 
>  drivers/platform/x86/amd/hsmp/acpi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index d974c2289f5a..b981ae3157ea 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -13,6 +13,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/array_size.h>
> +#include <linux/cleanup.h>
>  #include <linux/bits.h>
>  #include <linux/bitfield.h>
>  #include <linux/device.h>
> @@ -20,6 +21,7 @@
>  #include <linux/ioport.h>
>  #include <linux/kstrtox.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/sysfs.h>
>  #include <linux/uuid.h>
> @@ -44,6 +46,8 @@ struct hsmp_sys_attr {
>  	u32 msg_id;
>  };
>  
> +static DEFINE_MUTEX(hsmp_lock);
> +
>  static int amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
>  			      u32 *value, bool write)
>  {
> @@ -584,6 +588,7 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  	hsmp_pdev = get_hsmp_pdev();
>  	if (!hsmp_pdev)
>  		return -ENOMEM;
> +	guard(mutex)(&hsmp_lock);
>  
>  	if (!hsmp_pdev->is_probed) {
>  		hsmp_pdev->num_sockets = amd_num_nodes();
> @@ -620,6 +625,7 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  
>  static void hsmp_acpi_remove(struct platform_device *pdev)
>  {
> +	guard(mutex)(&hsmp_lock);
>  	/*
>  	 * We register only one misc_device even on multi-socket system.
>  	 * So, deregister should happen only once.
> 

[PATCH v2 2/4] platform/x86/amd/hsmp: Add reference counter to track ACPI probe and removal

[Suma Hegde]

Implement reference counting to ensure that the memory allocated for
the hsmp_pdev->sock structure is released, and the miscellaneous device
is deregistered only after all socket's removal operations have been
completed.

Also replace hsmp_pdev->is_probed with hsmp_pdev->ref_cnt.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
Changes since v1:
New patch

 drivers/platform/x86/amd/hsmp/acpi.c | 16 ++++++++--------
 drivers/platform/x86/amd/hsmp/hsmp.c | 17 +++++++++++++++++
 drivers/platform/x86/amd/hsmp/hsmp.h |  2 +-
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index b981ae3157ea..232105226407 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/uuid.h>
 
@@ -590,16 +591,15 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	guard(mutex)(&hsmp_lock);
 
-	if (!hsmp_pdev->is_probed) {
+	if (!hsmp_pdev->ref_cnt) {
 		hsmp_pdev->num_sockets = amd_num_nodes();
 		if (hsmp_pdev->num_sockets == 0 || hsmp_pdev->num_sockets > MAX_AMD_NUM_NODES) {
 			dev_err(&pdev->dev, "Wrong number of sockets\n");
 			return -ENODEV;
 		}
 
-		hsmp_pdev->sock = devm_kcalloc(&pdev->dev, hsmp_pdev->num_sockets,
-					       sizeof(*hsmp_pdev->sock),
-					       GFP_KERNEL);
+		hsmp_pdev->sock = kcalloc(hsmp_pdev->num_sockets, sizeof(*hsmp_pdev->sock),
+					  GFP_KERNEL);
 		if (!hsmp_pdev->sock)
 			return -ENOMEM;
 	}
@@ -610,15 +610,15 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (!hsmp_pdev->is_probed) {
+	if (!hsmp_pdev->ref_cnt) {
 		ret = hsmp_misc_register(&pdev->dev);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to register misc device\n");
 			return ret;
 		}
-		hsmp_pdev->is_probed = true;
 		dev_dbg(&pdev->dev, "AMD HSMP ACPI is probed successfully\n");
 	}
+	hsmp_pdev->ref_cnt++;
 
 	return 0;
 }
@@ -630,9 +630,9 @@ static void hsmp_acpi_remove(struct platform_device *pdev)
 	 * We register only one misc_device even on multi-socket system.
 	 * So, deregister should happen only once.
 	 */
-	if (hsmp_pdev->is_probed) {
+	if (!(--hsmp_pdev->ref_cnt)) {
 		hsmp_misc_deregister();
-		hsmp_pdev->is_probed = false;
+		kfree(hsmp_pdev->sock);
 	}
 }
 
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 885e2f8136fd..39804ee848ba 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -12,6 +12,8 @@
 #include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/init.h>
+#include <linux/module.h>
 #include <linux/semaphore.h>
 #include <linux/sysfs.h>
 
@@ -454,6 +456,21 @@ struct hsmp_plat_device *get_hsmp_pdev(void)
 {
 	return &hsmp_pdev;
 }
+
+static int __init hsmp_common_init(void)
+{
+	hsmp_pdev.ref_cnt = 0;
+
+	return 0;
+}
+
+static void __exit hsmp_common_exit(void)
+{
+}
+
+device_initcall(hsmp_common_init);
+module_exit(hsmp_common_exit);
+
 EXPORT_SYMBOL_NS_GPL(get_hsmp_pdev, "AMD_HSMP");
 
 MODULE_DESCRIPTION("AMD HSMP Common driver");
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index 0509a442eaae..1b16fd6a38e1 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -54,7 +54,7 @@ struct hsmp_plat_device {
 	struct hsmp_socket *sock;
 	u32 proto_ver;
 	u16 num_sockets;
-	bool is_probed;
+	int ref_cnt;
 };
 
 int hsmp_cache_proto_ver(u16 sock_ind);
-- 
2.25.1

Re: [PATCH v2 2/4] platform/x86/amd/hsmp: Add reference counter to track ACPI probe and removal

[Ilpo Järvinen]

On Wed, 23 Jul 2025, Suma Hegde wrote:

> Implement reference counting to ensure that the memory allocated for
> the hsmp_pdev->sock structure is released, and the miscellaneous device
> is deregistered only after all socket's removal operations have been
> completed.
> 
> Also replace hsmp_pdev->is_probed with hsmp_pdev->ref_cnt.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v1:
> New patch
> 
>  drivers/platform/x86/amd/hsmp/acpi.c | 16 ++++++++--------
>  drivers/platform/x86/amd/hsmp/hsmp.c | 17 +++++++++++++++++
>  drivers/platform/x86/amd/hsmp/hsmp.h |  2 +-
>  3 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index b981ae3157ea..232105226407 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/uuid.h>
>  
> @@ -590,16 +591,15 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	guard(mutex)(&hsmp_lock);
>  
> -	if (!hsmp_pdev->is_probed) {
> +	if (!hsmp_pdev->ref_cnt) {
>  		hsmp_pdev->num_sockets = amd_num_nodes();
>  		if (hsmp_pdev->num_sockets == 0 || hsmp_pdev->num_sockets > MAX_AMD_NUM_NODES) {
>  			dev_err(&pdev->dev, "Wrong number of sockets\n");
>  			return -ENODEV;
>  		}
>  
> -		hsmp_pdev->sock = devm_kcalloc(&pdev->dev, hsmp_pdev->num_sockets,
> -					       sizeof(*hsmp_pdev->sock),
> -					       GFP_KERNEL);
> +		hsmp_pdev->sock = kcalloc(hsmp_pdev->num_sockets, sizeof(*hsmp_pdev->sock),
> +					  GFP_KERNEL);
>  		if (!hsmp_pdev->sock)
>  			return -ENOMEM;
>  	}
> @@ -610,15 +610,15 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	if (!hsmp_pdev->is_probed) {
> +	if (!hsmp_pdev->ref_cnt) {
>  		ret = hsmp_misc_register(&pdev->dev);
>  		if (ret) {
>  			dev_err(&pdev->dev, "Failed to register misc device\n");
>  			return ret;
>  		}
> -		hsmp_pdev->is_probed = true;
>  		dev_dbg(&pdev->dev, "AMD HSMP ACPI is probed successfully\n");
>  	}
> +	hsmp_pdev->ref_cnt++;

Unfortunately, also this is buggy.

hsmp_pdev->sock is assigned before we're committed to increase ref_cnt 
which causes a memleak as the next one probing after the mutex is unlocked 
on return will overwrite hsmp_pdev->sock.

There are also linux/kref.h linux/refcount.h which should be considered 
instead of creating own ref count code.

>  	return 0;
>  }
> @@ -630,9 +630,9 @@ static void hsmp_acpi_remove(struct platform_device *pdev)
>  	 * We register only one misc_device even on multi-socket system.
>  	 * So, deregister should happen only once.
>  	 */
> -	if (hsmp_pdev->is_probed) {
> +	if (!(--hsmp_pdev->ref_cnt)) {
>  		hsmp_misc_deregister();
> -		hsmp_pdev->is_probed = false;
> +		kfree(hsmp_pdev->sock);
>  	}
>  }
>  
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 885e2f8136fd..39804ee848ba 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -12,6 +12,8 @@
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/semaphore.h>
>  #include <linux/sysfs.h>
>  
> @@ -454,6 +456,21 @@ struct hsmp_plat_device *get_hsmp_pdev(void)
>  {
>  	return &hsmp_pdev;
>  }
> +
> +static int __init hsmp_common_init(void)
> +{
> +	hsmp_pdev.ref_cnt = 0;
> +
> +	return 0;
> +}
> +
> +static void __exit hsmp_common_exit(void)
> +{
> +}
> +
> +device_initcall(hsmp_common_init);
> +module_exit(hsmp_common_exit);
> +
>  EXPORT_SYMBOL_NS_GPL(get_hsmp_pdev, "AMD_HSMP");
>  
>  MODULE_DESCRIPTION("AMD HSMP Common driver");
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 0509a442eaae..1b16fd6a38e1 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -54,7 +54,7 @@ struct hsmp_plat_device {
>  	struct hsmp_socket *sock;
>  	u32 proto_ver;
>  	u16 num_sockets;
> -	bool is_probed;
> +	int ref_cnt;
>  };
>  
>  int hsmp_cache_proto_ver(u16 sock_ind);
> 

-- 
 i.

[PATCH v2 3/4] platform/x86/amd/hsmp: Move initialization of num_sockets to hsmp_common_init()

[Suma Hegde]

With the addition of hsmp_common_init() in hsmp.c, initialize
hsmp_pdev.num_sockets within this function and remove its
initialization from both plat.c and acpi.c.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
Changes since v1:
New patch

 drivers/platform/x86/amd/hsmp/acpi.c |  8 --------
 drivers/platform/x86/amd/hsmp/hsmp.c | 13 +++++++++++++
 drivers/platform/x86/amd/hsmp/plat.c | 18 ++++--------------
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index 232105226407..15c4cedc2759 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -29,8 +29,6 @@
 
 #include <uapi/asm-generic/errno-base.h>
 
-#include <asm/amd/node.h>
-
 #include "hsmp.h"
 
 #define DRIVER_NAME		"hsmp_acpi"
@@ -592,12 +590,6 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 	guard(mutex)(&hsmp_lock);
 
 	if (!hsmp_pdev->ref_cnt) {
-		hsmp_pdev->num_sockets = amd_num_nodes();
-		if (hsmp_pdev->num_sockets == 0 || hsmp_pdev->num_sockets > MAX_AMD_NUM_NODES) {
-			dev_err(&pdev->dev, "Wrong number of sockets\n");
-			return -ENODEV;
-		}
-
 		hsmp_pdev->sock = kcalloc(hsmp_pdev->num_sockets, sizeof(*hsmp_pdev->sock),
 					  GFP_KERNEL);
 		if (!hsmp_pdev->sock)
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 39804ee848ba..e05d824045d6 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -7,7 +7,10 @@
  * This file provides a device implementation for HSMP interface
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <asm/amd/hsmp.h>
+#include <asm/amd/nb.h>
 
 #include <linux/acpi.h>
 #include <linux/delay.h>
@@ -459,6 +462,16 @@ struct hsmp_plat_device *get_hsmp_pdev(void)
 
 static int __init hsmp_common_init(void)
 {
+	/*
+	 * amd_num_nodes() returns number of SMN/DF interfaces present in the system
+	 * if we have N SMN/DF interfaces that ideally means N sockets
+	 */
+	hsmp_pdev.num_sockets = amd_num_nodes();
+	if (hsmp_pdev.num_sockets == 0 || hsmp_pdev.num_sockets > MAX_AMD_NUM_NODES) {
+		pr_err("Wrong number of sockets\n");
+		return -ENODEV;
+	}
+
 	hsmp_pdev.ref_cnt = 0;
 
 	return 0;
diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
index f8aa844d33e4..0c9f8b868bcd 100644
--- a/drivers/platform/x86/amd/hsmp/plat.c
+++ b/drivers/platform/x86/amd/hsmp/plat.c
@@ -205,6 +205,10 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 {
 	int ret;
 
+	hsmp_pdev = get_hsmp_pdev();
+	if (!hsmp_pdev)
+		return -ENOMEM;
+
 	hsmp_pdev->sock = devm_kcalloc(&pdev->dev, hsmp_pdev->num_sockets,
 				       sizeof(*hsmp_pdev->sock),
 				       GFP_KERNEL);
@@ -310,20 +314,6 @@ static int __init hsmp_plt_init(void)
 		return ret;
 	}
 
-	hsmp_pdev = get_hsmp_pdev();
-	if (!hsmp_pdev)
-		return -ENOMEM;
-
-	/*
-	 * amd_num_nodes() returns number of SMN/DF interfaces present in the system
-	 * if we have N SMN/DF interfaces that ideally means N sockets
-	 */
-	hsmp_pdev->num_sockets = amd_num_nodes();
-	if (hsmp_pdev->num_sockets == 0 || hsmp_pdev->num_sockets > MAX_AMD_NUM_NODES) {
-		pr_err("Wrong number of sockets\n");
-		return ret;
-	}
-
 	ret = platform_driver_register(&amd_hsmp_driver);
 	if (ret)
 		return ret;
-- 
2.25.1

[PATCH v2 4/4] platform/x86/amd/hsmp: Ensure initialization of the sock struct in hsmp_send_message()

[Suma Hegde]

If all sockets are not probed, invoking hsmp_send_message() might result in
unexpected behavior due to accessing an uninitialized socket structure.

The initialization of the sock structure can be confirmed if sock->dev
is initialized.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
Changes since v1:
New patch to address the partial probe/removal issue.

 drivers/platform/x86/amd/hsmp/acpi.c | 4 ++++
 drivers/platform/x86/amd/hsmp/hsmp.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index 15c4cedc2759..a2d91d4a3e02 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -617,6 +617,10 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 
 static void hsmp_acpi_remove(struct platform_device *pdev)
 {
+	struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
+
+	sock->dev = NULL;
+
 	guard(mutex)(&hsmp_lock);
 	/*
 	 * We register only one misc_device even on multi-socket system.
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index e05d824045d6..a4420db42781 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -219,6 +219,9 @@ int hsmp_send_message(struct hsmp_message *msg)
 		return -ENODEV;
 	sock = &hsmp_pdev.sock[msg->sock_ind];
 
+	if (!sock->dev)
+		return -ENODEV;
+
 	ret = down_interruptible(&sock->hsmp_sem);
 	if (ret < 0)
 		return ret;
-- 
2.25.1

Re: [PATCH v2 4/4] platform/x86/amd/hsmp: Ensure initialization of the sock struct in hsmp_send_message()

[Ilpo Järvinen]

On Wed, 23 Jul 2025, Suma Hegde wrote:

> If all sockets are not probed, invoking hsmp_send_message() might result in
> unexpected behavior due to accessing an uninitialized socket structure.
> 
> The initialization of the sock structure can be confirmed if sock->dev
> is initialized.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v1:
> New patch to address the partial probe/removal issue.
> 
>  drivers/platform/x86/amd/hsmp/acpi.c | 4 ++++
>  drivers/platform/x86/amd/hsmp/hsmp.c | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 15c4cedc2759..a2d91d4a3e02 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -617,6 +617,10 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  
>  static void hsmp_acpi_remove(struct platform_device *pdev)
>  {
> +	struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
> +
> +	sock->dev = NULL;
> +
>  	guard(mutex)(&hsmp_lock);
>  	/*
>  	 * We register only one misc_device even on multi-socket system.
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index e05d824045d6..a4420db42781 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -219,6 +219,9 @@ int hsmp_send_message(struct hsmp_message *msg)
>  		return -ENODEV;
>  	sock = &hsmp_pdev.sock[msg->sock_ind];
>  
> +	if (!sock->dev)
> +		return -ENODEV;
> +
>  	ret = down_interruptible(&sock->hsmp_sem);
>  	if (ret < 0)
>  		return ret;

This is still racy. AFAICT, nothing prevents assigning NULL into sock->dev 
because of remove while hsmp_send_message() is running and that can result 
in dereferencing NULL.

-- 
 i.

Re: [PATCH v2 4/4] platform/x86/amd/hsmp: Ensure initialization of the sock struct in hsmp_send_message()

[Suma Hegde]

Hi Ilpo,


On 7/23/2025 12:40 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, 23 Jul 2025, Suma Hegde wrote:
>
>> If all sockets are not probed, invoking hsmp_send_message() might result in
>> unexpected behavior due to accessing an uninitialized socket structure.
>>
>> The initialization of the sock structure can be confirmed if sock->dev
>> is initialized.
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>> Changes since v1:
>> New patch to address the partial probe/removal issue.
>>
>>   drivers/platform/x86/amd/hsmp/acpi.c | 4 ++++
>>   drivers/platform/x86/amd/hsmp/hsmp.c | 3 +++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
>> index 15c4cedc2759..a2d91d4a3e02 100644
>> --- a/drivers/platform/x86/amd/hsmp/acpi.c
>> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
>> @@ -617,6 +617,10 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>>
>>   static void hsmp_acpi_remove(struct platform_device *pdev)
>>   {
>> +     struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
>> +
>> +     sock->dev = NULL;
>> +
>>        guard(mutex)(&hsmp_lock);
>>        /*
>>         * We register only one misc_device even on multi-socket system.
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
>> index e05d824045d6..a4420db42781 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>> @@ -219,6 +219,9 @@ int hsmp_send_message(struct hsmp_message *msg)
>>                return -ENODEV;
>>        sock = &hsmp_pdev.sock[msg->sock_ind];
>>
>> +     if (!sock->dev)
>> +             return -ENODEV;
>> +
>>        ret = down_interruptible(&sock->hsmp_sem);
>>        if (ret < 0)
>>                return ret;
> This is still racy. AFAICT, nothing prevents assigning NULL into sock->dev
> because of remove while hsmp_send_message() is running and that can result
> in dereferencing NULL.

Could you please suggest on how to resolve this issue?


> --
>   i.


Thanks and Regards,

Suma

Re: [PATCH v2 4/4] platform/x86/amd/hsmp: Ensure initialization of the sock struct in hsmp_send_message()

[Suma Hegde]

Hi Ilpo,


On 7/24/2025 2:50 PM, Suma Hegde wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
>
>
> Hi Ilpo,
>
>
> On 7/23/2025 12:40 PM, Ilpo Järvinen wrote:
>> Caution: This message originated from an External Source. Use proper 
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> On Wed, 23 Jul 2025, Suma Hegde wrote:
>>
>>> If all sockets are not probed, invoking hsmp_send_message() might 
>>> result in
>>> unexpected behavior due to accessing an uninitialized socket structure.
>>>
>>> The initialization of the sock structure can be confirmed if sock->dev
>>> is initialized.
>>>
>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>>> ---
>>> Changes since v1:
>>> New patch to address the partial probe/removal issue.
>>>
>>>   drivers/platform/x86/amd/hsmp/acpi.c | 4 ++++
>>>   drivers/platform/x86/amd/hsmp/hsmp.c | 3 +++
>>>   2 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c 
>>> b/drivers/platform/x86/amd/hsmp/acpi.c
>>> index 15c4cedc2759..a2d91d4a3e02 100644
>>> --- a/drivers/platform/x86/amd/hsmp/acpi.c
>>> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
>>> @@ -617,6 +617,10 @@ static int hsmp_acpi_probe(struct 
>>> platform_device *pdev)
>>>
>>>   static void hsmp_acpi_remove(struct platform_device *pdev)
>>>   {
>>> +     struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
>>> +
>>> +     sock->dev = NULL;
>>> +
>>>        guard(mutex)(&hsmp_lock);
>>>        /*
>>>         * We register only one misc_device even on multi-socket system.
>>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c 
>>> b/drivers/platform/x86/amd/hsmp/hsmp.c
>>> index e05d824045d6..a4420db42781 100644
>>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>>> @@ -219,6 +219,9 @@ int hsmp_send_message(struct hsmp_message *msg)
>>>                return -ENODEV;
>>>        sock = &hsmp_pdev.sock[msg->sock_ind];
>>>
>>> +     if (!sock->dev)
>>> +             return -ENODEV;
>>> +
>>>        ret = down_interruptible(&sock->hsmp_sem);
>>>        if (ret < 0)
>>>                return ret;
>> This is still racy. AFAICT, nothing prevents assigning NULL into 
>> sock->dev
>> because of remove while hsmp_send_message() is running and that can 
>> result
>> in dereferencing NULL.
>
> Could you please suggest on how to resolve this issue?
>
>

I will relocate hsmp_fops to acpi.c and plat.c from hsmp.c. 
Consequently, the module reference

count for hsmp_acpi should be incremented when the device file is opened.

Does this solution address the issue adequately? Is it acceptable?


>> -- 
>>   i.
>
>
> Thanks and Regards,
>
> Suma
>
>

Re: [PATCH v2 4/4] platform/x86/amd/hsmp: Ensure initialization of the sock struct in hsmp_send_message()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]

On Sat, 2 Aug 2025, Suma Hegde wrote:
> On 7/24/2025 2:50 PM, Suma Hegde wrote:
> > On 7/23/2025 12:40 PM, Ilpo Järvinen wrote:
> > > On Wed, 23 Jul 2025, Suma Hegde wrote:
> > > 
> > > > If all sockets are not probed, invoking hsmp_send_message() might result
> > > > in
> > > > unexpected behavior due to accessing an uninitialized socket structure.
> > > > 
> > > > The initialization of the sock structure can be confirmed if sock->dev
> > > > is initialized.
> > > > 
> > > > Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> > > > Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> > > > ---
> > > > Changes since v1:
> > > > New patch to address the partial probe/removal issue.
> > > > 
> > > >   drivers/platform/x86/amd/hsmp/acpi.c | 4 ++++
> > > >   drivers/platform/x86/amd/hsmp/hsmp.c | 3 +++
> > > >   2 files changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/platform/x86/amd/hsmp/acpi.c
> > > > b/drivers/platform/x86/amd/hsmp/acpi.c
> > > > index 15c4cedc2759..a2d91d4a3e02 100644
> > > > --- a/drivers/platform/x86/amd/hsmp/acpi.c
> > > > +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> > > > @@ -617,6 +617,10 @@ static int hsmp_acpi_probe(struct platform_device
> > > > *pdev)
> > > > 
> > > >   static void hsmp_acpi_remove(struct platform_device *pdev)
> > > >   {
> > > > +     struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
> > > > +
> > > > +     sock->dev = NULL;
> > > > +
> > > >        guard(mutex)(&hsmp_lock);
> > > >        /*
> > > >         * We register only one misc_device even on multi-socket system.
> > > > diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > b/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > index e05d824045d6..a4420db42781 100644
> > > > --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > @@ -219,6 +219,9 @@ int hsmp_send_message(struct hsmp_message *msg)
> > > >                return -ENODEV;
> > > >        sock = &hsmp_pdev.sock[msg->sock_ind];
> > > > 
> > > > +     if (!sock->dev)
> > > > +             return -ENODEV;
> > > > +
> > > >        ret = down_interruptible(&sock->hsmp_sem);
> > > >        if (ret < 0)
> > > >                return ret;
> > > This is still racy. AFAICT, nothing prevents assigning NULL into sock->dev
> > > because of remove while hsmp_send_message() is running and that can result
> > > in dereferencing NULL.
> > 
> > Could you please suggest on how to resolve this issue?

Maybe rwsem would help, remove takes it for write and accessor for read.
But I've not really looked deeply into the details (summer time and 
everything).

> I will relocate hsmp_fops to acpi.c and plat.c from hsmp.c. Consequently, the
> module reference
> 
> count for hsmp_acpi should be incremented when the device file is opened.
> 
> Does this solution address the issue adequately? Is it acceptable?

I'm not sure how that addresses the NULL assignment, but perhaps I'll just 
need to see the code to follow it or maybe you were trying to suggest a 
solution into some entirely different problem?

-- 
 i.