[SRU] ][Bionic][PATCH 1/1] ocxl: Fix page fault handler in case of fault on dying process

Message ID dfba75844f55a029f22af5a9885e0c507eb8480b.1532967825.git.joseph.salisbury@canonical.com
State New
Headers show
Series
  • [SRU] ][Bionic][PATCH 1/1] ocxl: Fix page fault handler in case of fault on dying process
Related show

Commit Message

Joseph Salisbury July 30, 2018, 4:31 p.m.
From: Frederic Barrat <fbarrat@linux.ibm.com>

BugLink: http://bugs.launchpad.net/bugs/1781436

If a process exits without doing proper cleanup, there's a window
where an opencapi device can try to access the memory of the dying
process and may trigger a page fault. That's an expected scenario and
the ocxl driver holds a reference on the mm_struct of the process
until the opencapi device is notified of the process exiting.
However, if mm_users is already at 0, i.e. the address space of the
process has already been destroyed, the driver shouldn't try resolving
the page fault, as it will fail, but it can also try accessing already
freed data.

It is fixed by only calling the bottom half of the page fault handler
if mm_users is greater than 0 and get a reference on mm_users instead
of mm_count. Otherwise, we can safely return a translation fault to
the device, as its associated memory context is being removed. The
opencapi device will be properly cleaned up shortly after when closing
the file descriptors.

Fixes: 5ef3166e8a32 ("ocxl: Driver code for 'generic' opencapi devices")
Cc: stable@vger.kernel.org # v4.16+
Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-By: Alastair D'Silva <alastair@d-silva.org>
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
(cherry picked from linux-next commit d497ebf5fb3a026c0817f8c96cde578787f24093)
Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>
---
 drivers/misc/ocxl/link.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Stefan Bader July 31, 2018, 10:21 a.m. | #1
On 30.07.2018 18:31, Joseph Salisbury wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1781436

Not ultimately important but if we touch the commit msg anyway we should try to
move to use https:// whenever possible.
> 
> If a process exits without doing proper cleanup, there's a window
> where an opencapi device can try to access the memory of the dying
> process and may trigger a page fault. That's an expected scenario and
> the ocxl driver holds a reference on the mm_struct of the process
> until the opencapi device is notified of the process exiting.
> However, if mm_users is already at 0, i.e. the address space of the
> process has already been destroyed, the driver shouldn't try resolving
> the page fault, as it will fail, but it can also try accessing already
> freed data.
> 
> It is fixed by only calling the bottom half of the page fault handler
> if mm_users is greater than 0 and get a reference on mm_users instead
> of mm_count. Otherwise, we can safely return a translation fault to
> the device, as its associated memory context is being removed. The
> opencapi device will be properly cleaned up shortly after when closing
> the file descriptors.
> 
> Fixes: 5ef3166e8a32 ("ocxl: Driver code for 'generic' opencapi devices")
> Cc: stable@vger.kernel.org # v4.16+
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-By: Alastair D'Silva <alastair@d-silva.org>
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> (cherry picked from linux-next commit d497ebf5fb3a026c0817f8c96cde578787f24093)
(cherry picked from commit d497ebf5fb3a026c0817f8c96cde578787f24093 linux-next)
> Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Cherry pick and should follow "... from commit <sha1> [<reference>]" format.

(and the subject contains unbalanced [] ;) )

