Skip to contentSkip to conversation

pseries/papr-hvpipe: Fix deadlock, races and misc cleanups

PATCH Applied Ritesh Harjani (IBM) 3 participants 13 messages May 1 – May 11

Subsystems (2)
Stable branch PowerPC
lore
Share & tools
lore

Ritesh Harjani (IBM)

author reviewer
#0 lore raw

v3 cover pseries/papr-hvpipe: Fix deadlock, races and misc cleanups

While going over papr-hvpipe code, there were a few fixes which were identified. This patch series is an attempt to fix those along with some misc cleanups. Me and Haren are trying to get these patches verified on a real HW. The tests are not straight forward and we are waiting for the results.
Will update on the test results once we hear back from the internal test team.

v2->v3:
======
1. Rearranged the patches in such a way that it is easier to backport the fixes if required.
2. Clubbed patch-8 and patch-10 (of v2) since they both were changing the same function.
3. Handled ret>=0 case in copy_to_user patch, when the user itself may request for 0 effective bytes (after the HDR_LEN).

[v2]: https://lore.kernel.org/linuxppc-dev/[email protected]/

v1->v2:
========
1. Fix a possible deadlock due to use of spin_lock instead of spin_lock_irqsave.
2. Prevent kernel stack uninit memory leak to userspace
3. Fix the race condition in null-ptr-deref case where there may be an msg pending to be consumed from the hvpipe.
4. Fixed error handling in init routine in patch-10

[v1]: https://lore.kernel.org/linuxppc-dev/[email protected]/#t

Ritesh Harjani (IBM) (9):
pseries/papr-hvpipe: Fix race with interrupt handler pseries/papr-hvpipe: Prevent kernel stack memory leak to userspace pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle() pseries/papr-hvpipe: Fix & simplify error handling in papr_hvpipe_init() pseries/papr-hvpipe: Fix the usage of copy_to_user() pseries/papr-hvpipe: Simplify spin unlock usage in papr_hvpipe_handle_release() pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg() pseries/papr-hvpipe: Fix style and checkpatch issues in enable_hvpipe_IRQ()

 arch/powerpc/platforms/pseries/papr-hvpipe.c | 181 ++++++++++---------
 arch/powerpc/platforms/pseries/papr-hvpipe.h |   1 -
 2 files changed, 97 insertions(+), 85 deletions(-)

--
2.39.5

author reviewer
#1 lore raw

PATCH v3 1/9 pseries/papr-hvpipe: Fix race with interrupt handler

While executing ->ioctl handler or ->release handler, if an interrupt fires on the same cpu, then we can enter into a deadlock.

This patch fixes both these handlers to take spin_lock_irq{save|restore} versions of the lock to prevent this deadlock.

