diff mbox series

[v3,RESEND,2/2] tpm: add support for nonblocking operation

Message ID 153359005823.27531.1050952672299708433.stgit@tstruk-mobl1.jf.intel.com (mailing list archive)
State New, archived
Headers show
Series tpm: add support for nonblocking operation | expand

Commit Message

Tadeusz Struk Aug. 6, 2018, 9:14 p.m. UTC
Currently the TPM driver only supports blocking calls, which doesn't allow
asynchronous IO operations to the TPM hardware.
This patch changes it and adds support for nonblocking write and a new poll
function to enable applications, which want to take advantage of this.

Tested-by: Philip Tricca <philip.b.tricca@intel.com>
Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |  145 +++++++++++++++++++++++++++----------
 drivers/char/tpm/tpm-dev.c        |   16 +++-
 drivers/char/tpm/tpm-dev.h        |   16 +++-
 drivers/char/tpm/tpm-interface.c  |    1 
 drivers/char/tpm/tpm.h            |    1 
 drivers/char/tpm/tpmrm-dev.c      |   19 ++++-
 6 files changed, 145 insertions(+), 53 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

James Bottomley Aug. 6, 2018, 11:05 p.m. UTC | #1
On Mon, 2018-08-06 at 14:14 -0700, Tadeusz Struk wrote:
[...]
> +static void tpm_async_work(struct work_struct *work)
> +{
> +	struct file_priv *priv =
> +			container_of(work, struct file_priv,
> async_work);
> +	ssize_t ret;
> +
> +	ret = tpm_transmit(priv->chip, priv->space, priv-
> >data_buffer,
> +			   sizeof(priv->data_buffer), 0);

Here' you assume the buffer_mutex was taken in write, which is done
(see below).  However, here, since there was no change to tpm_transmit,
you'll sleep in the context of the worker queue waiting for the command
to complete and return.

> +	tpm_put_ops(priv->chip);
> +	if (ret > 0) {
> +		priv->data_pending = ret;
> +		mod_timer(&priv->user_read_timer, jiffies + (120 *
> HZ));
> +	}
> +	mutex_unlock(&priv->buffer_mutex);

But you don't release buffer_mutex here until the tpm command has
completed.

> +	wake_up_interruptible(&priv->async_wait);
> +}
> +