>  drivers/misc/ocxl/link.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index 88876ae8f330..a963b0a4a3c5 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -136,7 +136,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work)
>  	int rc;
>  
>  	/*
> -	 * We need to release a reference on the mm whenever exiting this
> +	 * We must release a reference on mm_users whenever exiting this
>  	 * function (taken in the memory fault interrupt handler)
>  	 */
>  	rc = copro_handle_mm_fault(fault->pe_data.mm, fault->dar, fault->dsisr,
> @@ -172,7 +172,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work)
>  	}
>  	r = RESTART;
>  ack:
> -	mmdrop(fault->pe_data.mm);
> +	mmput(fault->pe_data.mm);
>  	ack_irq(spa, r);
>  }
>  
> @@ -184,6 +184,7 @@ static irqreturn_t xsl_fault_handler(int irq, void *data)
>  	struct pe_data *pe_data;
>  	struct ocxl_process_element *pe;
>  	int lpid, pid, tid;
> +	bool schedule = false;
>  
>  	read_irq(spa, &dsisr, &dar, &pe_handle);
>  	trace_ocxl_fault(spa->spa_mem, pe_handle, dsisr, dar, -1);
> @@ -226,14 +227,19 @@ static irqreturn_t xsl_fault_handler(int irq, void *data)
>  	}
>  	WARN_ON(pe_data->mm->context.id != pid);
>  
> -	spa->xsl_fault.pe = pe_handle;
> -	spa->xsl_fault.dar = dar;
> -	spa->xsl_fault.dsisr = dsisr;
> -	spa->xsl_fault.pe_data = *pe_data;
> -	mmgrab(pe_data->mm); /* mm count is released by bottom half */
> -
> +	if (mmget_not_zero(pe_data->mm)) {
> +			spa->xsl_fault.pe = pe_handle;
> +			spa->xsl_fault.dar = dar;
> +			spa->xsl_fault.dsisr = dsisr;
> +			spa->xsl_fault.pe_data = *pe_data;
> +			schedule = true;
> +			/* mm_users count released by bottom half */
> +	}
>  	rcu_read_unlock();
> -	schedule_work(&spa->xsl_fault.fault_work);
> +	if (schedule)
> +		schedule_work(&spa->xsl_fault.fault_work);
> +	else
> +		ack_irq(spa, ADDRESS_ERROR);
>  	return IRQ_HANDLED;
>  }
>  
>
Kleber Souza July 31, 2018, 10:49 a.m. | #2
On 07/30/18 18:31, Joseph Salisbury wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1781436
> 
> If a process exits without doing proper cleanup, there's a window
> where an opencapi device can try to access the memory of the dying
> process and may trigger a page fault. That's an expected scenario and
> the ocxl driver holds a reference on the mm_struct of the process
> until the opencapi device is notified of the process exiting.
> However, if mm_users is already at 0, i.e. the address space of the
> process has already been destroyed, the driver shouldn't try resolving
> the page fault, as it will fail, but it can also try accessing already
> freed data.
> 
> It is fixed by only calling the bottom half of the page fault handler
> if mm_users is greater than 0 and get a reference on mm_users instead
> of mm_count. Otherwise, we can safely return a translation fault to
> the device, as its associated memory context is being removed. The
> opencapi device will be properly cleaned up shortly after when closing
> the file descriptors.
> 
> Fixes: 5ef3166e8a32 ("ocxl: Driver code for 'generic' opencapi devices")
> Cc: stable@vger.kernel.org # v4.16+
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-By: Alastair D'Silva <alastair@d-silva.org>
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> (cherry picked from linux-next commit d497ebf5fb3a026c0817f8c96cde578787f24093)
> Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>