Cc [email protected]
Fixes 814ef095f12c9 ("powerpc/pseries: Add papr-hvpipe char driver for HVPIPE interfaces")
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +11 -9
index 14ae480d060a..c41d45e1986d 100644
@@ -444,13 +444,14 @@ static int papr_hvpipe_handle_release(struct inode *inode,
 				struct file *file)
 {
 	struct hvpipe_source_info *src_info;
+	unsigned long flags;
 
 	/*
 	 * Hold the lock, remove source from src_list, reset the
 	 * hvpipe status and release the lock to prevent any race
 	 * with message event IRQ.
 	 */
-	spin_lock(&hvpipe_src_list_lock);
+	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
 	src_info = file->private_data;
 	list_del(&src_info->list);
 	file->private_data = NULL;

@@ -461,10 +462,10 @@ static int papr_hvpipe_handle_release(struct inode *inode,
 	 */
 	if (src_info->hvpipe_status & HVPIPE_MSG_AVAILABLE) {
 		src_info->hvpipe_status = 0;
-		spin_unlock(&hvpipe_src_list_lock);
+		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 		hvpipe_rtas_recv_msg(NULL, 0);
 	} else
-		spin_unlock(&hvpipe_src_list_lock);
+		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 
 	kfree(src_info);
 	return 0;

@@ -480,20 +481,21 @@ static const struct file_operations papr_hvpipe_handle_ops = {
 static int papr_hvpipe_dev_create_handle(u32 srcID)
 {
 	struct hvpipe_source_info *src_info __free(kfree) = NULL;
+	unsigned long flags;
 
-	spin_lock(&hvpipe_src_list_lock);
+	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
 	/*
 	 * Do not allow more than one process communicates with
 	 * each source.
 	 */
 	src_info = hvpipe_find_source(srcID);
 	if (src_info) {
-		spin_unlock(&hvpipe_src_list_lock);
+		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 		pr_err("pid(%d) is already using the source(%d)\n",
 				src_info->tsk->pid, srcID);
 		return -EALREADY;
 	}
-	spin_unlock(&hvpipe_src_list_lock);
+	spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 
 	src_info = kzalloc_obj(*src_info, GFP_KERNEL_ACCOUNT);
 	if (!src_info)

@@ -510,18 +512,18 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 		return fdf.err;
 
 	retain_and_null_ptr(src_info);
-	spin_lock(&hvpipe_src_list_lock);
+	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
 	/*
 	 * If two processes are executing ioctl() for the same
 	 * source ID concurrently, prevent the second process to
 	 * acquire FD.
 	 */
 	if (hvpipe_find_source(srcID)) {
-		spin_unlock(&hvpipe_src_list_lock);
+		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 		return -EALREADY;
 	}
 	list_add(&src_info->list, &hvpipe_src_list);
-	spin_unlock(&hvpipe_src_list_lock);
+	spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 	return fd_publish(fdf);
 }
Signature · 1 line
2.39.5
author reviewer
#2 lore raw

PATCH v3 2/9 pseries/papr-hvpipe: Prevent kernel stack memory leak to userspace

In reply to Ritesh Harjani (IBM)

The hdr variable is allocated on the stack and only hdr.version and hdr.flags are initialized explicitly. Because the struct papr_hvpipe_hdr contains reserved padding bytes (reserved[3] and reserved2[40]), these could leak the uninitialized bytes to userspace after copy_to_user().

This patch fixes that by initializing the whole struct to 0.

Cc [email protected]
Fixes cebdb522fd3ed ("powerpc/pseries: Receive payload with ibm,receive-hvpipe-msg RTAS")
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +1 -1
index c41d45e1986d..3392874ebdf6 100644
@@ -327,7 +327,7 @@ static ssize_t papr_hvpipe_handle_read(struct file *file,
 {
 
 	struct hvpipe_source_info *src_info = file->private_data;
-	struct papr_hvpipe_hdr hdr;
+	struct papr_hvpipe_hdr hdr = {};
 	long ret;
 
 	/*
Signature · 1 line
2.39.5
author reviewer
#3 lore raw

PATCH v3 3/9 pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle()

In reply to Ritesh Harjani (IBM)

commit 6d3789d347a7 ("papr-hvpipe: convert papr_hvpipe_dev_create_handle() to FD_PREPARE()"), changed the create handle to FD_PREPARE(), but it caused kernel
null-ptr-deref because after call to retain_and_null_ptr(src_info),
src_info is re-used for adding it to the global list.

Getting the following kernel panic in papr_hvpipe_dev_create_handle() when trying to add src_info to the list.
Kernel attempted to write user page (0) - exploit attempt? (uid: 0)

 BUG: Kernel NULL pointer dereference on write at 0x00000000
 Faulting instruction address: 0xc0000000001b44a0
 Oops: Kernel access of bad area, sig: 11 [#1]
 ...
 Call Trace:
 papr_hvpipe_dev_ioctl+0x1f4/0x48c (unreliable)
 sys_ioctl+0x528/0x1064
 system_call_exception+0x128/0x360
 system_call_vectored_common+0x15c/0x2ec

Now, the error handling with FD_PREPARE's file cleanup and __free(kfree) auto cleanup is getting too convoluted. This is mainly because we need to ensure only 1 user get the srcID handle. To simplify this, we allocate prepare the src_info in the beginning and add it to the global list under a spinlock after checking that no duplicates exist.

This simplify the error handling where if the FD_ADD fails, we can simply remove the src_info from the list and consume any pending msg in hvpipe to be cleared, after src_info became visible in the global list.

Cc Christian Brauner <[email protected]>
Cc [email protected]
Fixes 6d3789d347a7 ("papr-hvpipe: convert papr_hvpipe_dev_create_handle() to FD_PREPARE()")
Reported-by Haren Myneni <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 57 ++++++++++----------
 1 file changed, 30 insertions(+), 27 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +30 -27
index 3392874ebdf6..402781299497 100644
@@ -480,23 +480,10 @@ static const struct file_operations papr_hvpipe_handle_ops = {
 
 static int papr_hvpipe_dev_create_handle(u32 srcID)
 {
-	struct hvpipe_source_info *src_info __free(kfree) = NULL;
+	struct hvpipe_source_info *src_info;
+	int fd;
 	unsigned long flags;
 
-	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
-	/*
-	 * Do not allow more than one process communicates with
-	 * each source.
-	 */
-	src_info = hvpipe_find_source(srcID);
-	if (src_info) {
-		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
-		pr_err("pid(%d) is already using the source(%d)\n",
-				src_info->tsk->pid, srcID);
-		return -EALREADY;
-	}
-	spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
-
 	src_info = kzalloc_obj(*src_info, GFP_KERNEL_ACCOUNT);
 	if (!src_info)
 		return -ENOMEM;

@@ -505,26 +492,42 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 	src_info->tsk = current;
 	init_waitqueue_head(&src_info->recv_wqh);
 
-	FD_PREPARE(fdf, O_RDONLY | O_CLOEXEC,
-		   anon_inode_getfile("[papr-hvpipe]", &papr_hvpipe_handle_ops,
-				      (void *)src_info, O_RDWR));
-	if (fdf.err)
-		return fdf.err;
-
-	retain_and_null_ptr(src_info);
-	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
 	/*
-	 * If two processes are executing ioctl() for the same
-	 * source ID concurrently, prevent the second process to
-	 * acquire FD.
+	 * Do not allow more than one process communicates with
+	 * each source.
 	 */
+	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
 	if (hvpipe_find_source(srcID)) {
 		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
+		pr_err("pid(%d) could not get the source(%d)\n",
+				src_info->tsk->pid, srcID);
+		kfree(src_info);
 		return -EALREADY;
 	}
 	list_add(&src_info->list, &hvpipe_src_list);
 	spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
-	return fd_publish(fdf);
+
+	fd = FD_ADD(O_RDONLY | O_CLOEXEC,
+		   anon_inode_getfile("[papr-hvpipe]", &papr_hvpipe_handle_ops,
+				      (void *)src_info, O_RDWR));
+	if (fd < 0) {
+		spin_lock_irqsave(&hvpipe_src_list_lock, flags);
+		list_del(&src_info->list);
+		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
+		/*
+		 * if we fail to add FD, that means no userspace program is
+		 * polling. In that case if there is a msg pending because the
+		 * interrupt was fired after the src_info was added to the
+		 * global list, then let's consume it here, to unblock the
+		 * hvpipe
+		 */
+		if (src_info->hvpipe_status & HVPIPE_MSG_AVAILABLE)
+			hvpipe_rtas_recv_msg(NULL, 0);
+		kfree(src_info);
+		return fd;
+	}
+
+	return fd;
 }
 
 /*
Signature · 1 line
2.39.5
author reviewer
#4 lore raw

PATCH v3 4/9 pseries/papr-hvpipe: Fix & simplify error handling in papr_hvpipe_init()

In reply to Ritesh Harjani (IBM)

Remove such 3 levels of nesting patterns to check success return values from function calls.

ret = enable_hvpipe_IRQ()
if (!ret)
ret = set_hvpipe_sys_param(1)
if (!ret)
ret = misc_register()

Instead just bail out to "out*:" labels, in case of any error. This simplifies the init flow.

While at it let's also fix the following error handling logic:
We have already enabled interrupt sources and enabled hvpipe to received interrupts, if misc_register() fails, we will destroy the workqueue, but the HMC might send us a msg via hvpipe which will call, queue work on the workqueue which might be destroyed.

So instead, let's reverse the order of enabling set_hvpipe_sys_param(1) and in case of an error let's remove the misc dev by calling misc_deregister().

Cc [email protected]
Fixes 39a08a4f94980 ("powerpc/pseries: Enable hvpipe with ibm,set-system-parameter RTAS")
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 28 ++++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +17 -11
index 402781299497..800649f309a5 100644
@@ -780,23 +780,29 @@ static int __init papr_hvpipe_init(void)
 	}
 
 	ret = enable_hvpipe_IRQ();
-	if (!ret) {
-		ret = set_hvpipe_sys_param(1);
-		if (!ret)
-			ret = misc_register(&papr_hvpipe_dev);
-	}
+	if (ret)
+		goto out_wq;
 
-	if (!ret) {
-		pr_info("hvpipe feature is enabled\n");
-		hvpipe_feature = true;
-		return 0;
-	}
+	ret = misc_register(&papr_hvpipe_dev);
+	if (ret)
+		goto out_wq;
 
-	pr_err("hvpipe feature is not enabled %d\n", ret);
+	ret = set_hvpipe_sys_param(1);
+	if (ret)
+		goto out_misc;
+
+	pr_info("hvpipe feature is enabled\n");
+	hvpipe_feature = true;
+	return 0;
+
+out_misc:
+	misc_deregister(&papr_hvpipe_dev);
+out_wq:
 	destroy_workqueue(papr_hvpipe_wq);
 out:
 	kfree(papr_hvpipe_work);
 	papr_hvpipe_work = NULL;
+	pr_err("hvpipe feature is not enabled %d\n", ret);
 	return ret;
 }
 machine_device_initcall(pseries, papr_hvpipe_init);
Signature · 1 line
2.39.5
author reviewer
#5 lore raw

PATCH v3 5/9 pseries/papr-hvpipe: Fix the usage of copy_to_user()

In reply to Ritesh Harjani (IBM)

copy_to_user() return bytes_not_copied to the user buffer. If there was an error writing bytes into the user buffer, i.e. if copy_to_user returns a non-zero value, then we should simply return -EFAULT from the ->read() call.

Otherwise, in the non-patched version, we may end up mixing "bytes_not_copied + bytes_copied (HVPIPE_HDR_LEN)" as the return value to the user in ->read() call

Also let's make sure we clear the hvpipe_status flag, if we have consumed the hvpipe msg by making the rtas call. ret = -EFAULT means copy_to_user has failed but that still means that the msg was read from the hvpipe, hence for both cases, success & -EFAULT, we should clear the HVPIPE_MSG_AVAILABLE flag in hvpipe_status.

Cc [email protected]
Fixes cebdb522fd3edd1 ("powerpc/pseries: Receive payload with ibm,receive-hvpipe-msg RTAS")
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 23 ++++++++++++--------
 1 file changed, 14 insertions(+), 9 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +14 -9
index 800649f309a5..c007560d2d8c 100644
@@ -206,10 +206,11 @@ static int hvpipe_rtas_recv_msg(char __user *buf, int size)
 					bytes_written, size);
 				bytes_written = size;
 			}
-			ret = copy_to_user(buf,
+			if (copy_to_user(buf,
 					rtas_work_area_raw_buf(work_area),
-					bytes_written);
-			if (!ret)
+					bytes_written))
+				ret = -EFAULT;
+			else
 				ret = bytes_written;
 		}
 	} else {

@@ -328,7 +329,7 @@ static ssize_t papr_hvpipe_handle_read(struct file *file,
 
 	struct hvpipe_source_info *src_info = file->private_data;
 	struct papr_hvpipe_hdr hdr = {};
-	long ret;
+	ssize_t ret = 0;
 
 	/*
 	 * Return -ENXIO during migration

@@ -376,7 +377,7 @@ static ssize_t papr_hvpipe_handle_read(struct file *file,
 
 	ret = copy_to_user(buf, &hdr, HVPIPE_HDR_LEN);
 	if (ret)
-		return ret;
+		return -EFAULT;
 
 	/*
 	 * Message event has payload, so get the payload with

@@ -385,19 +386,23 @@ static ssize_t papr_hvpipe_handle_read(struct file *file,
 	if (hdr.flags & HVPIPE_MSG_AVAILABLE) {
 		ret = hvpipe_rtas_recv_msg(buf + HVPIPE_HDR_LEN,
 				size - HVPIPE_HDR_LEN);
-		if (ret > 0) {
+		/*
+		 * Always clear MSG_AVAILABLE once the RTAS call has drained
+		 * the message, regardless of whether copy_to_user succeeded.
+		 */
+		if (ret >= 0 || ret == -EFAULT)
 			src_info->hvpipe_status &= ~HVPIPE_MSG_AVAILABLE;
-			ret += HVPIPE_HDR_LEN;
-		}
 	} else if (hdr.flags & HVPIPE_LOST_CONNECTION) {
 		/*
 		 * Hypervisor is closing the pipe for the specific
 		 * source. So notify user space.
 		 */
 		src_info->hvpipe_status &= ~HVPIPE_LOST_CONNECTION;
-		ret = HVPIPE_HDR_LEN;
 	}
 
+	if (ret >= 0)
+		ret += HVPIPE_HDR_LEN;
+
 	return ret;
 }
Signature · 1 line
2.39.5
author reviewer
#6 lore raw

PATCH v3 6/9 pseries/papr-hvpipe: Simplify spin unlock usage in papr_hvpipe_handle_release()

In reply to Ritesh Harjani (IBM)

Once the src_info is removed from the global list, no one can access it. This simplies the usage of spin_unlock_irqrestore() in papr_hvpipe_handle_release()

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +2 -3
index c007560d2d8c..5aa37f6ad8c9 100644
@@ -460,6 +460,7 @@ static int papr_hvpipe_handle_release(struct inode *inode,
 	src_info = file->private_data;
 	list_del(&src_info->list);
 	file->private_data = NULL;
+	spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 	/*
 	 * If the pipe for this specific source has any pending
 	 * payload, issue recv HVPIPE RTAS so that pipe will not

@@ -467,10 +468,8 @@ static int papr_hvpipe_handle_release(struct inode *inode,
 	 */
 	if (src_info->hvpipe_status & HVPIPE_MSG_AVAILABLE) {
 		src_info->hvpipe_status = 0;
-		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
 		hvpipe_rtas_recv_msg(NULL, 0);
-	} else
-		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
+	}
 
 	kfree(src_info);
 	return 0;
Signature · 1 line
2.39.5
author reviewer
#7 lore raw

PATCH v3 7/9 pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info

In reply to Ritesh Harjani (IBM)

We don't really use task_struct pointer for anything meaningful. So just kill it for now, and we can bring back later if we need this for any future debug purposes.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 5 ++---
 arch/powerpc/platforms/pseries/papr-hvpipe.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +2 -3
index 5aa37f6ad8c9..46159f1c1cf1 100644
@@ -493,7 +493,6 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 		return -ENOMEM;
 
 	src_info->srcID = srcID;
-	src_info->tsk = current;
 	init_waitqueue_head(&src_info->recv_wqh);
 
 	/*

@@ -503,8 +502,8 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 	spin_lock_irqsave(&hvpipe_src_list_lock, flags);
 	if (hvpipe_find_source(srcID)) {
 		spin_unlock_irqrestore(&hvpipe_src_list_lock, flags);
-		pr_err("pid(%d) could not get the source(%d)\n",
-				src_info->tsk->pid, srcID);
+		pr_err("pid(%s:%d) could not get the source(%d)\n",
+				current->comm, task_pid_nr(current), srcID);
 		kfree(src_info);
 		return -EALREADY;
 	}
arch/powerpc/platforms/pseries/papr-hvpipe.h +0 -1
index c343f4230865..4bdf7bb2fc4d 100644
@@ -21,7 +21,6 @@ struct hvpipe_source_info {
 	u32 srcID;
 	u32 hvpipe_status;
 	wait_queue_head_t recv_wqh;	 /* wake up poll() waitq */
-	struct task_struct *tsk;
 };
 
 /*
Signature · 1 line
2.39.5
author reviewer
#8 lore raw

PATCH v3 8/9 pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg()

In reply to Ritesh Harjani (IBM)

Simplify hvpipe_rtas_recv_msg() by removing three levels of nesting... if (!ret)
if (buf)
if (size < bytes_written)
... this refactoring of the function bails out to "out:" label first, in case of any error. This simplifies the init flow.

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 52 ++++++++++----------
 1 file changed, 26 insertions(+), 26 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +26 -26
index 46159f1c1cf1..3688b2be0445 100644
@@ -190,34 +190,34 @@ static int hvpipe_rtas_recv_msg(char __user *buf, int size)
 		return -ENOMEM;
 	}
 
-	ret = rtas_ibm_receive_hvpipe_msg(work_area, &srcID,
-					&bytes_written);
-	if (!ret) {
-		/*
-		 * Recv HVPIPE RTAS is successful.
-		 * When releasing FD or no one is waiting on the
-		 * specific source, issue recv HVPIPE RTAS call
-		 * so that pipe is not blocked - this func is called
-		 * with NULL buf.
-		 */
-		if (buf) {
-			if (size < bytes_written) {
-				pr_err("Received the payload size = %d, but the buffer size = %d\n",
-					bytes_written, size);
-				bytes_written = size;
-			}
-			if (copy_to_user(buf,
-					rtas_work_area_raw_buf(work_area),
-					bytes_written))
-				ret = -EFAULT;
-			else
-				ret = bytes_written;
-		}
-	} else {
-		pr_err("ibm,receive-hvpipe-msg failed with %d\n",
-				ret);
+	/*
+	 * Recv HVPIPE RTAS is successful.
+	 * When releasing FD or no one is waiting on the
+	 * specific source, issue recv HVPIPE RTAS call
+	 * so that pipe is not blocked - this func is called
+	 * with NULL buf.
+	 */
+	ret = rtas_ibm_receive_hvpipe_msg(work_area, &srcID, &bytes_written);
+	if (ret) {
+		pr_err("ibm,receive-hvpipe-msg failed with %d\n", ret);
+		goto out;
 	}
 
+	if (!buf)
+		goto out;
+
+	if (size < bytes_written) {
+		pr_err("Received the payload size = %d, but the buffer size = %d\n",
+				bytes_written, size);
+		bytes_written = size;
+	}
+
+	if (copy_to_user(buf, rtas_work_area_raw_buf(work_area), bytes_written))
+		ret = -EFAULT;
+	else
+		ret = bytes_written;
+
+out:
 	rtas_work_area_free(work_area);
 	return ret;
 }

