From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>,
"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Cc: "peterz@infradead.org" <peterz@infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 4/7] libnvdimm/bus: Prepare the nd_ioctl() path to be re-entrant
Date: Thu, 18 Jul 2019 18:21:20 +0000 [thread overview]
Message-ID: <76c3363adaf3fe1a553876e3b3d5af4af1ab922a.camel@intel.com> (raw)
In-Reply-To: <156341208947.292348.10560140326807607481.stgit@dwillia2-desk3.amr.corp.intel.com>
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;
> }
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
next prev parent reply other threads:[~2019-07-18 18:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-18 1:07 [PATCH v2 0/7] libnvdimm: Fix async operations and locking Dan Williams
2019-07-18 1:07 ` [PATCH v2 1/7] drivers/base: Introduce kill_device() Dan Williams
2019-07-18 2:29 ` Greg Kroah-Hartman
[not found] ` <156341207332.292348.14959761496009347574.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-07-19 0:45 ` Sasha Levin
2019-07-18 1:07 ` [PATCH v2 2/7] libnvdimm/bus: Prevent duplicate device_unregister() calls Dan Williams
2019-07-18 1:08 ` [PATCH v2 3/7] libnvdimm/region: Register badblocks before namespaces Dan Williams
2019-07-18 18:16 ` Verma, Vishal L
2019-07-18 1:08 ` [PATCH v2 4/7] libnvdimm/bus: Prepare the nd_ioctl() path to be re-entrant Dan Williams
2019-07-18 18:21 ` Verma, Vishal L [this message]
2019-07-18 1:08 ` [PATCH v2 5/7] libnvdimm/bus: Stop holding nvdimm_bus_list_mutex over __nd_ioctl() Dan Williams
2019-07-18 1:08 ` [PATCH v2 6/7] libnvdimm/bus: Fix wait_nvdimm_bus_probe_idle() ABBA deadlock Dan Williams
2019-07-18 2:04 ` Sasha Levin
2019-07-18 6:39 ` Dan Williams
2019-07-18 1:08 ` [PATCH v2 7/7] driver-core, libnvdimm: Let device subsystems add local lockdep coverage Dan Williams
2019-07-18 2:28 ` Greg Kroah-Hartman
2019-07-18 16:09 ` Ira Weiny
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=76c3363adaf3fe1a553876e3b3d5af4af1ab922a.camel@intel.com \
--to=vishal.l.verma@intel.com \
--cc=dan.j.williams@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).