pseries/papr-hvpipe: Fix deadlock, races and misc cleanups
Subsystems (2)
Ritesh Harjani (IBM)
reviewerv2 cover pseries/papr-hvpipe: Fix deadlock, races and misc cleanups
While going over papr-hvpipe code, there were a few races & a possible recursive deadlock identified. This patch series is an attempt to fix those along with some misc cleanups. The changes are looking sane to me in v2, so I would appreciate any reviews, however it still hasn't been tested on the real HW, which I will test and update.
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) (10):
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 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: Simplify error handling in papr_hvpipe_init() pseries/papr-hvpipe: Fix style and checkpatch issues in enable_hvpipe_IRQ() pseries/papr-hvpipe: Fix error handling in case of init routine fails
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
Ritesh Harjani (IBM)
reviewerRFC v2 1/10 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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +11 -9
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 2/10 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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +1 -1
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 3/10 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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 57 ++++++++++---------- 1 file changed, 30 insertions(+), 27 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +30 -27
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 4/10 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 simply return -EFAULT from the ->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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 23 ++++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +14 -9
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 5/10 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()
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +2 -3
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 6/10 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.
--- 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
@@ -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
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 7/10 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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 52 ++++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +26 -26
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 8/10 pseries/papr-hvpipe: 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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 26 +++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +15 -11
@@ -783,23 +783,27 @@ 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 =set_hvpipe_sys_param (1 ); +if (ret) + goto out_wq; -pr_err ("hvpipe feature is not enabled %d\n" , ret); + ret =misc_register (&papr_hvpipe_dev); +if (ret) + goto out_wq; + +pr_info ("hvpipe feature is enabled\n" ); + hvpipe_feature = true; +return 0 ; + +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
Ritesh Harjani (IBM)
reviewerRFC v2 9/10 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 " ==".
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +6 -7
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 10/10 pseries/papr-hvpipe: Fix error handling in case of init routine fails
In reply to Ritesh Harjani (IBM)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().
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +5 -3
@@ -785,18 +785,20 @@ static int __init papr_hvpipe_init(void)if (ret) goto out_wq; - ret =set_hvpipe_sys_param (1 ); + ret =misc_register (&papr_hvpipe_dev);if (ret) goto out_wq; - ret =misc_register (&papr_hvpipe_dev); + ret =set_hvpipe_sys_param (1 );if (ret) - goto out_wq; + 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:
--
2.39.5