--
2.39.5

author reviewer
#9 lore raw

PATCH v3 9/9 pseries/papr-hvpipe: Fix style and checkpatch issues in enable_hvpipe_IRQ()

In reply to Ritesh Harjani (IBM)

While at it let's also fix the similar style issue in enable_hvpipe_IRQ() function. This also fixes a minor checkpatch warning which I got due to an extra space before " ==".

Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +6 -7
index 3688b2be0445..0c40bdde45e2 100644
@@ -693,20 +693,19 @@ static int __init enable_hvpipe_IRQ(void)
 	struct device_node *np;
 
 	hvpipe_check_exception_token = rtas_function_token(RTAS_FN_CHECK_EXCEPTION);
-	if (hvpipe_check_exception_token  == RTAS_UNKNOWN_SERVICE)
+	if (hvpipe_check_exception_token == RTAS_UNKNOWN_SERVICE)
 		return -ENODEV;
 
 	/* hvpipe events */
 	np = of_find_node_by_path("/event-sources/ibm,hvpipe-msg-events");
-	if (np != NULL) {
-		request_event_sources_irqs(np, hvpipe_event_interrupt,
-					"HPIPE_EVENT");
-		of_node_put(np);
-	} else {
-		pr_err("Can not enable hvpipe event IRQ\n");
+	if (!np) {
+		pr_err("No device node found, could not enable hvpipe event IRQ\n");
 		return -ENODEV;
 	}
 
+	request_event_sources_irqs(np, hvpipe_event_interrupt, "HPIPE_EVENT");
+	of_node_put(np);
+
 	return 0;
 }