Given the "cherry picked" line is fixed as Stefan mentioned:

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  drivers/misc/ocxl/link.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index 88876ae8f330..a963b0a4a3c5 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -136,7 +136,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work)
>  	int rc;
>  
>  	/*
> -	 * We need to release a reference on the mm whenever exiting this
> +	 * We must release a reference on mm_users whenever exiting this
>  	 * function (taken in the memory fault interrupt handler)
>  	 */
>  	rc = copro_handle_mm_fault(fault->pe_data.mm, fault->dar, fault->dsisr,
> @@ -172,7 +172,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work)
>  	}
>  	r = RESTART;
>  ack:
> -	mmdrop(fault->pe_data.mm);
> +	mmput(fault->pe_data.mm);
>  	ack_irq(spa, r);
>  }
>  
> @@ -184,6 +184,7 @@ static irqreturn_t xsl_fault_handler(int irq, void *data)
>  	struct pe_data *pe_data;
>  	struct ocxl_process_element *pe;
>  	int lpid, pid, tid;
> +	bool schedule = false;
>  
>  	read_irq(spa, &dsisr, &dar, &pe_handle);
>  	trace_ocxl_fault(spa->spa_mem, pe_handle, dsisr, dar, -1);
> @@ -226,14 +227,19 @@ static irqreturn_t xsl_fault_handler(int irq, void *data)
>  	}
>  	WARN_ON(pe_data->mm->context.id != pid);
>  
> -	spa->xsl_fault.pe = pe_handle;
> -	spa->xsl_fault.dar = dar;
> -	spa->xsl_fault.dsisr = dsisr;
> -	spa->xsl_fault.pe_data = *pe_data;
> -	mmgrab(pe_data->mm); /* mm count is released by bottom half */
> -
> +	if (mmget_not_zero(pe_data->mm)) {
> +			spa->xsl_fault.pe = pe_handle;
> +			spa->xsl_fault.dar = dar;
> +			spa->xsl_fault.dsisr = dsisr;
> +			spa->xsl_fault.pe_data = *pe_data;
> +			schedule = true;
> +			/* mm_users count released by bottom half */
> +	}
>  	rcu_read_unlock();
> -	schedule_work(&spa->xsl_fault.fault_work);
> +	if (schedule)
> +		schedule_work(&spa->xsl_fault.fault_work);
> +	else
> +		ack_irq(spa, ADDRESS_ERROR);
>  	return IRQ_HANDLED;
>  }
>  
>
Kleber Souza July 31, 2018, 1:12 p.m. | #3
On 07/30/18 18:31, Joseph Salisbury wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1781436
> 
> If a process exits without doing proper cleanup, there's a window
> where an opencapi device can try to access the memory of the dying
> process and may trigger a page fault. That's an expected scenario and
> the ocxl driver holds a reference on the mm_struct of the process
> until the opencapi device is notified of the process exiting.
> However, if mm_users is already at 0, i.e. the address space of the
> process has already been destroyed, the driver shouldn't try resolving
> the page fault, as it will fail, but it can also try accessing already
> freed data.
> 
> It is fixed by only calling the bottom half of the page fault handler
> if mm_users is greater than 0 and get a reference on mm_users instead
> of mm_count. Otherwise, we can safely return a translation fault to
> the device, as its associated memory context is being removed. The
> opencapi device will be properly cleaned up shortly after when closing
> the file descriptors.
> 
> Fixes: 5ef3166e8a32 ("ocxl: Driver code for 'generic' opencapi devices")
> Cc: stable@vger.kernel.org # v4.16+
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-By: Alastair D'Silva <alastair@d-silva.org>
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> (cherry picked from linux-next commit d497ebf5fb3a026c0817f8c96cde578787f24093)
> Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> ---
>  drivers/misc/ocxl/link.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index 88876ae8f330..a963b0a4a3c5 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -136,7 +136,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work)
>  	int rc;
>  
>  	/*
> -	 * We need to release a reference on the mm whenever exiting this
> +	 * We must release a reference on mm_users whenever exiting this
>  	 * function (taken in the memory fault interrupt handler)
>  	 */
>  	rc = copro_handle_mm_fault(fault->pe_data.mm, fault->dar, fault->dsisr,
> @@ -172,7 +172,7 @@ static void xsl_fault_handler_bh(struct work_struct *fault_work)
>  	}
>  	r = RESTART;
>  ack:
> -	mmdrop(fault->pe_data.mm);
> +	mmput(fault->pe_data.mm);
>  	ack_irq(spa, r);
>  }
>  
> @@ -184,6 +184,7 @@ static irqreturn_t xsl_fault_handler(int irq, void *data)
>  	struct pe_data *pe_data;
>  	struct ocxl_process_element *pe;
>  	int lpid, pid, tid;
> +	bool schedule = false;
>  
>  	read_irq(spa, &dsisr, &dar, &pe_handle);
>  	trace_ocxl_fault(spa->spa_mem, pe_handle, dsisr, dar, -1);
> @@ -226,14 +227,19 @@ static irqreturn_t xsl_fault_handler(int irq, void *data)
>  	}
>  	WARN_ON(pe_data->mm->context.id != pid);
>  
> -	spa->xsl_fault.pe = pe_handle;
> -	spa->xsl_fault.dar = dar;
> -	spa->xsl_fault.dsisr = dsisr;
> -	spa->xsl_fault.pe_data = *pe_data;
> -	mmgrab(pe_data->mm); /* mm count is released by bottom half */
> -
> +	if (mmget_not_zero(pe_data->mm)) {
> +			spa->xsl_fault.pe = pe_handle;
> +			spa->xsl_fault.dar = dar;
> +			spa->xsl_fault.dsisr = dsisr;
> +			spa->xsl_fault.pe_data = *pe_data;
> +			schedule = true;
> +			/* mm_users count released by bottom half */
> +	}
>  	rcu_read_unlock();
> -	schedule_work(&spa->xsl_fault.fault_work);
> +	if (schedule)
> +		schedule_work(&spa->xsl_fault.fault_work);
> +	else
> +		ack_irq(spa, ADDRESS_ERROR);
>  	return IRQ_HANDLED;
>  }
>  
> 