[...]
> @@ -118,25 +155,48 @@ ssize_t tpm_common_write(struct file *file,
> const char __user *buf,
>  	 * the char dev is held open.
>  	 */
>  	if (tpm_try_get_ops(priv->chip)) {
> -		mutex_unlock(&priv->buffer_mutex);
> -		return -EPIPE;
> +		ret = -EPIPE;
> +		goto out;
>  	}
> -	out_size = tpm_transmit(priv->chip, priv->space, priv-
> >data_buffer,
> -				sizeof(priv->data_buffer), 0);
>  
> -	tpm_put_ops(priv->chip);
> -	if (out_size < 0) {
> -		mutex_unlock(&priv->buffer_mutex);
> -		return out_size;
> +	/*
> +	 * If in nonblocking mode schedule an async job to send
> +	 * the command return the size.
> +	 * In case of error the err code will be returned in
> +	 * the subsequent read call.
> +	 */
> +	if (file->f_flags & O_NONBLOCK) {
> +		queue_work(tpm_dev_wq, &priv->async_work);
> +		return size;

Here you return holding the buffer_mutex, waiting for tpm_async_work to
release it.

But now I've written my tpm work item and got it queued, I can't write
another one without blocking on the buffer_mutex at the top of
tpm_common_write(), and since that doesn't get released until the
previous command completed, I can only queue one command before I
block.  For an async interface, shouldn't I be able to queue an
arbitrary number of commands without blocking?

James

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tadeusz Struk Aug. 7, 2018, 12:09 a.m. UTC | #2
On 08/06/2018 04:05 PM, James Bottomley wrote:
> For an async interface, shouldn't I be able to queue an
> arbitrary number of commands without blocking?

That was the approach in the v1 version of this patch, but
Jason requested this to be changed so that only one command
at a time can be processed.
Thanks,
James Bottomley Aug. 7, 2018, 12:35 a.m. UTC | #3
On Mon, 2018-08-06 at 17:09 -0700, Tadeusz Struk wrote:
> On 08/06/2018 04:05 PM, James Bottomley wrote:
> > For an async interface, shouldn't I be able to queue an
> > arbitrary number of commands without blocking?
> 
> That was the approach in the v1 version of this patch, but
> Jason requested this to be changed so that only one command
> at a time can be processed.

He did?  I don't remember that.  I think he told you the TPM itself can
only process one operation at once so you didn't need an elaborate
allocation scheme.

But anyway, if you're happy to limit the interface to block after one
command is issued, how is it useful as an asynchronous interface?  I
thought the whole argument for the patch was to avoid the producer-
consumer approach which is possible with the current interface and to
use a fully event driven polling interface which can be implemented
single threaded.  If you can block in submission, this latter isn't
really possible because your interface isn't really asynchronous.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tadeusz Struk Aug. 7, 2018, 5:54 p.m. UTC | #4
On 08/06/2018 05:35 PM, James Bottomley wrote:
> On Mon, 2018-08-06 at 17:09 -0700, Tadeusz Struk wrote:
>> On 08/06/2018 04:05 PM, James Bottomley wrote:
>>> For an async interface, shouldn't I be able to queue an
>>> arbitrary number of commands without blocking?
>>
>> That was the approach in the v1 version of this patch, but
>> Jason requested this to be changed so that only one command
>> at a time can be processed.
> 
> He did?  I don't remember that.  I think he told you the TPM itself can
> only process one operation at once so you didn't need an elaborate
> allocation scheme.

Right, but the allocation was needed only if more than one command
would be queued at a given time.

> 
> But anyway, if you're happy to limit the interface to block after one
> command is issued, how is it useful as an asynchronous interface?  I
> thought the whole argument for the patch was to avoid the producer-
> consumer approach which is possible with the current interface and to
> use a fully event driven polling interface which can be implemented
> single threaded.  If you can block in submission, this latter isn't
> really possible because your interface isn't really asynchronous.

Well it is. This change makes the interface non-blocking and adds a poll
interface. Application can submit a command in a non-blocking way, go
do something else and get a notification via poll mechanism when the
response is ready to consume. We could implement it in a way that more
commands can be queued at a time, but in this case there would need to
be limit on how many commands can be en-queued. Allowing to send many
commands without any limit could be harmful. So what would it be? 10? 50?
And what would happen if an application sends 10 commands only to find
out the the first has failed? The drive doesn't know about that as it
only copies buffers back and forth. There will need to be an interface
for the application to rollback all the enqueued commands and stat over.
Also what would be the use case for this? TPM is not a crypto accelerator
where one submits a batch of buffers for encryption. Usually the sequence
of commands requires that subsequent command needs to refer the result
from the previous one. For example first command creates a key and the
second does something with it passing a handle to the key created in step
one. Do you have any particular scenario in mind for multiple commands
in-flight?

Thanks,
Jason Gunthorpe Aug. 7, 2018, 6:20 p.m. UTC | #5
On Mon, Aug 06, 2018 at 04:05:48PM -0700, James Bottomley wrote:

> > @@ -118,25 +155,48 @@ ssize_t tpm_common_write(struct file *file,
> > const char __user *buf,
> >  	 * the char dev is held open.
> >  	 */
> >  	if (tpm_try_get_ops(priv->chip)) {
> > -		mutex_unlock(&priv->buffer_mutex);
> > -		return -EPIPE;
> > +		ret = -EPIPE;
> > +		goto out;
> >  	}
> > -	out_size = tpm_transmit(priv->chip, priv->space, priv-
> > >data_buffer,
> > -				sizeof(priv->data_buffer), 0);
> >  
> > -	tpm_put_ops(priv->chip);
> > -	if (out_size < 0) {
> > -		mutex_unlock(&priv->buffer_mutex);
> > -		return out_size;
> > +	/*
> > +	 * If in nonblocking mode schedule an async job to send
> > +	 * the command return the size.
> > +	 * In case of error the err code will be returned in
> > +	 * the subsequent read call.
> > +	 */
> > +	if (file->f_flags & O_NONBLOCK) {
> > +		queue_work(tpm_dev_wq, &priv->async_work);
> > +		return size;
> 
> Here you return holding the buffer_mutex, waiting for tpm_async_work to
> release it.

Doesn't lockdep complain when locks are left held after returning to
user space? Even if it doesn't, that is a pretty ugly thing to do.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tadeusz Struk Aug. 7, 2018, 7:09 p.m. UTC | #6
On 08/07/2018 11:20 AM, Jason Gunthorpe wrote:
> Doesn't lockdep complain when locks are left held after returning to
> user space? Even if it doesn't, that is a pretty ugly thing to do.

I didn't notice anything from lockdep during my testing,
but I will change it to release the lock before returning.
Will send v4 soon.
Thanks,
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..3f2089f75c30 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,34 @@ 
  * License.
  *
  */
+#include <linux/poll.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/workqueue.h>
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static struct workqueue_struct *tpm_dev_wq;
+static DEFINE_MUTEX(tpm_dev_wq_lock);
+
+static void tpm_async_work(struct work_struct *work)
+{
+	struct file_priv *priv =
+			container_of(work, struct file_priv, async_work);
+	ssize_t ret;
+
+	ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+			   sizeof(priv->data_buffer), 0);
+
+	tpm_put_ops(priv->chip);
+	if (ret > 0) {
+		priv->data_pending = ret;
+		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+	}
+	mutex_unlock(&priv->buffer_mutex);
+	wake_up_interruptible(&priv->async_wait);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
 	struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -29,30 +52,44 @@  static void user_reader_timeout(struct timer_list *t)
 	pr_warn("TPM user space timeout is deprecated (pid=%d)\n",
 		task_tgid_nr(current));
 
-	schedule_work(&priv->work);
+	schedule_work(&priv->timeout_work);
 }
 
-static void timeout_work(struct work_struct *work)
+static void tpm_timeout_work(struct work_struct *work)
 {
-	struct file_priv *priv = container_of(work, struct file_priv, work);
+	struct file_priv *priv = container_of(work, struct file_priv,
+					      timeout_work);
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->data_pending = 0;
 	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
 	mutex_unlock(&priv->buffer_mutex);
+	wake_up_interruptible(&priv->async_wait);
 }
 
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
-		     struct file_priv *priv, struct tpm_space *space)
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+		    struct file_priv *priv, struct tpm_space *space)
 {
 	priv->chip = chip;
 	priv->space = space;
 
 	mutex_init(&priv->buffer_mutex);
 	timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
-	INIT_WORK(&priv->work, timeout_work);
-
+	INIT_WORK(&priv->timeout_work, tpm_timeout_work);
+	INIT_WORK(&priv->async_work, tpm_async_work);
+	init_waitqueue_head(&priv->async_wait);
 	file->private_data = priv;
+	mutex_lock(&tpm_dev_wq_lock);
+	if (!tpm_dev_wq) {
+		tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+		if (!tpm_dev_wq) {
+			mutex_unlock(&tpm_dev_wq_lock);
+			return -ENOMEM;
+		}
+	}
+
+	mutex_unlock(&tpm_dev_wq_lock);
+	return 0;
 }
 
 ssize_t tpm_common_read(struct file *file, char __user *buf,
@@ -63,15 +100,17 @@  ssize_t tpm_common_read(struct file *file, char __user *buf,
 	int rc;
 
 	del_singleshot_timer_sync(&priv->user_read_timer);
-	flush_work(&priv->work);
+	flush_work(&priv->timeout_work);
 	mutex_lock(&priv->buffer_mutex);
 
 	if (priv->data_pending) {
 		ret_size = min_t(ssize_t, size, priv->data_pending);
-		rc = copy_to_user(buf, priv->data_buffer, ret_size);
-		memset(priv->data_buffer, 0, priv->data_pending);
-		if (rc)
-			ret_size = -EFAULT;
+		if (ret_size > 0) {
+			rc = copy_to_user(buf, priv->data_buffer, ret_size);
+			memset(priv->data_buffer, 0, priv->data_pending);
+			if (rc)
+				ret_size = -EFAULT;
+		}
 
 		priv->data_pending = 0;
 	}
@@ -84,10 +123,9 @@  ssize_t tpm_common_write(struct file *file, const char __user *buf,
 			 size_t size, loff_t *off)
 {
 	struct file_priv *priv = file->private_data;
-	size_t in_size = size;
-	ssize_t out_size;
+	int ret = 0;
 
-	if (in_size > TPM_BUFSIZE)
+	if (size > TPM_BUFSIZE)
 		return -E2BIG;
 
 	mutex_lock(&priv->buffer_mutex);
@@ -97,20 +135,19 @@  ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	 * buffered writes from blocking here.
 	 */
 	if (priv->data_pending != 0) {
-		mutex_unlock(&priv->buffer_mutex);
-		return -EBUSY;
+		ret = -EBUSY;
+		goto out;
 	}
 
-	if (copy_from_user
-	    (priv->data_buffer, (void __user *) buf, in_size)) {
-		mutex_unlock(&priv->buffer_mutex);
-		return -EFAULT;
+	if (copy_from_user(priv->data_buffer, buf, size)) {
+		ret = -EFAULT;
+		goto out;
 	}
 
-	if (in_size < 6 ||
-	    in_size < be32_to_cpu(*((__be32 *) (priv->data_buffer + 2)))) {
-		mutex_unlock(&priv->buffer_mutex);
-		return -EINVAL;
+	if (size < 6 ||
+	    size < be32_to_cpu(*((__be32 *)(priv->data_buffer + 2)))) {
+		ret = -EINVAL;
+		goto out;
 	}
 
 	/* atomic tpm command send and result receive. We only hold the ops
@@ -118,25 +155,48 @@  ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	 * the char dev is held open.
 	 */
 	if (tpm_try_get_ops(priv->chip)) {
-		mutex_unlock(&priv->buffer_mutex);
-		return -EPIPE;
+		ret = -EPIPE;
+		goto out;
 	}
-	out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
-				sizeof(priv->data_buffer), 0);
 
-	tpm_put_ops(priv->chip);
-	if (out_size < 0) {
-		mutex_unlock(&priv->buffer_mutex);
-		return out_size;
+	/*
+	 * If in nonblocking mode schedule an async job to send
+	 * the command return the size.
+	 * In case of error the err code will be returned in
+	 * the subsequent read call.
+	 */
+	if (file->f_flags & O_NONBLOCK) {
+		queue_work(tpm_dev_wq, &priv->async_work);
+		return size;
 	}
 
-	priv->data_pending = out_size;
+	ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
+			   sizeof(priv->data_buffer), 0);
+	tpm_put_ops(priv->chip);
+
+	if (ret > 0) {
+		priv->data_pending = ret;
+		mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+		ret = size;
+	}
+out:
 	mutex_unlock(&priv->buffer_mutex);
+	return ret;
+}
 
-	/* Set a timeout by which the reader must come claim the result */
-	mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+__poll_t tpm_common_poll(struct file *file, poll_table *wait)
+{
+	struct file_priv *priv = file->private_data;
+	__poll_t mask = 0;
 
-	return in_size;
+	poll_wait(file, &priv->async_wait, wait);
+
+	if (priv->data_pending)
+		mask = EPOLLIN | EPOLLRDNORM;
+	else
+		mask = EPOLLOUT | EPOLLWRNORM;
+
+	return mask;
 }
 
 /*
@@ -144,8 +204,17 @@  ssize_t tpm_common_write(struct file *file, const char __user *buf,
  */
 void tpm_common_release(struct file *file, struct file_priv *priv)
 {
+	flush_work(&priv->async_work);
 	del_singleshot_timer_sync(&priv->user_read_timer);
-	flush_work(&priv->work);
+	flush_work(&priv->timeout_work);
 	file->private_data = NULL;
 	priv->data_pending = 0;
 }
+
+void __exit tpm_dev_common_exit(void)
+{
+	if (tpm_dev_wq) {
+		destroy_workqueue(tpm_dev_wq);
+		tpm_dev_wq = NULL;
+	}
+}
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 98b9630c3a36..1e353a4f4b7e 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -24,6 +24,7 @@  static int tpm_open(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip;
 	struct file_priv *priv;
+	int rc = -ENOMEM;
 
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
 
@@ -37,15 +38,21 @@  static int tpm_open(struct inode *inode, struct file *file)
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
-		goto out;
+		goto out_clear;
 
-	tpm_common_open(file, chip, priv, NULL);
+	rc = tpm_common_open(file, chip, priv, NULL);
+	if (rc)
+		goto out_free;
 
 	return 0;
 
- out:
+out_free:
+	kfree(priv);
+
+out_clear:
 	clear_bit(0, &chip->is_open);
-	return -ENOMEM;
+
+	return rc;
 }
 
 /*
@@ -68,5 +75,6 @@  const struct file_operations tpm_fops = {
 	.open = tpm_open,
 	.read = tpm_common_read,
 	.write = tpm_common_write,
+	.poll = tpm_common_poll,
 	.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index 4048677bbd78..fa49c885a2cb 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -2,28 +2,32 @@ 
 #ifndef _TPM_DEV_H
 #define _TPM_DEV_H
 
+#include <linux/poll.h>
 #include "tpm.h"
 
 struct file_priv {
 	struct tpm_chip *chip;
 	struct tpm_space *space;
 
-	/* Data passed to and from the tpm via the read/write calls */
-	size_t data_pending;
+	/* Holds the amount of data passed or an error code from async op */
+	ssize_t data_pending;
 	struct mutex buffer_mutex;
 
 	struct timer_list user_read_timer;      /* user needs to claim result */
-	struct work_struct work;
+	struct work_struct timeout_work;
+	struct work_struct async_work;
+	wait_queue_head_t async_wait;
 
 	u8 data_buffer[TPM_BUFSIZE];
 };
 
-void tpm_common_open(struct file *file, struct tpm_chip *chip,
-		     struct file_priv *priv, struct tpm_space *space);
+int tpm_common_open(struct file *file, struct tpm_chip *chip,
+		    struct file_priv *priv, struct tpm_space *space);
 ssize_t tpm_common_read(struct file *file, char __user *buf,
 			size_t size, loff_t *off);
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
 			 size_t size, loff_t *off);
-void tpm_common_release(struct file *file, struct file_priv *priv);
+__poll_t tpm_common_poll(struct file *file, poll_table *wait);
 
+void tpm_common_release(struct file *file, struct file_priv *priv);
 #endif
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..9e3de6447512 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1428,6 +1428,7 @@  static void __exit tpm_exit(void)
 	class_destroy(tpm_class);
 	class_destroy(tpmrm_class);
 	unregister_chrdev_region(tpm_devt, 2*TPM_NUM_DEVICES);
+	tpm_dev_common_exit();
 }
 
 subsys_initcall(tpm_init);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..b909dc9e251f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -604,4 +604,5 @@  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 
 int tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
+void tpm_dev_common_exit(void);
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 96006c6b9696..f131c97c1577 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -23,14 +23,22 @@  static int tpmrm_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 
 	rc = tpm2_init_space(&priv->space);
-	if (rc) {
-		kfree(priv);
-		return -ENOMEM;
-	}
+	if (rc)
+		goto out_free;
 
-	tpm_common_open(file, chip, &priv->priv, &priv->space);
+	rc = tpm_common_open(file, chip, &priv->priv, &priv->space);
+	if (rc)
+		goto out_del_space;
 
 	return 0;
+
+out_del_space:
+	tpm2_del_space(chip, &priv->space);
+
+out_free:
+	kfree(priv);
+
+	return rc;
 }
 
 static int tpmrm_release(struct inode *inode, struct file *file)
@@ -51,5 +59,6 @@  const struct file_operations tpmrm_fops = {
 	.open = tpmrm_open,
 	.read = tpm_common_read,
 	.write = tpm_common_write,
+	.poll = tpm_common_poll,
 	.release = tpmrm_release,
 };