Signature · 1 line
2.39.5
#10 lore raw
In reply to Ritesh Harjani (IBM)
Quoted 6 lines · Ritesh Harjani (IBM)

While going over papr-hvpipe code, there were a few fixes which were identified. This patch series is an attempt to fix those along with some misc cleanups. Me and Haren are trying to get these patches verified on a real HW. The tests are not straight forward and we are waiting for the results.
Will update on the test results once we hear back from the internal test team.

Quoted 7 lines

v2->v3:
======
1. Rearranged the patches in such a way that it is easier to backport the fixes if required.
2. Clubbed patch-8 and patch-10 (of v2) since they both were changing the same function.
3. Handled ret>=0 case in copy_to_user patch, when the user itself may request for 0 effective bytes (after the HDR_LEN).

Since this is CCed to stable, it is currently being evaluated by RSCT. We can merge it once we receive an Acked-by from RSCT.

Thanks,
Aboorva

Quoted 30 lines

[v2]: https://lore.kernel.org/linuxppc-dev/[email protected]/

v1->v2:
========
1. Fix a possible deadlock due to use of spin_lock instead of spin_lock_irqsave.
2. Prevent kernel stack uninit memory leak to userspace
3. Fix the race condition in null-ptr-deref case where there may be an msg pending to be consumed from the hvpipe.
4. Fixed error handling in init routine in patch-10

