diff mbox series

[v2,4/7] libnvdimm/bus: Prepare the nd_ioctl() path to be re-entrant

Message ID 156341208947.292348.10560140326807607481.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
State Mainlined
Commit 6de5d06e657acdbcf9637dac37916a4a5309e0f4
Headers show
Series libnvdimm: Fix async operations and locking | expand

Commit Message

Dan Williams July 18, 2019, 1:08 a.m. UTC 
In preparation for not holding a lock over the execution of nd_ioctl(),
update the implementation to allow multiple threads to be attempting
ioctls at the same time. The bus lock still prevents multiple in-flight
->ndctl() invocations from corrupting each other's state, but static
global staging buffers are moved to the heap.

Reported-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/bus.c |   59 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

Comments

Verma, Vishal L July 18, 2019, 6:21 p.m. UTC | #1 
On Wed, 2019-07-17 at 18:08 -0700, Dan Williams wrote:
> In preparation for not holding a lock over the execution of
> nd_ioctl(),
> update the implementation to allow multiple threads to be attempting
> ioctls at the same time. The bus lock still prevents multiple in-
> flight
> ->ndctl() invocations from corrupting each other's state, but static
> global staging buffers are moved to the heap.
> 
> Reported-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/nvdimm/bus.c |   59 +++++++++++++++++++++++++++++++--------
> -----------
>  1 file changed, 37 insertions(+), 22 deletions(-)

Ran tens of iterations of the unit tests with this, and couldn't
reproduce the failure.

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
Tested-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 42713b210f51..a3180c28fb2b 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -970,20 +970,19 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		int read_only, unsigned int ioctl_cmd, unsigned long
> arg)
>  {
>  	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
> -	static char out_env[ND_CMD_MAX_ENVELOPE];
> -	static char in_env[ND_CMD_MAX_ENVELOPE];
>  	const struct nd_cmd_desc *desc = NULL;
>  	unsigned int cmd = _IOC_NR(ioctl_cmd);
>  	struct device *dev = &nvdimm_bus->dev;
>  	void __user *p = (void __user *) arg;
> +	char *out_env = NULL, *in_env = NULL;
>  	const char *cmd_name, *dimm_name;
>  	u32 in_len = 0, out_len = 0;
>  	unsigned int func = cmd;
>  	unsigned long cmd_mask;
>  	struct nd_cmd_pkg pkg;
>  	int rc, i, cmd_rc;
> +	void *buf = NULL;
>  	u64 buf_len = 0;
> -	void *buf;
>  
>  	if (nvdimm) {
>  		desc = nd_cmd_dimm_desc(cmd);
> @@ -1023,6 +1022,9 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		}
>  
>  	/* process an input envelope */
> +	in_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
> +	if (!in_env)
> +		return -ENOMEM;
>  	for (i = 0; i < desc->in_num; i++) {
>  		u32 in_size, copy;
>  
> @@ -1030,14 +1032,17 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		if (in_size == UINT_MAX) {
>  			dev_err(dev, "%s:%s unknown input size cmd: %s
> field: %d\n",
>  					__func__, dimm_name, cmd_name,
> i);
> -			return -ENXIO;
> +			rc = -ENXIO;
> +			goto out;
>  		}
> -		if (in_len < sizeof(in_env))
> -			copy = min_t(u32, sizeof(in_env) - in_len,
> in_size);
> +		if (in_len < ND_CMD_MAX_ENVELOPE)
> +			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - in_len,
> in_size);
>  		else
>  			copy = 0;
> -		if (copy && copy_from_user(&in_env[in_len], p + in_len,
> copy))
> -			return -EFAULT;
> +		if (copy && copy_from_user(&in_env[in_len], p + in_len,
> copy)) {
> +			rc = -EFAULT;
> +			goto out;
> +		}
>  		in_len += in_size;
>  	}
>  
> @@ -1049,6 +1054,12 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  	}
>  
>  	/* process an output envelope */
> +	out_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
> +	if (!out_env) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
>  	for (i = 0; i < desc->out_num; i++) {
>  		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
>  				(u32 *) in_env, (u32 *) out_env, 0);
> @@ -1057,15 +1068,18 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		if (out_size == UINT_MAX) {
>  			dev_dbg(dev, "%s unknown output size cmd: %s
> field: %d\n",
>  					dimm_name, cmd_name, i);
> -			return -EFAULT;
> +			rc = -EFAULT;
> +			goto out;
>  		}
> -		if (out_len < sizeof(out_env))
> -			copy = min_t(u32, sizeof(out_env) - out_len,
> out_size);
> +		if (out_len < ND_CMD_MAX_ENVELOPE)
> +			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - out_len,
> out_size);
>  		else
>  			copy = 0;
>  		if (copy && copy_from_user(&out_env[out_len],
> -					p + in_len + out_len, copy))
> -			return -EFAULT;
> +					p + in_len + out_len, copy)) {
> +			rc = -EFAULT;
> +			goto out;
> +		}
>  		out_len += out_size;
>  	}
>  
> @@ -1073,12 +1087,15 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
>  		dev_dbg(dev, "%s cmd: %s buf_len: %llu > %d\n",
> dimm_name,
>  				cmd_name, buf_len, ND_IOCTL_MAX_BUFLEN);
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto out;
>  	}
>  
>  	buf = vmalloc(buf_len);
> -	if (!buf)
> -		return -ENOMEM;
> +	if (!buf) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>  
>  	if (copy_from_user(buf, p, buf_len)) {
>  		rc = -EFAULT;
> @@ -1100,17 +1117,15 @@ static int __nd_ioctl(struct nvdimm_bus
> *nvdimm_bus, struct nvdimm *nvdimm,
>  		nvdimm_account_cleared_poison(nvdimm_bus, clear_err-
> >address,
>  				clear_err->cleared);
>  	}
> -	nvdimm_bus_unlock(&nvdimm_bus->dev);
>  
>  	if (copy_to_user(p, buf, buf_len))
>  		rc = -EFAULT;
>  
> -	vfree(buf);
> -	return rc;
> -
> - out_unlock:
> +out_unlock:
>  	nvdimm_bus_unlock(&nvdimm_bus->dev);
> - out:
> +out:
> +	kfree(in_env);
> +	kfree(out_env);
>  	vfree(buf);
>  	return rc;
>  }
>
diff mbox series

