From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:09:54
On power9, Virtual Accelerator Switchboard (VAS) allows user space or
kernel to communicate with Nest Accelerator (NX) directly using COPY/PASTE
instructions. NX provides various functionalities such as compression,
encryption and etc. But only compression (842 and GZIP formats) is
supported in Linux kernel on power9.
842 compression driver (drivers/crypto/nx/nx-842-powernv.c)
is already included in Linux. Only GZIP support will be available from
user space.
Applications can issue GZIP compression / decompression requests to NX with
COPY/PASTE instructions. When NX is processing these requests, can hit
fault on the request buffer (not in memory). It issues an interrupt and
pastes fault CRB in fault FIFO. Expects kernel to handle this fault and
return credits for both send and fault windows after processing.
This patch series adds IRQ and fault window setup, and NX fault handling:
- Alloc IRQ and trigger port address, and configure IRQ per VAS instance.
- Set port# for each window to generate an interrupt when noticed fault.
- Set fault window and FIFO on which NX paste fault CRB.
- Setup IRQ thread fault handler per VAS instance.
- When receiving an interrupt, Read CRBs from fault FIFO and update
coprocessor_status_block (CSB) in the corresponding CRB with translation
failure (CSB_CC_TRANSLATION). After issuing NX requests, process polls
on CSB address. When it sees translation error, can touch the request
buffer to bring the page in to memory and reissue NX request.
- If copy_to_user fails on user space CSB address, OS sends SEGV signal.
Tested these patches with NX-GZIP support and will be posting this series
soon.
Patches 1 & 2: Define alloc IRQ and get port address per chip which are needed
to alloc IRQ per VAS instance.
Patch 3: Define nx_fault_stamp on which NX writes fault status for the fault
CRB
Patch 4: Alloc and setup IRQ and trigger port address for each VAS instance
Patch 5: Setup fault window per each VAS instance. This window is used for
NX to paste fault CRB in FIFO.
Patches 6 & 7: Setup threaded IRQ per VAS and register NX with fault window
ID and port number for each send window so that NX paste fault CRB
in this window.
Patch 8: Reference to pid and mm so that pid is not used until window closed.
Needed for multi thread application where child can open a window
and can be used by parent later.
Patches 9 and 10: Process CRBs from fault FIFO and notify tasks by
updating CSB or through signals.
Patches 11 and 12: Return credits for send and fault windows after handling
faults.
Patch 14:Fix closing send window after all credits are returned. This issue
happens only for user space requests. No page faults on kernel
request buffer.
Changelog:
V2:
- Use threaded IRQ instead of own kernel thread handler
- Use pswid instead of user space CSB address to find valid CRB
- Removed unused macros and other changes as suggested by Christoph Hellwig
V3:
- Rebased to 5.5-rc2
- Use struct pid * instead of pid_t for vas_window tgid
- Code cleanup as suggested by Christoph Hellwig
V4:
- Define xive alloc and get IRQ info based on chip ID and use these
functions for IRQ setup per VAS instance. It eliminates skiboot
dependency as suggested by Oliver.
V5:
- Do not update CSB if the process is exiting (patch9)
V6:
- Add interrupt handler instead of default one and return IRQ_HANDLED
if the fault handling thread is already in progress. (Patch6)
- Use platform send window ID and CCW[0] bit to find valid CRB in
fault FIFO (Patch6).
- Return fault address to user space in BE and other changes as
suggested by Michael Neuling. (patch9)
- Rebased to 5.6-rc4
V7:
- Fix sparse warnings (patches 6,9 and 10)
Haren Myneni (14):
powerpc/xive: Define xive_native_alloc_irq_on_chip()
powerpc/xive: Define xive_native_alloc_get_irq_info()
powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
powerpc/vas: Alloc and setup IRQ and trigger port address
powerpc/vas: Setup fault window per VAS instance
powerpc/vas: Setup thread IRQ handler per VAS instance
powerpc/vas: Register NX with fault window ID and IRQ port value
powerpc/vas: Take reference to PID and mm for user space windows
powerpc/vas: Update CSB and notify process for fault CRBs
powerpc/vas: Print CRB and FIFO values
powerpc/vas: Do not use default credits for receive window
powerpc/vas: Return credits after handling fault
powerpc/vas: Display process stuck message
powerpc/vas: Free send window in VAS instance after credits returned
arch/powerpc/include/asm/icswx.h | 18 +-
arch/powerpc/include/asm/xive.h | 11 +-
arch/powerpc/platforms/powernv/Makefile | 2 +-
arch/powerpc/platforms/powernv/ocxl.c | 20 +-
arch/powerpc/platforms/powernv/vas-debug.c | 2 +-
arch/powerpc/platforms/powernv/vas-fault.c | 331 ++++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/vas-window.c | 185 ++++++++++++++--
arch/powerpc/platforms/powernv/vas.c | 101 ++++++++-
arch/powerpc/platforms/powernv/vas.h | 51 ++++-
arch/powerpc/sysdev/xive/native.c | 29 ++-
10 files changed, 703 insertions(+), 47 deletions(-)
create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c
--
1.8.3.1
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:13:51
This function allocates IRQ on a specific chip. VAS needs per chip
IRQ allocation and will have IRQ handler per VAS instance.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/include/asm/xive.h | 9 ++++++++-
arch/powerpc/sysdev/xive/native.c | 6 +++---
2 files changed, 11 insertions(+), 4 deletions(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:15:22
pnv_ocxl_alloc_xive_irq() in ocxl.c allocates IRQ and gets trigger port
address. VAS also needs this function, but based on chip ID. So moved
this common function to xive/native.c.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/include/asm/xive.h | 2 ++
arch/powerpc/platforms/powernv/ocxl.c | 20 ++------------------
arch/powerpc/sysdev/xive/native.c | 23 +++++++++++++++++++++++
3 files changed, 27 insertions(+), 18 deletions(-)
@@ -487,24 +487,8 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)intpnv_ocxl_alloc_xive_irq(u32*irq,u64*trigger_addr){-__be64flags,trigger_page;-s64rc;-u32hwirq;--hwirq=xive_native_alloc_irq();-if(!hwirq)-return-ENOENT;--rc=opal_xive_get_irq_info(hwirq,&flags,NULL,&trigger_page,NULL,-NULL);-if(rc||!trigger_page){-xive_native_free_irq(hwirq);-return-ENOENT;-}-*irq=hwirq;-*trigger_addr=be64_to_cpu(trigger_page);-return0;-+returnxive_native_alloc_get_irq_info(OPAL_XIVE_ANY_CHIP,irq,+trigger_addr);}EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:16:54
Kernel sets fault address and status in CRB for NX page fault on user
space address after processing page fault. User space gets the signal
and handles the fault mentioned in CRB by bringing the page in to
memory and send NX request again.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/include/asm/icswx.h | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:18:22
Alloc IRQ and get trigger port address for each VAS instance. Kernel
register this IRQ per VAS instance and sets this port for each send
window. NX interrupts the kernel when it sees page fault.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
arch/powerpc/platforms/powernv/vas.h | 2 ++
2 files changed, 30 insertions(+), 6 deletions(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:21:35
Setup thread IRQ handler per each VAS instance. When NX sees a fault
on CRB, kernel gets an interrupt and vas_fault_handler will be
executed to process fault CRBs. Read all valid CRBs from fault FIFO,
determine the corresponding send window from CRB and process fault
requests.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 90 +++++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/vas-window.c | 60 +++++++++++++++++++
arch/powerpc/platforms/powernv/vas.c | 49 +++++++++++++++-
arch/powerpc/platforms/powernv/vas.h | 6 ++
4 files changed, 204 insertions(+), 1 deletion(-)
@@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)return0;}EXPORT_SYMBOL_GPL(vas_win_close);++structvas_window*vas_pswid_to_window(structvas_instance*vinst,+uint32_tpswid)+{+intwinid;+structvas_window*window;++if(!pswid){+pr_devel("%s: called for pswid 0!\n",__func__);+returnERR_PTR(-ESRCH);+}++decode_pswid(pswid,NULL,&winid);++if(winid>=VAS_WINDOWS_PER_CHIP)+returnERR_PTR(-ESRCH);++/*+*Ifapplicationclosesthewindowbeforethehardware+*returnsthefaultCRB,weshouldwaitinvas_win_close()+*forthependingrequests.sothewindowmustbeactive+*andtheprocessalive.+*+*Ifitsakernelprocess,weshouldnotgetanyfaultsand+*shouldnotgethere.+*/+window=vinst->windows[winid];++if(!window){+pr_err("PSWID decode: Could not find window for winid %d pswid %d vinst 0x%p\n",+winid,pswid,vinst);+returnNULL;+}++/*+*Dosomesanitychecksonthedecodedwindow.Windowshouldbe+*NXGZIPusersendwindow.FTWwindowsshouldnotincurfaults+*sincetheirCRBsareignored(notqueuedonFIFOorprocessed+*byNX).+*/+if(!window->tx_win||!window->user_win||!window->nx_win||+window->cop==VAS_COP_TYPE_FAULT||+window->cop==VAS_COP_TYPE_FTW){+pr_err("PSWID decode: id %d, tx %d, user %d, nx %d, cop %d\n",+winid,window->tx_win,window->user_win,+window->nx_win,window->cop);+WARN_ON(1);+}++returnwindow;+}
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:22:41
For each user space send window, register NX with fault window ID
and port value so that NX paste CRBs in this fault FIFO when it
sees fault on the request buffer.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 15 +++++++++++++--
arch/powerpc/platforms/powernv/vas.h | 15 +++++++++++++++
2 files changed, 28 insertions(+), 2 deletions(-)
@@ -373,7 +373,7 @@ int init_winctx_regs(struct vas_window *window, struct vas_winctx *winctx)init_xlate_regs(window,winctx->user_win);val=0ULL;-val=SET_FIELD(VAS_FAULT_TX_WIN,val,0);+val=SET_FIELD(VAS_FAULT_TX_WIN,val,winctx->fault_win_id);write_hvwc_reg(window,VREG(FAULT_TX_WIN),val);/* In PowerNV, interrupts go to HV. */
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:23:54
Process close windows after its requests are completed. In multi-thread
applications, child can open a window but release FD will not be called
upon its exit. Parent thread will be closing it later upon its exit.
The parent can also send NX requests with this window and NX can
generate page faults. After kernel handles the page fault, send
signal to process by using PID if CSB address is invalid. Parent
thread will not receive signal since its PID is different from the one
saved in vas_window. So use tgid in case if the task for the pid saved
in window is not running and send signal to its parent.
To prevent reusing the pid until the window closed, take reference to
pid and task mm.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-debug.c | 2 +-
arch/powerpc/platforms/powernv/vas-window.c | 53 ++++++++++++++++++++++++++---
arch/powerpc/platforms/powernv/vas.h | 9 ++++-
3 files changed, 57 insertions(+), 7 deletions(-)
@@ -1068,8 +1067,43 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,gotofree_window;}-set_vinst_win(vinst,txwin);+if(txwin->user_win){+/*+*Windowopenedbychildthreadmaynotbeclosedwhen+*itexits.Sotakereferencetoitspidandreleaseit+*whenthewindowisfreebyparentthread.+*Acquireareferencetothetask'spidtomakesure+*pidwillnotbere-used-neededonlyformultithread+*applications.+*/+txwin->pid=get_task_pid(current,PIDTYPE_PID);+/*+*Acquireareferencetothetask'smm.+*/+txwin->mm=get_task_mm(current);+if(!txwin->mm){+put_pid(txwin->pid);+pr_err("VAS: pid(%d): mm_struct is not found\n",+current->pid);+rc=-EPERM;+gotofree_window;+}++mmgrab(txwin->mm);+mmput(txwin->mm);+mm_context_add_copro(txwin->mm);+/*+*Processcloseswindowduringexit.Inthecaseof+*multithreadapplication,childcanopenwindowand+*canexitwithoutclosingit.Expectsparentthread+*touseandclosethewindow.Sodonotneedtotake+*pidreferenceforparentthread.+*/+txwin->tgid=find_get_pid(task_tgid_vnr(current));+}++set_vinst_win(vinst,txwin);returntxwin;free_window:
@@ -1266,8 +1300,17 @@ int vas_win_close(struct vas_window *window)poll_window_castout(window);/* if send window, drop reference to matching receive window */-if(window->tx_win)+if(window->tx_win){+if(window->user_win){+/* Drop references to pid and mm */+put_pid(window->pid);+if(window->mm){+mmdrop(window->mm);+mm_context_remove_copro(window->mm);+}+}put_rx_win(window->rxwin);+}vas_window_free(window);
@@ -353,7 +353,9 @@ struct vas_window {booluser_win;/* True if user space window */void*hvwc_map;/* HV window context */void*uwc_map;/* OS/User window context */-pid_tpid;/* Linux process id of owner */+structpid*pid;/* Linux process id of owner */+structpid*tgid;/* Thread group ID of owner */+structmm_struct*mm;/* Linux process mm_struct */intwcreds_max;/* Window credits */char*dbgname;
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:25:22
For each fault CRB, update fault address in CRB (fault_storage_addr)
and translation error status in CSB so that user space can touch the
fault address and resend the request. If the user space passed invalid
CSB address send signal to process with SIGSEGV.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:27:54
System checkstops if RxFIFO overruns with more requests than the
maximum possible number of CRBs allowed in FIFO at any time. So
max credits value (rxattr.wcreds_max) is set and is passed to
vas_rx_win_open() by the the driver.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 4 ++--
arch/powerpc/platforms/powernv/vas.h | 2 --
2 files changed, 2 insertions(+), 4 deletions(-)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:29:10
NX expects OS to return credit for send window after processing each
fault. Also credit has to be returned even for fault window.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 9 +++++++++
arch/powerpc/platforms/powernv/vas-window.c | 17 +++++++++++++++++
arch/powerpc/platforms/powernv/vas.h | 1 +
3 files changed, 27 insertions(+)
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:30:25
NX may be processing requests while trying to close window. Wait until
all credits are returned and then free send window from VAS instance.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -1317,14 +1317,14 @@ int vas_win_close(struct vas_window *window)unmap_paste_region(window);-clear_vinst_win(window);-poll_window_busy_state(window);unpin_close_window(window);poll_window_credits(window);+clear_vinst_win(window);+poll_window_castout(window);/* if send window, drop reference to matching receive window */
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-06 20:32:12
Process can not close send window until all requests are processed.
Means wait until window state is not busy and send credits are
returned. Display debug messages in case taking longer to close the
window.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-03-17 04:10:53
Haren Myneni [off-list ref] writes:
Process close windows after its requests are completed. In multi-thread
applications, child can open a window but release FD will not be called
upon its exit. Parent thread will be closing it later upon its exit.
What if the parent exits first?
The parent can also send NX requests with this window and NX can
generate page faults. After kernel handles the page fault, send
signal to process by using PID if CSB address is invalid. Parent
thread will not receive signal since its PID is different from the one
saved in vas_window. So use tgid in case if the task for the pid saved
in window is not running and send signal to its parent.
To prevent reusing the pid until the window closed, take reference to
pid and task mm.
That text is all very dense. Can you please flesh it out and reword it
to clearly spell out what's going on in much more detail.
@@ -1266,8 +1300,17 @@ int vas_win_close(struct vas_window *window)poll_window_castout(window);/* if send window, drop reference to matching receive window */-if(window->tx_win)+if(window->tx_win){+if(window->user_win){+/* Drop references to pid and mm */+put_pid(window->pid);+if(window->mm){+mmdrop(window->mm);+mm_context_remove_copro(window->mm);
That seems backward. Once you drop the reference the mm can be freed
can't it?
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-03-17 05:30:10
Haren Myneni [off-list ref] writes:
quoted hunk
For each fault CRB, update fault address in CRB (fault_storage_addr)
and translation error status in CSB so that user space can touch the
fault address and resend the request. If the user space passed invalid
CSB address send signal to process with SIGSEGV.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
csb is on the stack, and later copied to user, which is a risk for
creating an infoleak.
Also please use reverse Christmas tree layout for your variables.
+
+ /*
+ * NX user space windows can not be opened for task->mm=NULL
+ * and faults will not be generated for kernel requests.
+ */
+ if (!window->mm || !window->user_win)
+ return;
If that's a should-never-happen condition then should it do a
WARN_ON_ONCE() rather than silently returning?
+ csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
+
+ csb.cc = CSB_CC_TRANSLATION;
+ csb.ce = CSB_CE_TERMINATION;
+ csb.cs = 0;
+ csb.count = 0;
+
+ /*
+ * NX operates and returns in BE format as defined CRB struct.
+ * So return fault_storage_addr in BE as NX pastes in FIFO and
+ * expects user space to convert to CPU format.
+ */
+ csb.address = crb->stamp.nx.fault_storage_addr;
+ csb.flags = 0;
I'm pretty sure this has initialised all the fields of csb.
But, I'd still be much happier if you zeroed the whole struct to begin
with, that way we know for sure we can't leak any uninitialised bytes to
userspace. It's only 16 bytes so it shouldn't add any noticeable
overhead.
+
+ pid = window->pid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Send window will be closed after processing all NX requests
+ * and process exits after closing all windows. In multi-thread
+ * applications, thread may not exists, but does not close FD
+ * (means send window) upon exit. Parent thread (tgid) can use
+ * and close the window later.
+ * pid and mm references are taken when window is opened by
+ * process (pid). So tgid is used only when child thread opens
+ * a window and exits without closing it in multithread tasks.
+ */
+ if (!tsk) {
+ pid = window->tgid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Parent thread will be closing window during its exit.
+ * So should not get here.
+ */
+ if (!tsk)
+ return;
Similar question on WARN_ON_ONCE()
+ }
+
+ /* Return if the task is exiting. */
Why? Just because it's no use? It's racy isn't it, so it can't be for
correctness?
There's no check that csb_addr is actually pointing into userspace, but
copy_to_user() does it for you.
+ rc = copy_to_user(csb_addr, &csb, sizeof(csb));
+ /*
+ * User space polls on csb.flags (first byte). So add barrier
+ * then copy first byte with csb flags update.
+ */
+ smp_mb();
You only need to order the stores above vs the store below to csb.flags.
So you should only need an smp_wmb() here.
+ if (!rc) {
+ csb.flags = CSB_V;
+ rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+ }
+ unuse_mm(window->mm);
+ put_task_struct(tsk);
+
+ /* Success */
+ if (!rc)
+ return;
+
+ pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
+ csb_addr, pid_vnr(pid));
+
+ clear_siginfo(&info);
+ info.si_signo = SIGSEGV;
+ info.si_errno = EFAULT;
+ info.si_code = SEGV_MAPERR;
+ info.si_addr = csb_addr;
+
+ /*
+ * process will be polling on csb.flags after request is sent to
+ * NX. So generally CSB update should not fail except when an
+ * application does not follow the process properly. So an error
+ * message will be displayed and leave it to user space whether
+ * to ignore or handle this signal.
+ */
+ rcu_read_lock();
+ rc = kill_pid_info(SIGSEGV, &info, pid);
+ rcu_read_unlock();
Shouldn't this be using force_sig_fault_to_task() or another helper,
rather than open-coding?
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-17 19:16:29
On Tue, 2020-03-17 at 15:09 +1100, Michael Ellerman wrote:
Haren Myneni [off-list ref] writes:
quoted
Process close windows after its requests are completed. In multi-thread
applications, child can open a window but release FD will not be called
upon its exit. Parent thread will be closing it later upon its exit.
What if the parent exits first?
Thanks for the review.
If the parent exists, child thread will close the window when it exits.
So we should not get the case where window is still open.
quoted
The parent can also send NX requests with this window and NX can
generate page faults. After kernel handles the page fault, send
signal to process by using PID if CSB address is invalid. Parent
thread will not receive signal since its PID is different from the one
saved in vas_window. So use tgid in case if the task for the pid saved
in window is not running and send signal to its parent.
To prevent reusing the pid until the window closed, take reference to
pid and task mm.
That text is all very dense. Can you please flesh it out and reword it
to clearly spell out what's going on in much more detail.
@@ -1266,8 +1300,17 @@ int vas_win_close(struct vas_window *window)poll_window_castout(window);/* if send window, drop reference to matching receive window */-if(window->tx_win)+if(window->tx_win){+if(window->user_win){+/* Drop references to pid and mm */+put_pid(window->pid);+if(window->mm){+mmdrop(window->mm);+mm_context_remove_copro(window->mm);
That seems backward. Once you drop the reference the mm can be freed
can't it?
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-17 19:30:34
On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
Haren Myneni [off-list ref] writes:
quoted
For each fault CRB, update fault address in CRB (fault_storage_addr)
and translation error status in CSB so that user space can touch the
fault address and resend the request. If the user space passed invalid
CSB address send signal to process with SIGSEGV.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
csb is on the stack, and later copied to user, which is a risk for
creating an infoleak.
Also please use reverse Christmas tree layout for your variables.
quoted
+
+ /*
+ * NX user space windows can not be opened for task->mm=NULL
+ * and faults will not be generated for kernel requests.
+ */
+ if (!window->mm || !window->user_win)
+ return;
If that's a should-never-happen condition then should it do a
WARN_ON_ONCE() rather than silently returning?
Will add WARN_ON
quoted
+ csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
+
+ csb.cc = CSB_CC_TRANSLATION;
+ csb.ce = CSB_CE_TERMINATION;
+ csb.cs = 0;
+ csb.count = 0;
+
+ /*
+ * NX operates and returns in BE format as defined CRB struct.
+ * So return fault_storage_addr in BE as NX pastes in FIFO and
+ * expects user space to convert to CPU format.
+ */
+ csb.address = crb->stamp.nx.fault_storage_addr;
+ csb.flags = 0;
I'm pretty sure this has initialised all the fields of csb.
But, I'd still be much happier if you zeroed the whole struct to begin
with, that way we know for sure we can't leak any uninitialised bytes to
userspace. It's only 16 bytes so it shouldn't add any noticeable
overhead.
Sure, will initialize csb
quoted
+
+ pid = window->pid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Send window will be closed after processing all NX requests
+ * and process exits after closing all windows. In multi-thread
+ * applications, thread may not exists, but does not close FD
+ * (means send window) upon exit. Parent thread (tgid) can use
+ * and close the window later.
+ * pid and mm references are taken when window is opened by
+ * process (pid). So tgid is used only when child thread opens
+ * a window and exits without closing it in multithread tasks.
+ */
+ if (!tsk) {
+ pid = window->tgid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Parent thread will be closing window during its exit.
+ * So should not get here.
+ */
+ if (!tsk)
+ return;
Similar question on WARN_ON_ONCE()
Yes, we can add WARN_ON
quoted
+ }
+
+ /* Return if the task is exiting. */
Why? Just because it's no use? It's racy isn't it, so it can't be for
correctness?
Yes process is exiting and no need to update CSB. We release the
task->usage refcount after copy_to_user().
There's no check that csb_addr is actually pointing into userspace, but
copy_to_user() does it for you.
quoted
+ rc = copy_to_user(csb_addr, &csb, sizeof(csb));
+ /*
+ * User space polls on csb.flags (first byte). So add barrier
+ * then copy first byte with csb flags update.
+ */
+ smp_mb();
You only need to order the stores above vs the store below to csb.flags.
So you should only need an smp_wmb() here.
Sure, will add
if (!rc) {
csb.flags = CSB_V;
smp_mb();
rc = copy_to_user(csb_addr, &csb, sizeof(u8));
}
quoted
+ if (!rc) {
+ csb.flags = CSB_V;
+ rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+ }
+ unuse_mm(window->mm);
+ put_task_struct(tsk);
+
+ /* Success */
+ if (!rc)
+ return;
+
+ pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
+ csb_addr, pid_vnr(pid));
+
+ clear_siginfo(&info);
+ info.si_signo = SIGSEGV;
+ info.si_errno = EFAULT;
+ info.si_code = SEGV_MAPERR;
+ info.si_addr = csb_addr;
+
+ /*
+ * process will be polling on csb.flags after request is sent to
+ * NX. So generally CSB update should not fail except when an
+ * application does not follow the process properly. So an error
+ * message will be displayed and leave it to user space whether
+ * to ignore or handle this signal.
+ */
+ rcu_read_lock();
+ rc = kill_pid_info(SIGSEGV, &info, pid);
+ rcu_read_unlock();
Shouldn't this be using force_sig_fault_to_task() or another helper,
rather than open-coding?
Applications or nxz library can ignore this signal based on si_addr or
take action like resend new request with valid csb_addr. Hence I did not
use force_sig_info_to_task().
From: Nicholas Piggin <npiggin@gmail.com> Date: 2020-03-23 00:13:43
Haren Myneni's on March 18, 2020 5:27 am:
On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
quoted
Haren Myneni [off-list ref] writes:
quoted
For each fault CRB, update fault address in CRB (fault_storage_addr)
and translation error status in CSB so that user space can touch the
fault address and resend the request. If the user space passed invalid
CSB address send signal to process with SIGSEGV.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
csb is on the stack, and later copied to user, which is a risk for
creating an infoleak.
Also please use reverse Christmas tree layout for your variables.
quoted
+
+ /*
+ * NX user space windows can not be opened for task->mm=NULL
+ * and faults will not be generated for kernel requests.
+ */
+ if (!window->mm || !window->user_win)
+ return;
If that's a should-never-happen condition then should it do a
WARN_ON_ONCE() rather than silently returning?
Will add WARN_ON
quoted
quoted
+ csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
+
+ csb.cc = CSB_CC_TRANSLATION;
+ csb.ce = CSB_CE_TERMINATION;
+ csb.cs = 0;
+ csb.count = 0;
+
+ /*
+ * NX operates and returns in BE format as defined CRB struct.
+ * So return fault_storage_addr in BE as NX pastes in FIFO and
+ * expects user space to convert to CPU format.
+ */
+ csb.address = crb->stamp.nx.fault_storage_addr;
+ csb.flags = 0;
I'm pretty sure this has initialised all the fields of csb.
But, I'd still be much happier if you zeroed the whole struct to begin
with, that way we know for sure we can't leak any uninitialised bytes to
userspace. It's only 16 bytes so it shouldn't add any noticeable
overhead.
Sure, will initialize csb
quoted
quoted
+
+ pid = window->pid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Send window will be closed after processing all NX requests
+ * and process exits after closing all windows. In multi-thread
+ * applications, thread may not exists, but does not close FD
+ * (means send window) upon exit. Parent thread (tgid) can use
+ * and close the window later.
+ * pid and mm references are taken when window is opened by
+ * process (pid). So tgid is used only when child thread opens
+ * a window and exits without closing it in multithread tasks.
+ */
+ if (!tsk) {
+ pid = window->tgid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Parent thread will be closing window during its exit.
+ * So should not get here.
+ */
+ if (!tsk)
+ return;
Similar question on WARN_ON_ONCE()
Yes, we can add WARN_ON
quoted
quoted
+ }
+
+ /* Return if the task is exiting. */
Why? Just because it's no use? It's racy isn't it, so it can't be for
correctness?
Yes process is exiting and no need to update CSB. We release the
task->usage refcount after copy_to_user().
There's no check that csb_addr is actually pointing into userspace, but
copy_to_user() does it for you.
quoted
+ rc = copy_to_user(csb_addr, &csb, sizeof(csb));
+ /*
+ * User space polls on csb.flags (first byte). So add barrier
+ * then copy first byte with csb flags update.
+ */
+ smp_mb();
You only need to order the stores above vs the store below to csb.flags.
So you should only need an smp_wmb() here.
Sure, will add
if (!rc) {
csb.flags = CSB_V;
smp_mb();
rc = copy_to_user(csb_addr, &csb, sizeof(u8));
}
quoted
quoted
+ if (!rc) {
+ csb.flags = CSB_V;
+ rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+ }
+ unuse_mm(window->mm);
+ put_task_struct(tsk);
+
+ /* Success */
+ if (!rc)
+ return;
+
+ pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
+ csb_addr, pid_vnr(pid));
+
+ clear_siginfo(&info);
+ info.si_signo = SIGSEGV;
+ info.si_errno = EFAULT;
+ info.si_code = SEGV_MAPERR;
+ info.si_addr = csb_addr;
+
+ /*
+ * process will be polling on csb.flags after request is sent to
+ * NX. So generally CSB update should not fail except when an
+ * application does not follow the process properly. So an error
+ * message will be displayed and leave it to user space whether
+ * to ignore or handle this signal.
+ */
The code would read a bit better if this comment goes at the start of
this error handling process it describes (before the error message).
And I feel a bit hypocritical complaining about readability, but it
could possibly do with some work.
/*
* The application should have provided a valid mapping for the
* csb, and not unmap it before the csb.flags update, so the
* copy_to_user should not fail.
*
* If the application fails to follow this protocol, log a kernel
* error and send a SEGV to the pid. This signal may be ignored,
* so can't use force_sig_fault_to_task()
*/
Something like tthat?
ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
it's held around kill_pid_info in kernel/signal.c for the find_vpid().
Thanks,
Nick
quoted
Shouldn't this be using force_sig_fault_to_task() or another helper,
rather than open-coding?
Applications or nxz library can ignore this signal based on si_addr or
take action like resend new request with valid csb_addr. Hence I did not
use force_sig_info_to_task().
From: Haren Myneni <haren@linux.ibm.com> Date: 2020-03-23 01:09:30
On Mon, 2020-03-23 at 10:06 +1000, Nicholas Piggin wrote:
Haren Myneni's on March 18, 2020 5:27 am:
quoted
On Tue, 2020-03-17 at 16:28 +1100, Michael Ellerman wrote:
quoted
Haren Myneni [off-list ref] writes:
quoted
For each fault CRB, update fault address in CRB (fault_storage_addr)
and translation error status in CSB so that user space can touch the
fault address and resend the request. If the user space passed invalid
CSB address send signal to process with SIGSEGV.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-fault.c | 114 +++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
csb is on the stack, and later copied to user, which is a risk for
creating an infoleak.
Also please use reverse Christmas tree layout for your variables.
quoted
+
+ /*
+ * NX user space windows can not be opened for task->mm=NULL
+ * and faults will not be generated for kernel requests.
+ */
+ if (!window->mm || !window->user_win)
+ return;
If that's a should-never-happen condition then should it do a
WARN_ON_ONCE() rather than silently returning?
Will add WARN_ON
quoted
quoted
+ csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
+
+ csb.cc = CSB_CC_TRANSLATION;
+ csb.ce = CSB_CE_TERMINATION;
+ csb.cs = 0;
+ csb.count = 0;
+
+ /*
+ * NX operates and returns in BE format as defined CRB struct.
+ * So return fault_storage_addr in BE as NX pastes in FIFO and
+ * expects user space to convert to CPU format.
+ */
+ csb.address = crb->stamp.nx.fault_storage_addr;
+ csb.flags = 0;
I'm pretty sure this has initialised all the fields of csb.
But, I'd still be much happier if you zeroed the whole struct to begin
with, that way we know for sure we can't leak any uninitialised bytes to
userspace. It's only 16 bytes so it shouldn't add any noticeable
overhead.
Sure, will initialize csb
quoted
quoted
+
+ pid = window->pid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Send window will be closed after processing all NX requests
+ * and process exits after closing all windows. In multi-thread
+ * applications, thread may not exists, but does not close FD
+ * (means send window) upon exit. Parent thread (tgid) can use
+ * and close the window later.
+ * pid and mm references are taken when window is opened by
+ * process (pid). So tgid is used only when child thread opens
+ * a window and exits without closing it in multithread tasks.
+ */
+ if (!tsk) {
+ pid = window->tgid;
+ tsk = get_pid_task(pid, PIDTYPE_PID);
+ /*
+ * Parent thread will be closing window during its exit.
+ * So should not get here.
+ */
+ if (!tsk)
+ return;
Similar question on WARN_ON_ONCE()
Yes, we can add WARN_ON
quoted
quoted
+ }
+
+ /* Return if the task is exiting. */
Why? Just because it's no use? It's racy isn't it, so it can't be for
correctness?
Yes process is exiting and no need to update CSB. We release the
task->usage refcount after copy_to_user().
There's no check that csb_addr is actually pointing into userspace, but
copy_to_user() does it for you.
quoted
+ rc = copy_to_user(csb_addr, &csb, sizeof(csb));
+ /*
+ * User space polls on csb.flags (first byte). So add barrier
+ * then copy first byte with csb flags update.
+ */
+ smp_mb();
You only need to order the stores above vs the store below to csb.flags.
So you should only need an smp_wmb() here.
Sure, will add
if (!rc) {
csb.flags = CSB_V;
smp_mb();
rc = copy_to_user(csb_addr, &csb, sizeof(u8));
}
quoted
quoted
+ if (!rc) {
+ csb.flags = CSB_V;
+ rc = copy_to_user(csb_addr, &csb, sizeof(u8));
+ }
+ unuse_mm(window->mm);
+ put_task_struct(tsk);
+
+ /* Success */
+ if (!rc)
+ return;
+
+ pr_debug("Invalid CSB address 0x%p signalling pid(%d)\n",
+ csb_addr, pid_vnr(pid));
+
+ clear_siginfo(&info);
+ info.si_signo = SIGSEGV;
+ info.si_errno = EFAULT;
+ info.si_code = SEGV_MAPERR;
+ info.si_addr = csb_addr;
+
+ /*
+ * process will be polling on csb.flags after request is sent to
+ * NX. So generally CSB update should not fail except when an
+ * application does not follow the process properly. So an error
+ * message will be displayed and leave it to user space whether
+ * to ignore or handle this signal.
+ */
The code would read a bit better if this comment goes at the start of
this error handling process it describes (before the error message).
And I feel a bit hypocritical complaining about readability, but it
could possibly do with some work.
/*
* The application should have provided a valid mapping for the
* csb, and not unmap it before the csb.flags update, so the
* copy_to_user should not fail.
*
* If the application fails to follow this protocol, log a kernel
* error and send a SEGV to the pid. This signal may be ignored,
* so can't use force_sig_fault_to_task()
*/
Something like tthat?
ipc/mqueue.c says kill_pid_info doesn't need rcu_read_lock(). AFAIKS
it's held around kill_pid_info in kernel/signal.c for the find_vpid().
I was following as in kill_proc_info(). Will remove rcu_read_lock().
Thanks,
Nick
quoted
quoted
Shouldn't this be using force_sig_fault_to_task() or another helper,
rather than open-coding?
Applications or nxz library can ignore this signal based on si_addr or
take action like resend new request with valid csb_addr. Hence I did not
use force_sig_info_to_task().