[v1]: https://lore.kernel.org/linuxppc-dev/[email protected]/#t

Ritesh Harjani (IBM) (9):
  pseries/papr-hvpipe: Fix race with interrupt handler pseries/papr-hvpipe: Prevent kernel stack memory leak to userspace pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle() pseries/papr-hvpipe: Fix & simplify error handling in papr_hvpipe_init() pseries/papr-hvpipe: Fix the usage of copy_to_user() pseries/papr-hvpipe: Simplify spin unlock usage in papr_hvpipe_handle_release() pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg() pseries/papr-hvpipe: Fix style and checkpatch issues in enable_hvpipe_IRQ()

 arch/powerpc/platforms/pseries/papr-hvpipe.c | 181 ++++++++++--------- arch/powerpc/platforms/pseries/papr-hvpipe.h |   1 -

 2 files changed, 97 insertions(+), 85 deletions(-)

--
2.39.5

#11 lore raw
Quoted 16 lines · Aboorva Devarajan
Quoted 6 lines · Ritesh Harjani (IBM)

While going over papr-hvpipe code, there were a few fixes which were identified. This patch series is an attempt to fix those along with some misc cleanups. Me and Haren are trying to get these patches verified on a real HW. The tests are not straight forward and we are waiting for the results.
Will update on the test results once we hear back from the internal test team.