Patch

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 42713b210f51..a3180c28fb2b 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -970,20 +970,19 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		int read_only, unsigned int ioctl_cmd, unsigned long arg)
 {
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
-	static char out_env[ND_CMD_MAX_ENVELOPE];
-	static char in_env[ND_CMD_MAX_ENVELOPE];
 	const struct nd_cmd_desc *desc = NULL;
 	unsigned int cmd = _IOC_NR(ioctl_cmd);
 	struct device *dev = &nvdimm_bus->dev;
 	void __user *p = (void __user *) arg;
+	char *out_env = NULL, *in_env = NULL;
 	const char *cmd_name, *dimm_name;
 	u32 in_len = 0, out_len = 0;
 	unsigned int func = cmd;
 	unsigned long cmd_mask;
 	struct nd_cmd_pkg pkg;
 	int rc, i, cmd_rc;
+	void *buf = NULL;
 	u64 buf_len = 0;
-	void *buf;
 
 	if (nvdimm) {
 		desc = nd_cmd_dimm_desc(cmd);
@@ -1023,6 +1022,9 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		}
 
 	/* process an input envelope */
+	in_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
+	if (!in_env)
+		return -ENOMEM;
 	for (i = 0; i < desc->in_num; i++) {
 		u32 in_size, copy;
 
@@ -1030,14 +1032,17 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		if (in_size == UINT_MAX) {
 			dev_err(dev, "%s:%s unknown input size cmd: %s field: %d\n",
 					__func__, dimm_name, cmd_name, i);
-			return -ENXIO;
+			rc = -ENXIO;
+			goto out;
 		}
-		if (in_len < sizeof(in_env))
-			copy = min_t(u32, sizeof(in_env) - in_len, in_size);
+		if (in_len < ND_CMD_MAX_ENVELOPE)
+			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - in_len, in_size);
 		else
 			copy = 0;
-		if (copy && copy_from_user(&in_env[in_len], p + in_len, copy))
-			return -EFAULT;
+		if (copy && copy_from_user(&in_env[in_len], p + in_len, copy)) {
+			rc = -EFAULT;
+			goto out;
+		}
 		in_len += in_size;
 	}
 
@@ -1049,6 +1054,12 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	}
 
 	/* process an output envelope */
+	out_env = kzalloc(ND_CMD_MAX_ENVELOPE, GFP_KERNEL);
+	if (!out_env) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
 	for (i = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
 				(u32 *) in_env, (u32 *) out_env, 0);
@@ -1057,15 +1068,18 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		if (out_size == UINT_MAX) {
 			dev_dbg(dev, "%s unknown output size cmd: %s field: %d\n",
 					dimm_name, cmd_name, i);
-			return -EFAULT;
+			rc = -EFAULT;
+			goto out;
 		}
-		if (out_len < sizeof(out_env))
-			copy = min_t(u32, sizeof(out_env) - out_len, out_size);
+		if (out_len < ND_CMD_MAX_ENVELOPE)
+			copy = min_t(u32, ND_CMD_MAX_ENVELOPE - out_len, out_size);
 		else
 			copy = 0;
 		if (copy && copy_from_user(&out_env[out_len],
-					p + in_len + out_len, copy))
-			return -EFAULT;
+					p + in_len + out_len, copy)) {
+			rc = -EFAULT;
+			goto out;
+		}
 		out_len += out_size;
 	}
 
@@ -1073,12 +1087,15 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
 		dev_dbg(dev, "%s cmd: %s buf_len: %llu > %d\n", dimm_name,
 				cmd_name, buf_len, ND_IOCTL_MAX_BUFLEN);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 
 	buf = vmalloc(buf_len);
-	if (!buf)
-		return -ENOMEM;
+	if (!buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
 	if (copy_from_user(buf, p, buf_len)) {
 		rc = -EFAULT;
@@ -1100,17 +1117,15 @@  static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		nvdimm_account_cleared_poison(nvdimm_bus, clear_err->address,
 				clear_err->cleared);
 	}
-	nvdimm_bus_unlock(&nvdimm_bus->dev);
 
 	if (copy_to_user(p, buf, buf_len))
 		rc = -EFAULT;
 
-	vfree(buf);
-	return rc;
-
- out_unlock:
+out_unlock:
 	nvdimm_bus_unlock(&nvdimm_bus->dev);
- out:
+out:
+	kfree(in_env);
+	kfree(out_env);
 	vfree(buf);
 	return rc;
 }