Applied to bionic/master-next branch, with the BugLink and "cherry
picked from ..." fixes suggested by Stefan.

Thanks,
Kleber
Seth Forshee Aug. 1, 2018, 1:25 p.m. | #4
On Mon, Jul 30, 2018 at 12:31:03PM -0400, Joseph Salisbury wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1781436
> 
> If a process exits without doing proper cleanup, there's a window
> where an opencapi device can try to access the memory of the dying
> process and may trigger a page fault. That's an expected scenario and
> the ocxl driver holds a reference on the mm_struct of the process
> until the opencapi device is notified of the process exiting.
> However, if mm_users is already at 0, i.e. the address space of the
> process has already been destroyed, the driver shouldn't try resolving
> the page fault, as it will fail, but it can also try accessing already
> freed data.
> 
> It is fixed by only calling the bottom half of the page fault handler
> if mm_users is greater than 0 and get a reference on mm_users instead
> of mm_count. Otherwise, we can safely return a translation fault to
> the device, as its associated memory context is being removed. The
> opencapi device will be properly cleaned up shortly after when closing
> the file descriptors.
> 
> Fixes: 5ef3166e8a32 ("ocxl: Driver code for 'generic' opencapi devices")
> Cc: stable@vger.kernel.org # v4.16+
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-By: Alastair D'Silva <alastair@d-silva.org>
> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> (cherry picked from linux-next commit d497ebf5fb3a026c0817f8c96cde578787f24093)
> Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>

Applied to cosmic/master-next and unstable/master, thanks!

Patch

diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
index 88876ae8f330..a963b0a4a3c5 100644
--- a/drivers/misc/ocxl/link.c
+++ b/drivers/misc/ocxl/link.c
@@ -136,7 +136,7 @@  static void xsl_fault_handler_bh(struct work_struct *fault_work)
 	int rc;
 
 	/*
-	 * We need to release a reference on the mm whenever exiting this
+	 * We must release a reference on mm_users whenever exiting this
 	 * function (taken in the memory fault interrupt handler)
 	 */
 	rc = copro_handle_mm_fault(fault->pe_data.mm, fault->dar, fault->dsisr,
@@ -172,7 +172,7 @@  static void xsl_fault_handler_bh(struct work_struct *fault_work)
 	}
 	r = RESTART;
 ack:
-	mmdrop(fault->pe_data.mm);
+	mmput(fault->pe_data.mm);
 	ack_irq(spa, r);
 }
 
@@ -184,6 +184,7 @@  static irqreturn_t xsl_fault_handler(int irq, void *data)
 	struct pe_data *pe_data;
 	struct ocxl_process_element *pe;
 	int lpid, pid, tid;
+	bool schedule = false;
 
 	read_irq(spa, &dsisr, &dar, &pe_handle);
 	trace_ocxl_fault(spa->spa_mem, pe_handle, dsisr, dar, -1);
@@ -226,14 +227,19 @@  static irqreturn_t xsl_fault_handler(int irq, void *data)
 	}
 	WARN_ON(pe_data->mm->context.id != pid);
 
-	spa->xsl_fault.pe = pe_handle;
-	spa->xsl_fault.dar = dar;
-	spa->xsl_fault.dsisr = dsisr;
-	spa->xsl_fault.pe_data = *pe_data;
-	mmgrab(pe_data->mm); /* mm count is released by bottom half */
-
+	if (mmget_not_zero(pe_data->mm)) {
+			spa->xsl_fault.pe = pe_handle;
+			spa->xsl_fault.dar = dar;
+			spa->xsl_fault.dsisr = dsisr;
+			spa->xsl_fault.pe_data = *pe_data;
+			schedule = true;
+			/* mm_users count released by bottom half */
+	}
 	rcu_read_unlock();
-	schedule_work(&spa->xsl_fault.fault_work);
+	if (schedule)
+		schedule_work(&spa->xsl_fault.fault_work);
+	else
+		ack_irq(spa, ADDRESS_ERROR);
 	return IRQ_HANDLED;
 }