Quoted 7 lines

v2->v3:
======
1. Rearranged the patches in such a way that it is easier to backport the fixes if required.
2. Clubbed patch-8 and patch-10 (of v2) since they both were changing the same function.
3. Handled ret>=0 case in copy_to_user patch, when the user itself may request for 0 effective bytes (after the HDR_LEN).

Quoted 4 lines

Since this is CCed to stable, it is currently being evaluated by RSCT. We can merge it once we receive an Acked-by from RSCT.

An update from RSCT: with the patch, the earlier issues observed are now resolved, and the inband RMC connection is successfully established with the patched kernel.

Thanks,
Aboorva

Quoted 31 lines
Quoted 31 lines

[v2]: https://lore.kernel.org/linuxppc-dev/[email protected]/

v1->v2:
========
1. Fix a possible deadlock due to use of spin_lock instead of spin_lock_irqsave.
2. Prevent kernel stack uninit memory leak to userspace
3. Fix the race condition in null-ptr-deref case where there may be an msg pending to be consumed from the hvpipe.
4. Fixed error handling in init routine in patch-10

[v1]: https://lore.kernel.org/linuxppc-dev/[email protected]/#t

Ritesh Harjani (IBM) (9):
  pseries/papr-hvpipe: Fix race with interrupt handler pseries/papr-hvpipe: Prevent kernel stack memory leak to userspace pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle() pseries/papr-hvpipe: Fix & simplify error handling in papr_hvpipe_init() pseries/papr-hvpipe: Fix the usage of copy_to_user() pseries/papr-hvpipe: Simplify spin unlock usage in papr_hvpipe_handle_release() pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg() pseries/papr-hvpipe: Fix style and checkpatch issues in enable_hvpipe_IRQ()

 arch/powerpc/platforms/pseries/papr-hvpipe.c | 181 ++++++++++--------- arch/powerpc/platforms/pseries/papr-hvpipe.h |   1 -

 2 files changed, 97 insertions(+), 85 deletions(-)

