[PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

Subsystems: char and misc drivers, the rest

STALE3066d

6 messages, 3 authors, 2018-03-15 · open the first message on its own page

[PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

From: Vaibhav Jain <hidden>
Date: 2018-03-08 10:05:59

It is possible for a CXL card to have a valid PSL but no valid
AFUs. When this happens we have a valid instance of 'struct cxl'
representing the adapter but with its member 'struct cxl_afu *cxl[]'
as empty. Unfortunately at many placed within cxl code (especially
during an EEH) the elements of this array are passed on to various
other cxl functions. Which may result in kernel oops/panic when this
'struct cxl_afu *' is dereferenced.

So this patch puts a NULL check at the beginning of various cxl
functions that accept 'struct cxl_afu *' as a formal argument and are
called from with a loop of the form:

       for (i = 0; i < adapter->slices; i++) {
       	   	afu = adapter->afu[i];
		/* call some function with 'afu' */
	}

Signed-off-by: Vaibhav Jain <redacted>
---
 drivers/misc/cxl/api.c     |  2 +-
 drivers/misc/cxl/context.c |  3 +++
 drivers/misc/cxl/guest.c   |  4 ++++
 drivers/misc/cxl/main.c    |  3 +++
 drivers/misc/cxl/native.c  |  4 ++++
 drivers/misc/cxl/pci.c     | 13 ++++++++++++-
 6 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 753b1a698fc4..3466ef8b9e86 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c

@@ -128,7 +128,7 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev *dev)
 	int rc;
 
 	afu = cxl_pci_to_afu(dev);
-	if (IS_ERR(afu))
+	if (IS_ERR_OR_NULL(afu))
 		return ERR_CAST(afu);
 
 	ctx = cxl_context_alloc();

diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c
index 7ff315ad3692..3957e6e7d187 100644
--- a/drivers/misc/cxl/context.c
+++ b/drivers/misc/cxl/context.c

@@ -303,6 +303,9 @@ void cxl_context_detach_all(struct cxl_afu *afu)
 	struct cxl_context *ctx;
 	int tmp;
 
+	if (afu == NULL)
+		return;
+
 	mutex_lock(&afu->contexts_lock);
 	idr_for_each_entry(&afu->contexts_idr, ctx, tmp) {
 		/*

diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index f58b4b6c79f2..8165f6f26704 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c

@@ -760,6 +760,8 @@ static int activate_afu_directed(struct cxl_afu *afu)
 
 static int guest_afu_activate_mode(struct cxl_afu *afu, int mode)
 {
+	if (afu == NULL)
+		return -EINVAL;
 	if (!mode)
 		return 0;
 	if (!(mode & afu->modes_supported))

@@ -791,6 +793,8 @@ static int deactivate_afu_directed(struct cxl_afu *afu)
 
 static int guest_afu_deactivate_mode(struct cxl_afu *afu, int mode)
 {
+	if (afu == NULL)
+		return -EINVAL;
 	if (!mode)
 		return 0;
 	if (!(mode & afu->modes_supported))

diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c
index c1ba0d42cbc8..296a71ca6f2e 100644
--- a/drivers/misc/cxl/main.c
+++ b/drivers/misc/cxl/main.c

@@ -271,6 +271,9 @@ struct cxl_afu *cxl_alloc_afu(struct cxl *adapter, int slice)
 
 int cxl_afu_select_best_mode(struct cxl_afu *afu)
 {
+	if (afu == NULL)
+		return -EINVAL;
+
 	if (afu->modes_supported & CXL_MODE_DIRECTED)
 		return cxl_ops->afu_activate_mode(afu, CXL_MODE_DIRECTED);

diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 1b3d7c65ea3f..d46415b19b71 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c

@@ -971,6 +971,8 @@ static int deactivate_dedicated_process(struct cxl_afu *afu)
 
 static int native_afu_deactivate_mode(struct cxl_afu *afu, int mode)
 {
+	if (afu == NULL)
+		return -EINVAL;
 	if (mode == CXL_MODE_DIRECTED)
 		return deactivate_afu_directed(afu);
 	if (mode == CXL_MODE_DEDICATED)

@@ -980,6 +982,8 @@ static int native_afu_deactivate_mode(struct cxl_afu *afu, int mode)
 
 static int native_afu_activate_mode(struct cxl_afu *afu, int mode)
 {
+	if (!afu)
+		return -EINVAL;
 	if (!mode)
 		return 0;
 	if (!(mode & afu->modes_supported))

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 758842f65a1b..8c87d9fdcf5a 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -1295,6 +1295,9 @@ static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pc
 {
 	int rc;
 
+	if (afu == NULL)
+		return -EINVAL;
+
 	if ((rc = pci_map_slice_regs(afu, adapter, dev)))
 		return rc;

@@ -1341,6 +1344,10 @@ static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pc
 
 static void pci_deconfigure_afu(struct cxl_afu *afu)
 {
+
+	if (afu == NULL)
+		return;
+
 	/*
 	 * It's okay to deconfigure when AFU is already locked, otherwise wait
 	 * until there are no readers

@@ -2078,6 +2085,10 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
 	pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
 	pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;
 
+	/* Force a hotplug if an uninitiaized AFU is encountered */
+	if (afu == NULL)
+		return PCI_ERS_RESULT_DISCONNECT;
+
 	/* There should only be one entry, but go through the list
 	 * anyway
 	 */

@@ -2330,7 +2341,7 @@ static void cxl_pci_resume(struct pci_dev *pdev)
 	for (i = 0; i < adapter->slices; i++) {
 		afu = adapter->afu[i];
 
-		if (afu->phb == NULL)
+		if (afu == NULL || afu->phb == NULL)
 			continue;
 
 		list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {

-- 
2.14.3

Re: [PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

From: Andrew Donnellan <hidden>
Date: 2018-03-09 00:26:01

On 08/03/18 21:05, Vaibhav Jain wrote:

It is possible for a CXL card to have a valid PSL but no valid
AFUs. When this happens we have a valid instance of 'struct cxl'
representing the adapter but with its member 'struct cxl_afu *cxl[]'
as empty. Unfortunately at many placed within cxl code (especially
during an EEH) the elements of this array are passed on to various
other cxl functions. Which may result in kernel oops/panic when this
'struct cxl_afu *' is dereferenced.

So this patch puts a NULL check at the beginning of various cxl
functions that accept 'struct cxl_afu *' as a formal argument and are
called from with a loop of the form:

        for (i = 0; i < adapter->slices; i++) {
        	   	afu = adapter->afu[i];
		/* call some function with 'afu' */
	}

Surely in this case adapter->slices should be 0?

We might still need to harden for other cases...

quoted hunk

Signed-off-by: Vaibhav Jain <redacted>
---
  drivers/misc/cxl/api.c     |  2 +-
  drivers/misc/cxl/context.c |  3 +++
  drivers/misc/cxl/guest.c   |  4 ++++
  drivers/misc/cxl/main.c    |  3 +++
  drivers/misc/cxl/native.c  |  4 ++++
  drivers/misc/cxl/pci.c     | 13 ++++++++++++-
  6 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 753b1a698fc4..3466ef8b9e86 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c

@@ -128,7 +128,7 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev *dev)
  	int rc;

  	afu = cxl_pci_to_afu(dev);
-	if (IS_ERR(afu))
+	if (IS_ERR_OR_NULL(afu))
  		return ERR_CAST(afu);

  	ctx = cxl_context_alloc();

diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c
index 7ff315ad3692..3957e6e7d187 100644
--- a/drivers/misc/cxl/context.c
+++ b/drivers/misc/cxl/context.c

@@ -303,6 +303,9 @@ void cxl_context_detach_all(struct cxl_afu *afu)
  	struct cxl_context *ctx;
  	int tmp;

+	if (afu == NULL)
+		return;
+
  	mutex_lock(&afu->contexts_lock);
  	idr_for_each_entry(&afu->contexts_idr, ctx, tmp) {
  		/*

diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index f58b4b6c79f2..8165f6f26704 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c

@@ -760,6 +760,8 @@ static int activate_afu_directed(struct cxl_afu *afu)

  static int guest_afu_activate_mode(struct cxl_afu *afu, int mode)
  {
+	if (afu == NULL)
+		return -EINVAL;
  	if (!mode)
  		return 0;
  	if (!(mode & afu->modes_supported))

@@ -791,6 +793,8 @@ static int deactivate_afu_directed(struct cxl_afu *afu)

  static int guest_afu_deactivate_mode(struct cxl_afu *afu, int mode)
  {
+	if (afu == NULL)
+		return -EINVAL;
  	if (!mode)
  		return 0;
  	if (!(mode & afu->modes_supported))

diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c
index c1ba0d42cbc8..296a71ca6f2e 100644
--- a/drivers/misc/cxl/main.c
+++ b/drivers/misc/cxl/main.c

@@ -271,6 +271,9 @@ struct cxl_afu *cxl_alloc_afu(struct cxl *adapter, int slice)

  int cxl_afu_select_best_mode(struct cxl_afu *afu)
  {
+	if (afu == NULL)
+		return -EINVAL;
+
  	if (afu->modes_supported & CXL_MODE_DIRECTED)
  		return cxl_ops->afu_activate_mode(afu, CXL_MODE_DIRECTED);

diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 1b3d7c65ea3f..d46415b19b71 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c

@@ -971,6 +971,8 @@ static int deactivate_dedicated_process(struct cxl_afu *afu)

  static int native_afu_deactivate_mode(struct cxl_afu *afu, int mode)
  {
+	if (afu == NULL)
+		return -EINVAL;
  	if (mode == CXL_MODE_DIRECTED)
  		return deactivate_afu_directed(afu);
  	if (mode == CXL_MODE_DEDICATED)

@@ -980,6 +982,8 @@ static int native_afu_deactivate_mode(struct cxl_afu *afu, int mode)

  static int native_afu_activate_mode(struct cxl_afu *afu, int mode)
  {
+	if (!afu)
+		return -EINVAL;
  	if (!mode)
  		return 0;
  	if (!(mode & afu->modes_supported))

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 758842f65a1b..8c87d9fdcf5a 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -1295,6 +1295,9 @@ static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pc
  {
  	int rc;

+	if (afu == NULL)
+		return -EINVAL;
+
  	if ((rc = pci_map_slice_regs(afu, adapter, dev)))
  		return rc;

@@ -1341,6 +1344,10 @@ static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pc

  static void pci_deconfigure_afu(struct cxl_afu *afu)
  {
+
+	if (afu == NULL)
+		return;
+
  	/*
  	 * It's okay to deconfigure when AFU is already locked, otherwise wait
  	 * until there are no readers

@@ -2078,6 +2085,10 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
  	pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
  	pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;

+	/* Force a hotplug if an uninitiaized AFU is encountered */
+	if (afu == NULL)
+		return PCI_ERS_RESULT_DISCONNECT;
+
  	/* There should only be one entry, but go through the list
  	 * anyway
  	 */

@@ -2330,7 +2341,7 @@ static void cxl_pci_resume(struct pci_dev *pdev)
  	for (i = 0; i < adapter->slices; i++) {
  		afu = adapter->afu[i];

-		if (afu->phb == NULL)
+		if (afu == NULL || afu->phb == NULL)
  			continue;

  		list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

From: Vaibhav Jain <hidden>
Date: 2018-03-09 03:00:16

Thanks for looking into this patch Andrew,

Andrew Donnellan [off-list ref] writes:

On 08/03/18 21:05, Vaibhav Jain wrote:

quoted

It is possible for a CXL card to have a valid PSL but no valid
AFUs. When this happens we have a valid instance of 'struct cxl'
representing the adapter but with its member 'struct cxl_afu *cxl[]'
as empty. Unfortunately at many placed within cxl code (especially
during an EEH) the elements of this array are passed on to various
other cxl functions. Which may result in kernel oops/panic when this
'struct cxl_afu *' is dereferenced.

So this patch puts a NULL check at the beginning of various cxl
functions that accept 'struct cxl_afu *' as a formal argument and are
called from with a loop of the form:

        for (i = 0; i < adapter->slices; i++) {
        	   	afu = adapter->afu[i];
		/* call some function with 'afu' */
	}

Surely in this case adapter->slices should be 0?

Not necessarily, as adapter->slice doesnt take into account AFUs that
fail to init. I saw this issue in one specific case were the only slice
on the card had issued with the AFU descriptor caused CXL init of that
AFU to fail.

We might still need to harden for other cases...

Yes we may need some more hardening especially in our AFU descriptor
parsing code.

-- 
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

From: Andrew Donnellan <hidden>
Date: 2018-03-09 06:02:45

On 09/03/18 13:59, Vaibhav Jain wrote:

Thanks for looking into this patch Andrew,

Andrew Donnellan [off-list ref] writes:

quoted

On 08/03/18 21:05, Vaibhav Jain wrote:

quoted

It is possible for a CXL card to have a valid PSL but no valid
AFUs. When this happens we have a valid instance of 'struct cxl'
representing the adapter but with its member 'struct cxl_afu *cxl[]'
as empty. Unfortunately at many placed within cxl code (especially
during an EEH) the elements of this array are passed on to various
other cxl functions. Which may result in kernel oops/panic when this
'struct cxl_afu *' is dereferenced.

So this patch puts a NULL check at the beginning of various cxl
functions that accept 'struct cxl_afu *' as a formal argument and are
called from with a loop of the form:

         for (i = 0; i < adapter->slices; i++) {
         	   	afu = adapter->afu[i];
		/* call some function with 'afu' */
	}

Surely in this case adapter->slices should be 0?

Not necessarily, as adapter->slice doesnt take into account AFUs that
fail to init. I saw this issue in one specific case were the only slice
on the card had issued with the AFU descriptor caused CXL init of that
AFU to fail.

Ah OK, makes sense.


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

From: Frederic Barrat <hidden>
Date: 2018-03-13 10:20:36


Le 08/03/2018 à 11:05, Vaibhav Jain a écrit :

It is possible for a CXL card to have a valid PSL but no valid
AFUs. When this happens we have a valid instance of 'struct cxl'
representing the adapter but with its member 'struct cxl_afu *cxl[]'
as empty. Unfortunately at many placed within cxl code (especially
during an EEH) the elements of this array are passed on to various
other cxl functions. Which may result in kernel oops/panic when this
'struct cxl_afu *' is dereferenced.

So this patch puts a NULL check at the beginning of various cxl
functions that accept 'struct cxl_afu *' as a formal argument and are
called from with a loop of the form:

        for (i = 0; i < adapter->slices; i++) {
        	   	afu = adapter->afu[i];
		/* call some function with 'afu' */
	}


So we are calling functions with an invalid afu argument. We can verify 
in the callees the value of the afu pointer, like you're doing here, but 
why not tackle it at source and avoid calling the function in the first 
place? It would have the nice side effect of reminding developers that 
the AFU array can be empty.
We already have a few checks in place in the "for (i = 0; i < 
adapter->slices; i++)" loops, but it was overlooked when eeh support was 
added. I think we should fix that.

   Fred

quoted hunk

Signed-off-by: Vaibhav Jain <redacted>
---
  drivers/misc/cxl/api.c     |  2 +-
  drivers/misc/cxl/context.c |  3 +++
  drivers/misc/cxl/guest.c   |  4 ++++
  drivers/misc/cxl/main.c    |  3 +++
  drivers/misc/cxl/native.c  |  4 ++++
  drivers/misc/cxl/pci.c     | 13 ++++++++++++-
  6 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 753b1a698fc4..3466ef8b9e86 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c

@@ -128,7 +128,7 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev *dev)
  	int rc;

  	afu = cxl_pci_to_afu(dev);
-	if (IS_ERR(afu))
+	if (IS_ERR_OR_NULL(afu))
  		return ERR_CAST(afu);

  	ctx = cxl_context_alloc();

diff --git a/drivers/misc/cxl/context.c b/drivers/misc/cxl/context.c
index 7ff315ad3692..3957e6e7d187 100644
--- a/drivers/misc/cxl/context.c
+++ b/drivers/misc/cxl/context.c

@@ -303,6 +303,9 @@ void cxl_context_detach_all(struct cxl_afu *afu)
  	struct cxl_context *ctx;
  	int tmp;

+	if (afu == NULL)
+		return;
+
  	mutex_lock(&afu->contexts_lock);
  	idr_for_each_entry(&afu->contexts_idr, ctx, tmp) {
  		/*

diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index f58b4b6c79f2..8165f6f26704 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c

@@ -760,6 +760,8 @@ static int activate_afu_directed(struct cxl_afu *afu)

  static int guest_afu_activate_mode(struct cxl_afu *afu, int mode)
  {
+	if (afu == NULL)
+		return -EINVAL;
  	if (!mode)
  		return 0;
  	if (!(mode & afu->modes_supported))

@@ -791,6 +793,8 @@ static int deactivate_afu_directed(struct cxl_afu *afu)

  static int guest_afu_deactivate_mode(struct cxl_afu *afu, int mode)
  {
+	if (afu == NULL)
+		return -EINVAL;
  	if (!mode)
  		return 0;
  	if (!(mode & afu->modes_supported))

diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c
index c1ba0d42cbc8..296a71ca6f2e 100644
--- a/drivers/misc/cxl/main.c
+++ b/drivers/misc/cxl/main.c

@@ -271,6 +271,9 @@ struct cxl_afu *cxl_alloc_afu(struct cxl *adapter, int slice)

  int cxl_afu_select_best_mode(struct cxl_afu *afu)
  {
+	if (afu == NULL)
+		return -EINVAL;
+
  	if (afu->modes_supported & CXL_MODE_DIRECTED)
  		return cxl_ops->afu_activate_mode(afu, CXL_MODE_DIRECTED);

diff --git a/drivers/misc/cxl/native.c b/drivers/misc/cxl/native.c
index 1b3d7c65ea3f..d46415b19b71 100644
--- a/drivers/misc/cxl/native.c
+++ b/drivers/misc/cxl/native.c

@@ -971,6 +971,8 @@ static int deactivate_dedicated_process(struct cxl_afu *afu)

  static int native_afu_deactivate_mode(struct cxl_afu *afu, int mode)
  {
+	if (afu == NULL)
+		return -EINVAL;
  	if (mode == CXL_MODE_DIRECTED)
  		return deactivate_afu_directed(afu);
  	if (mode == CXL_MODE_DEDICATED)

@@ -980,6 +982,8 @@ static int native_afu_deactivate_mode(struct cxl_afu *afu, int mode)

  static int native_afu_activate_mode(struct cxl_afu *afu, int mode)
  {
+	if (!afu)
+		return -EINVAL;
  	if (!mode)
  		return 0;
  	if (!(mode & afu->modes_supported))

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 758842f65a1b..8c87d9fdcf5a 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -1295,6 +1295,9 @@ static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pc
  {
  	int rc;

+	if (afu == NULL)
+		return -EINVAL;
+
  	if ((rc = pci_map_slice_regs(afu, adapter, dev)))
  		return rc;

@@ -1341,6 +1344,10 @@ static int pci_configure_afu(struct cxl_afu *afu, struct cxl *adapter, struct pc

  static void pci_deconfigure_afu(struct cxl_afu *afu)
  {
+
+	if (afu == NULL)
+		return;
+
  	/*
  	 * It's okay to deconfigure when AFU is already locked, otherwise wait
  	 * until there are no readers

@@ -2078,6 +2085,10 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
  	pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
  	pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;

+	/* Force a hotplug if an uninitiaized AFU is encountered */
+	if (afu == NULL)
+		return PCI_ERS_RESULT_DISCONNECT;
+
  	/* There should only be one entry, but go through the list
  	 * anyway
  	 */

@@ -2330,7 +2341,7 @@ static void cxl_pci_resume(struct pci_dev *pdev)
  	for (i = 0; i < adapter->slices; i++) {
  		afu = adapter->afu[i];

-		if (afu->phb == NULL)
+		if (afu == NULL || afu->phb == NULL)
  			continue;

  		list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {

Re: [PATCH] cxl: Perform NULL check for 'cxl_afu *' at various places in cxl

From: Vaibhav Jain <hidden>
Date: 2018-03-15 06:14:19

Thanks for reviewing this patch Fred,

Frederic Barrat [off-list ref] writes:

So we are calling functions with an invalid afu argument. We can verify 
in the callees the value of the afu pointer, like you're doing here, but 
why not tackle it at source and avoid calling the function in the first 
place?

Couple of reasons:
* Having checks for NULL afu at the call sites rather than the functions
  being called will lead to these checks scattered at more than a few
  places in code. Many of these functions are called via indirect branch
  from cxl_backend_ops and have their call sites scattered in the
  code. Having this check in the called functions is a much smaller change
  in comparison.

* Some of these functions are called are called from outside CXL e.g
  cxl_dev_context_init(). So putting the NULL check at every caller site
  might not be possible.

* Some funtions need to handle AFU init failures in special way e.g like
  cxl_vphb_error_detected() need to force a card hotplug so that AFU
  reinit can be tried.

* None of these functions are in any hotpath in the code so having an
  extra check at the beginning of the code wont have a much performance
  impact.

-- 
Vaibhav Jain [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help