--
2.39.5

maintainer Applied
#12 lore raw
In reply to Ritesh Harjani (IBM)
Quoted 14 lines · Ritesh Harjani (IBM)

While going over papr-hvpipe code, there were a few fixes which were identified. This patch series is an attempt to fix those along with some misc cleanups. Me and Haren are trying to get these patches verified on a real HW. The tests are not straight forward and we are waiting for the results.
Will update on the test results once we hear back from the internal test team.

v2->v3:
======
1. Rearranged the patches in such a way that it is easier to backport the fixes if required.
2. Clubbed patch-8 and patch-10 (of v2) since they both were changing the same function.
3. Handled ret>=0 case in copy_to_user patch, when the user itself may request for 0 effective bytes (after the HDR_LEN).

Quoted 1 line

[...]

Applied to powerpc/fixes.

[1/9] pseries/papr-hvpipe: Fix race with interrupt handler

      https://git.kernel.org/powerpc/c/7a4f0846ee6cc8cf44ae0046ed42e3259d1dd45b
[2/9] pseries/papr-hvpipe: Prevent kernel stack memory leak to userspace
      https://git.kernel.org/powerpc/c/cefeed44296261173a806bef988b26bc565da4be
[3/9] pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle()
      https://git.kernel.org/powerpc/c/1b9f7aafa44f5ce852c00509104d10fd9eb0f402
[4/9] pseries/papr-hvpipe: Fix & simplify error handling in papr_hvpipe_init()
      https://git.kernel.org/powerpc/c/713e468cdbc2277db6ce949c32c1acbd83501733
[5/9] pseries/papr-hvpipe: Fix the usage of copy_to_user()
      https://git.kernel.org/powerpc/c/d48654bd8b1a75f662e224d257db54de475120dc
[6/9] pseries/papr-hvpipe: Simplify spin unlock usage in papr_hvpipe_handle_release()
      https://git.kernel.org/powerpc/c/2eeac577480848801b35885b3a8201aa35f46236
[7/9] pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info
      https://git.kernel.org/powerpc/c/4e2d83c80495a9327141e8636f25dde13155f14f
[8/9] pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg()
      https://git.kernel.org/powerpc/c/fe53d2ae82c06aa2d6402624af01e8f8ddfcd5b3
[9/9] pseries/papr-hvpipe: Fix style and checkpatch issues in enable_hvpipe_IRQ()
      https://git.kernel.org/powerpc/c/629d1a901de57490d29a495273df11e64993ec04

cheers