[PATCH] tpm: add support for nonblocking operation

[PATCH] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allows
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the driver supports only blocking calls, which
doesn't allow asynchronous operation. This patch changes it and adds support
for nonblocking write and a new poll function to enable applications using
the API as designed by the spec.
The new functionality can be tested using standard TPM tools implemented
in [2], with modified TCTI from [3].

[1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |  170 +++++++++++++++++++++++++++++++------
 drivers/char/tpm/tpm-dev.c        |    1 
 drivers/char/tpm/tpm-dev.h        |    9 +-
 drivers/char/tpm/tpmrm-dev.c      |    1 
 4 files changed, 149 insertions(+), 32 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..0f3378b5198a 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,46 @@
  * License.
  *
  */
+#include <linux/poll.h>
+#include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/workqueue.h>
 #include "tpm.h"
 #include "tpm-dev.h"
 
+static DECLARE_WAIT_QUEUE_HEAD(tpm_dev_wait);
+static struct workqueue_struct *tpm_dev_wq;
+
+struct tpm_dev_work {
+	struct work_struct work;
+	struct file_priv *priv;
+	struct tpm_space *space;
+	ssize_t resp_size;
+	bool nonblocking;
+};
+
+static void tpm_async_work(struct work_struct *work)
+{
+	struct tpm_dev_work *tpm_work =
+		container_of(work, struct tpm_dev_work, work);
+	struct file_priv *priv = tpm_work->priv;
+
+	tpm_work->resp_size = tpm_transmit(priv->chip, tpm_work->space,
+					   priv->data_buffer,
+					   sizeof(priv->data_buffer), 0);
+	if (!tpm_work->nonblocking) {
+		wake_up_interruptible(&tpm_dev_wait);
+		return;
+	}
+
+	tpm_put_ops(priv->chip);
+	priv->data_pending = tpm_work->resp_size;
+	mutex_unlock(&priv->buffer_mutex);
+	mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+	kfree(tpm_work);
+}
+
 static void user_reader_timeout(struct timer_list *t)
 {
 	struct file_priv *priv = from_timer(priv, t, user_read_timer);
@@ -40,6 +75,7 @@ static void timeout_work(struct work_struct *work)
 	priv->data_pending = 0;
 	memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
 	mutex_unlock(&priv->buffer_mutex);
+	wake_up_interruptible(&tpm_dev_wait);
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
@@ -66,10 +102,12 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 
 	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;
 	}
@@ -82,10 +120,11 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 			 size_t size, loff_t *off, struct tpm_space *space)
 {
 	struct file_priv *priv = file->private_data;
-	size_t in_size = size;
-	ssize_t out_size;
+	struct tpm_dev_work *work;
+	DECLARE_WAITQUEUE(wait, current);
+	int ret = 0;
 
-	if (in_size > TPM_BUFSIZE)
+	if (size > TPM_BUFSIZE)
 		return -E2BIG;
 
 	mutex_lock(&priv->buffer_mutex);
@@ -95,20 +134,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
@@ -116,25 +154,85 @@ 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, space, priv->data_buffer,
-				sizeof(priv->data_buffer), 0);
 
+	/*
+	 * Schedule an async job to send the command
+	 */
+	work = kzalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		tpm_put_ops(priv->chip);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	work->priv = priv;
+	work->space = space;
+	work->nonblocking = file->f_flags & O_NONBLOCK;
+	INIT_WORK(&work->work, tpm_async_work);
+	queue_work(tpm_dev_wq, &work->work);
+
+	/*
+	 * If in nonblocking mode, return the size.
+	 * In case of an error the error code will be
+	 * returned in the subsequent read call.
+	 */
+	if (work->nonblocking)
+		return size;
+
+	add_wait_queue(&tpm_dev_wait, &wait);
+	current->state = TASK_INTERRUPTIBLE;
+	for (;;) {
+		if (work->resp_size)
+			break;
+
+		if (signal_pending(current)) {
+			ret = -ERESTARTSYS;
+			break;
+		}
+		schedule();
+	}
+	current->state = TASK_RUNNING;
+	remove_wait_queue(&tpm_dev_wait, &wait);
 	tpm_put_ops(priv->chip);
-	if (out_size < 0) {
-		mutex_unlock(&priv->buffer_mutex);
-		return out_size;
+
+	if (ret)
+		goto out_free;
+
+	if (work->resp_size < 0) {
+		ret = work->resp_size;
+		goto out_free;
 	}
 
-	priv->data_pending = out_size;
+	priv->data_pending = work->resp_size;
+	kfree(work);
 	mutex_unlock(&priv->buffer_mutex);
-
-	/* Set a timeout by which the reader must come claim the result */
+	wake_up_interruptible(&tpm_dev_wait);
 	mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+	return size;
+
+out_free:
+	kfree(work);
+out:
+	mutex_unlock(&priv->buffer_mutex);
+	return ret;
+}
+
+__poll_t tpm_common_poll(struct file *file, poll_table *wait)
+{
+	struct file_priv *priv = file->private_data;
+	__poll_t mask = 0;
+
+	poll_wait(file, &tpm_dev_wait, wait);
+
+	if (priv->data_pending)
+		mask = EPOLLIN | EPOLLRDNORM;
+	else
+		mask = EPOLLOUT | EPOLLWRNORM;
 
-	return in_size;
+	return mask;
 }
 
 /*
@@ -147,3 +245,19 @@ void tpm_common_release(struct file *file, struct file_priv *priv)
 	file->private_data = NULL;
 	priv->data_pending = 0;
 }
+
+static int __init tpm_dev_common_init(void)
+{
+	tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
+
+	return !tpm_dev_wq ? -ENOMEM : 0;
+}
+
+late_initcall(tpm_dev_common_init);
+
+static void __exit tpm_dev_common_exit(void)
+{
+	destroy_workqueue(tpm_dev_wq);
+}
+
+__exitcall(tpm_dev_common_exit);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..dc0fdfdf24c4 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -74,5 +74,6 @@ const struct file_operations tpm_fops = {
 	.open = tpm_open,
 	.read = tpm_common_read,
 	.write = tpm_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 b24cfb4d3ee1..61a3ee0024e4 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -2,13 +2,13 @@
 #ifndef _TPM_DEV_H
 #define _TPM_DEV_H
 
+#include <linux/poll.h>
 #include "tpm.h"
 
 struct file_priv {
 	struct tpm_chip *chip;
-
-	/* 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 */
@@ -23,6 +23,7 @@ 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, struct tpm_space *space);
-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/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..bbfd384b67b2 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -60,6 +60,7 @@ const struct file_operations tpmrm_fops = {
 	.open = tpmrm_open,
 	.read = tpm_common_read,
 	.write = tpmrm_write,
+	.poll = tpm_common_poll,
 	.release = tpmrm_release,
 };
 

Re: [PATCH] tpm: add support for nonblocking operation

[Jason Gunthorpe]

On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
> The TCG SAPI specification [1] defines a set of functions, which allows
> applications to use the TPM device in either blocking or non-blocking fashion.
> Each command defined by the specification has a corresponding
> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> mode of operation. Currently the driver supports only blocking calls, which
> doesn't allow asynchronous operation. This patch changes it and adds support
> for nonblocking write and a new poll function to enable applications using
> the API as designed by the spec.
> The new functionality can be tested using standard TPM tools implemented
> in [2], with modified TCTI from [3].
> 
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
> [2] https://github.com/tpm2-software/tpm2-tools
> [3] https://github.com/tstruk/tpm2-tss/tree/async
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
>  drivers/char/tpm/tpm-dev-common.c |  170 +++++++++++++++++++++++++++++++------
>  drivers/char/tpm/tpm-dev.c        |    1 
>  drivers/char/tpm/tpm-dev.h        |    9 +-
>  drivers/char/tpm/tpmrm-dev.c      |    1 
>  4 files changed, 149 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index e4a04b2d3c32..0f3378b5198a 100644
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -17,11 +17,46 @@
>   * License.
>   *
>   */
> +#include <linux/poll.h>
> +#include <linux/sched/signal.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/workqueue.h>
>  #include "tpm.h"
>  #include "tpm-dev.h"
>  
> +static DECLARE_WAIT_QUEUE_HEAD(tpm_dev_wait);
> +static struct workqueue_struct *tpm_dev_wq;
> +
> +struct tpm_dev_work {
> +	struct work_struct work;
> +	struct file_priv *priv;
> +	struct tpm_space *space;
> +	ssize_t resp_size;
> +	bool nonblocking;
> +};

There can only be one operation going at once, so why not put this in
the file_priv? Which might be needed because..

> +static void tpm_async_work(struct work_struct *work)
> +{
> +	struct tpm_dev_work *tpm_work =
> +		container_of(work, struct tpm_dev_work, work);
> +	struct file_priv *priv = tpm_work->priv;

What prevents priv from being freed at this point?

> @@ -82,10 +120,11 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  			 size_t size, loff_t *off, struct tpm_space *space)
>  {
>  	struct file_priv *priv = file->private_data;
> -	size_t in_size = size;
> -	ssize_t out_size;
> +	struct tpm_dev_work *work;
> +	DECLARE_WAITQUEUE(wait, current);
> +	int ret = 0;

There is not really any reason to change the flow for the blocking
case, maybe just call the work function directly?

Also, it is getting confusing to have two things called 'work' (the
timer and the async executor)

> +__poll_t tpm_common_poll(struct file *file, poll_table *wait)
> +{
> +	struct file_priv *priv = file->private_data;
> +	__poll_t mask = 0;
> +
> +	poll_wait(file, &tpm_dev_wait, wait);

Why a global wait queue? Should be in file_priv I'd think.

> +static int __init tpm_dev_common_init(void)
> +{
> +	tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
> +
> +	return !tpm_dev_wq ? -ENOMEM : 0;
> +}
> +
> +late_initcall(tpm_dev_common_init);
> +
> +static void __exit tpm_dev_common_exit(void)
> +{
> +	destroy_workqueue(tpm_dev_wq);
> +}

I wonder if it is worth creating this when the first file is
opened.. Lots of systems have TPMs but few use the userspace..

Jason

Re: [PATCH] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/01/2018 07:37 AM, Jason Gunthorpe wrote:
>> +struct tpm_dev_work {
>> +	struct work_struct work;
>> +	struct file_priv *priv;
>> +	struct tpm_space *space;
>> +	ssize_t resp_size;
>> +	bool nonblocking;
>> +};
> There can only be one operation going at once, so why not put this in
> the file_priv? Which might be needed because..

I need the *space as well, which doesn't really belong to file_priv
that's why I added this new struct. You are right the file_priv
isn't really needed here. I used it to have the priv->data_pending
but I can have just a ptr to data_pending in tpm_dev_work.

> 
>> +static void tpm_async_work(struct work_struct *work)
>> +{
>> +	struct tpm_dev_work *tpm_work =
>> +		container_of(work, struct tpm_dev_work, work);
>> +	struct file_priv *priv = tpm_work->priv;
> What prevents priv from being freed at this point?

The flush_work() that is currently missing from tpm_common_release() :)

> 
>> @@ -82,10 +120,11 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>>  			 size_t size, loff_t *off, struct tpm_space *space)
>>  {
>>  	struct file_priv *priv = file->private_data;
>> -	size_t in_size = size;
>> -	ssize_t out_size;
>> +	struct tpm_dev_work *work;
>> +	DECLARE_WAITQUEUE(wait, current);
>> +	int ret = 0;
> There is not really any reason to change the flow for the blocking
> case, maybe just call the work function directly?

That was my first idea, but then I thought it mighty be better to
have the same flow for both blocking and non-blocking.
I'm fine with doing it either way.

> 
> Also, it is getting confusing to have two things called 'work' (the
> timer and the async executor)
> 
>> +__poll_t tpm_common_poll(struct file *file, poll_table *wait)
>> +{
>> +	struct file_priv *priv = file->private_data;
>> +	__poll_t mask = 0;
>> +
>> +	poll_wait(file, &tpm_dev_wait, wait);
> Why a global wait queue? Should be in file_priv I'd think.

Right, I will move it to file_priv.

> 
>> +static int __init tpm_dev_common_init(void)
>> +{
>> +	tpm_dev_wq = alloc_workqueue("tpm_dev_wq", WQ_MEM_RECLAIM, 0);
>> +
>> +	return !tpm_dev_wq ? -ENOMEM : 0;
>> +}
>> +
>> +late_initcall(tpm_dev_common_init);
>> +
>> +static void __exit tpm_dev_common_exit(void)
>> +{
>> +	destroy_workqueue(tpm_dev_wq);
>> +}
> I wonder if it is worth creating this when the first file is
> opened.. Lots of systems have TPMs but few use the userspace.

I can do that.

Thanks for review Jason. I will send a v2 soon.
-- 
Tadeusz

Re: [PATCH] tpm: add support for nonblocking operation

[Jason Gunthorpe]

On Fri, Jun 01, 2018 at 09:55:52AM -0700, Tadeusz Struk wrote:
> On 06/01/2018 07:37 AM, Jason Gunthorpe wrote:
> >> +struct tpm_dev_work {
> >> +	struct work_struct work;
> >> +	struct file_priv *priv;
> >> +	struct tpm_space *space;
> >> +	ssize_t resp_size;
> >> +	bool nonblocking;
> >> +};
> > There can only be one operation going at once, so why not put this in
> > the file_priv? Which might be needed because..
> 
> I need the *space as well, which doesn't really belong to file_priv
> that's why I added this new struct. You are right the file_priv
> isn't really needed here. I used it to have the priv->data_pending
> but I can have just a ptr to data_pending in tpm_dev_work.

I mean in the sense that each file_priv can have exactly one async
work outstanding, so you can just add this struct to file_priv instead
of allocing it everytime..

Jason

Re: [PATCH] tpm: add support for nonblocking operation

[Jarkko Sakkinen]

On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
> The TCG SAPI specification [1] defines a set of functions, which allows
> applications to use the TPM device in either blocking or non-blocking fashion.
> Each command defined by the specification has a corresponding
> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> mode of operation. Currently the driver supports only blocking calls, which
> doesn't allow asynchronous operation. This patch changes it and adds support
> for nonblocking write and a new poll function to enable applications using
> the API as designed by the spec.
> The new functionality can be tested using standard TPM tools implemented
> in [2], with modified TCTI from [3].

I would need some statistics before I have interest to take these
changes in any form eg use case where this matters in the end.

/Jarkko

Re: [PATCH] tpm: add support for nonblocking operation

[Jarkko Sakkinen]

On Mon, Jun 04, 2018 at 10:55:54PM +0300, Jarkko Sakkinen wrote:
> On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
> > The TCG SAPI specification [1] defines a set of functions, which allows
> > applications to use the TPM device in either blocking or non-blocking fashion.
> > Each command defined by the specification has a corresponding
> > Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
> > together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> > mode of operation. Currently the driver supports only blocking calls, which
> > doesn't allow asynchronous operation. This patch changes it and adds support
> > for nonblocking write and a new poll function to enable applications using
> > the API as designed by the spec.
> > The new functionality can be tested using standard TPM tools implemented
> > in [2], with modified TCTI from [3].
> 
> I would need some statistics before I have interest to take these
> changes in any form eg use case where this matters in the end.

And please CC linux-kernel and linux-security-module.

/Jarkko

Re: [PATCH] tpm: add support for nonblocking operation

[flihp]

On 06/04/2018 12:55 PM, Jarkko Sakkinen wrote:
> On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
>> The TCG SAPI specification [1] defines a set of functions, which allows
>> applications to use the TPM device in either blocking or non-blocking fashion.
>> Each command defined by the specification has a corresponding
>> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
>> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
>> mode of operation. Currently the driver supports only blocking calls, which
>> doesn't allow asynchronous operation. This patch changes it and adds support
>> for nonblocking write and a new poll function to enable applications using
>> the API as designed by the spec.
>> The new functionality can be tested using standard TPM tools implemented
>> in [2], with modified TCTI from [3].
> 
> I would need some statistics before I have interest to take these
> changes in any form eg use case where this matters in the end.

The use cases motivating this feature are the same ones that motivated
the non-blocking behavior of other kernel interfaces (files, sockets and
other hardware) that has the potential to block threads in a process. By
implementing this same behavior in the TPM driver our goal is to enable
use of the TPM in programming languages / frameworks implementing an
"event-driven" model. There are a lot of them out there but since the
TSS2 APIs are currently limited to C our example code is in glib / GSource.

Hopefully this is sufficient but if it isn't it would help us to get
additional details on what you're looking for.

Thanks,
Philip

Re: [PATCH] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/08/2018 12:36 PM, flihp wrote:
> On 06/04/2018 12:55 PM, Jarkko Sakkinen wrote:
>> On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
>>> The TCG SAPI specification [1] defines a set of functions, which allows
>>> applications to use the TPM device in either blocking or non-blocking fashion.
>>> Each command defined by the specification has a corresponding
>>> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
>>> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
>>> mode of operation. Currently the driver supports only blocking calls, which
>>> doesn't allow asynchronous operation. This patch changes it and adds support
>>> for nonblocking write and a new poll function to enable applications using
>>> the API as designed by the spec.
>>> The new functionality can be tested using standard TPM tools implemented
>>> in [2], with modified TCTI from [3].
>>
>> I would need some statistics before I have interest to take these
>> changes in any form eg use case where this matters in the end.
> 
> The use cases motivating this feature are the same ones that motivated
> the non-blocking behavior of other kernel interfaces (files, sockets and
> other hardware) that has the potential to block threads in a process. By
> implementing this same behavior in the TPM driver our goal is to enable
> use of the TPM in programming languages / frameworks implementing an
> "event-driven" model. There are a lot of them out there but since the
> TSS2 APIs are currently limited to C our example code is in glib / GSource.
> 
> Hopefully this is sufficient but if it isn't it would help us to get
> additional details on what you're looking for.

I tried to run some tests w/r/t blocking to non-blocking.
and for example for tpm2_createprimary (type RSA) it looks more less like this:
blocking:
real	0m3.194s
user	0m0.052s
sys	0m0.094s

vs nonblocking:
real	0m1.031s
user	0m0.043s
sys	0m0.162s

that's on Minnowboard Turbot D0 PLATFORM
running FW Version: MNW2MAX1.X64.0097.R01.1709211204

The number are different every time I run it though.
Is this what you wanted to see?

Thanks,
-- 
Tadeusz

[PATCH v2 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allows
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
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 feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3].

[1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async

---
Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
      tpm: add ptr to the tpm_space struct to file_priv
      tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  152 ++++++++++++++++++++++++++++---------
 drivers/char/tpm/tpm-dev.c        |   22 +++--
 drivers/char/tpm/tpm-dev.h        |   19 +++--
 drivers/char/tpm/tpmrm-dev.c      |   31 ++++----
 4 files changed, 152 insertions(+), 72 deletions(-)

--
TS

[PATCH v2 0/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

The TCG SAPI specification [1] defines a set of functions, which allows
applications to use the TPM device in either blocking or non-blocking fashion.
Each command defined by the specification has a corresponding
Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
mode of operation. Currently the TPM driver supports only blocking calls,
which doesn't allow asynchronous IO operations.
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 feature.
The new functionality can be tested using standard TPM tools implemented
in [2], together with modified TCTI from [3].

[1] https://trustedcomputinggroup.org/wp-content/uploads/TSS_SAPI_Version-1.1_Revision-22_review_030918.pdf
[2] https://github.com/tpm2-software/tpm2-tools
[3] https://github.com/tstruk/tpm2-tss/tree/async

---
Changes in v2:
- Split the change into two separate patches. First patch adds a pointer
  to the space to the struct file_priv to have access to it from the async job.
  This is to avoid memory allocations on every write call. Now everything
  what's needed is in the file_priv struct.
- Renamed the 'work' member of the timer to avoid confusion.
  Now there are 'timeout_work' and 'async_work'.
- Removed the global wait queue and moved it to file_priv.
- Only creating the work queue when the first file is opened.

Tadeusz Struk (2):
      tpm: add ptr to the tpm_space struct to file_priv
      tpm: add support for nonblocking operation

 drivers/char/tpm/tpm-dev-common.c |  152 ++++++++++++++++++++++++++++---------
 drivers/char/tpm/tpm-dev.c        |   22 +++--
 drivers/char/tpm/tpm-dev.h        |   19 +++--
 drivers/char/tpm/tpmrm-dev.c      |   31 ++++----
 4 files changed, 152 insertions(+), 72 deletions(-)

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

[PATCH v2 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |    8 +++++---
 drivers/char/tpm/tpm-dev.c        |   10 ++--------
 drivers/char/tpm/tpm-dev.h        |    5 +++--
 drivers/char/tpm/tpmrm-dev.c      |   14 ++------------
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-		     struct file_priv *priv)
+		     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);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-			 size_t size, loff_t *off, struct tpm_space *space)
+			 size_t size, loff_t *off)
 {
 	struct file_priv *priv = file->private_data;
 	size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 		mutex_unlock(&priv->buffer_mutex);
 		return -EPIPE;
 	}
-	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+	out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
 				sizeof(priv->data_buffer), 0);
 
 	tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
 	if (priv == NULL)
 		goto out;
 
-	tpm_common_open(file, chip, priv);
+	tpm_common_open(file, chip, priv, NULL);
 
 	return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
 	return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-			 size_t size, loff_t *off)
-{
-	return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
 	.llseek = no_llseek,
 	.open = tpm_open,
 	.read = tpm_common_read,
-	.write = tpm_write,
+	.write = tpm_common_write,
 	.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 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;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-		     struct file_priv *priv);
+		     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, struct tpm_space *space);
+			 size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	}
 
-	tpm_common_open(file, chip, &priv->priv);
+	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
 	return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-		   size_t size, loff_t *off)
-{
-	struct file_priv *fpriv = file->private_data;
-	struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-	return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operations tpmrm_fops = {
 	.owner = THIS_MODULE,
 	.llseek = no_llseek,
 	.open = tpmrm_open,
 	.read = tpm_common_read,
-	.write = tpmrm_write,
+	.write = tpm_common_write,
 	.release = tpmrm_release,
 };
-

[PATCH v2 1/2] tpm: add ptr to the tpm_space struct to file_priv

[Tadeusz Struk]

Add a ptr to struct tpm_space to the file_priv to have an easy
access to it in the async job without the need to allocate memory.
This also allows to consolidate of the write operations for
the two interfaces.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |    8 +++++---
 drivers/char/tpm/tpm-dev.c        |   10 ++--------
 drivers/char/tpm/tpm-dev.h        |    5 +++--
 drivers/char/tpm/tpmrm-dev.c      |   14 ++------------
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index e4a04b2d3c32..f0c033b69b62 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -43,9 +43,11 @@ static void timeout_work(struct work_struct *work)
 }
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-		     struct file_priv *priv)
+		     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);
@@ -79,7 +81,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 }
 
 ssize_t tpm_common_write(struct file *file, const char __user *buf,
-			 size_t size, loff_t *off, struct tpm_space *space)
+			 size_t size, loff_t *off)
 {
 	struct file_priv *priv = file->private_data;
 	size_t in_size = size;
@@ -119,7 +121,7 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 		mutex_unlock(&priv->buffer_mutex);
 		return -EPIPE;
 	}
-	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
+	out_size = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
 				sizeof(priv->data_buffer), 0);
 
 	tpm_put_ops(priv->chip);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index ebd74ab5abef..98b9630c3a36 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
 	if (priv == NULL)
 		goto out;
 
-	tpm_common_open(file, chip, priv);
+	tpm_common_open(file, chip, priv, NULL);
 
 	return 0;
 
@@ -48,12 +48,6 @@ static int tpm_open(struct inode *inode, struct file *file)
 	return -ENOMEM;
 }
 
-static ssize_t tpm_write(struct file *file, const char __user *buf,
-			 size_t size, loff_t *off)
-{
-	return tpm_common_write(file, buf, size, off, NULL);
-}
-
 /*
  * Called on file close
  */
@@ -73,6 +67,6 @@ const struct file_operations tpm_fops = {
 	.llseek = no_llseek,
 	.open = tpm_open,
 	.read = tpm_common_read,
-	.write = tpm_write,
+	.write = tpm_common_write,
 	.release = tpm_release,
 };
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index b24cfb4d3ee1..4048677bbd78 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -6,6 +6,7 @@
 
 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;
@@ -18,11 +19,11 @@ struct file_priv {
 };
 
 void tpm_common_open(struct file *file, struct tpm_chip *chip,
-		     struct file_priv *priv);
+		     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, struct tpm_space *space);
+			 size_t size, loff_t *off);
 void tpm_common_release(struct file *file, struct file_priv *priv);
 
 #endif
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index 1a0e97a5da5a..96006c6b9696 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -28,7 +28,7 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	}
 
-	tpm_common_open(file, chip, &priv->priv);
+	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
 	return 0;
 }
@@ -45,21 +45,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static ssize_t tpmrm_write(struct file *file, const char __user *buf,
-		   size_t size, loff_t *off)
-{
-	struct file_priv *fpriv = file->private_data;
-	struct tpmrm_priv *priv = container_of(fpriv, struct tpmrm_priv, priv);
-
-	return tpm_common_write(file, buf, size, off, &priv->space);
-}
-
 const struct file_operations tpmrm_fops = {
 	.owner = THIS_MODULE,
 	.llseek = no_llseek,
 	.open = tpmrm_open,
 	.read = tpm_common_read,
-	.write = tpmrm_write,
+	.write = tpm_common_write,
 	.release = tpmrm_release,
 };
-

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

[PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

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.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |  148 ++++++++++++++++++++++++++++---------
 drivers/char/tpm/tpm-dev.c        |   16 +++-
 drivers/char/tpm/tpm-dev.h        |   16 +++-
 drivers/char/tpm/tpmrm-dev.c      |   19 ++++-
 4 files changed, 146 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..97625639e20f 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,35 @@
  * License.
  *
  */
+#include <linux/poll.h>
+#include <linux/sched/signal.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 +53,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 +101,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 +124,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 +136,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 +156,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;
+}
+
+__poll_t tpm_common_poll(struct file *file, poll_table *wait)
+{
+	struct file_priv *priv = file->private_data;
+	__poll_t mask = 0;
 
-	/* Set a timeout by which the reader must come claim the result */
-	mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+	poll_wait(file, &priv->async_wait, wait);
 
-	return in_size;
+	if (priv->data_pending)
+		mask = EPOLLIN | EPOLLRDNORM;
+	else
+		mask = EPOLLOUT | EPOLLWRNORM;
+
+	return mask;
 }
 
 /*
@@ -144,8 +205,19 @@ 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;
 }
+
+static void __exit tpm_dev_common_exit(void)
+{
+	if (tpm_dev_wq) {
+		destroy_workqueue(tpm_dev_wq);
+		tpm_dev_wq = NULL;
+	}
+}
+
+__exitcall(tpm_dev_common_exit);
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/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,
 };

[PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

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.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/char/tpm/tpm-dev-common.c |  148 ++++++++++++++++++++++++++++---------
 drivers/char/tpm/tpm-dev.c        |   16 +++-
 drivers/char/tpm/tpm-dev.h        |   16 +++-
 drivers/char/tpm/tpmrm-dev.c      |   19 ++++-
 4 files changed, 146 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f0c033b69b62..97625639e20f 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -17,11 +17,35 @@
  * License.
  *
  */
+#include <linux/poll.h>
+#include <linux/sched/signal.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 +53,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 +101,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 +124,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 +136,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 +156,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;
+}
+
+__poll_t tpm_common_poll(struct file *file, poll_table *wait)
+{
+	struct file_priv *priv = file->private_data;
+	__poll_t mask = 0;
 
-	/* Set a timeout by which the reader must come claim the result */
-	mod_timer(&priv->user_read_timer, jiffies + (120 * HZ));
+	poll_wait(file, &priv->async_wait, wait);
 
-	return in_size;
+	if (priv->data_pending)
+		mask = EPOLLIN | EPOLLRDNORM;
+	else
+		mask = EPOLLOUT | EPOLLWRNORM;
+
+	return mask;
 }
 
 /*
@@ -144,8 +205,19 @@ 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;
 }
+
+static void __exit tpm_dev_common_exit(void)
+{
+	if (tpm_dev_wq) {
+		destroy_workqueue(tpm_dev_wq);
+		tpm_dev_wq = NULL;
+	}
+}
+
+__exitcall(tpm_dev_common_exit);
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/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,
 };

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

Re: [PATCH] tpm: add support for nonblocking operation

[James Bottomley]

On Fri, 2018-06-08 at 12:36 -0700, flihp wrote:
> On 06/04/2018 12:55 PM, Jarkko Sakkinen wrote:
> > On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
> > > The TCG SAPI specification [1] defines a set of functions, which
> > > allows applications to use the TPM device in either blocking or
> > > non-blocking fashion. Each command defined by the specification
> > > has a corresponding Tss2_Sys_<COMMAND>_Prepare() and
> > > Tss2_Sys_<COMMAND>_Complete() call, which together with
> > > Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> > > mode of operation. Currently the driver supports only blocking
> > > calls, which doesn't allow asynchronous operation. This patch
> > > changes it and adds support for nonblocking write and a new poll
> > > function to enable applications using the API as designed by the
> > > spec. The new functionality can be tested using standard TPM
> > > tools implemented in [2], with modified TCTI from [3].
> > 
> > I would need some statistics before I have interest to take these
> > changes in any form eg use case where this matters in the end.
> 
> The use cases motivating this feature are the same ones that
> motivated the non-blocking behavior of other kernel interfaces
> (files, sockets and other hardware) that has the potential to block
> threads in a process. By implementing this same behavior in the TPM
> driver our goal is to enable use of the TPM in programming languages
> / frameworks implementing an "event-driven" model. There are a lot of
> them out there but since the TSS2 APIs are currently limited to C our
> example code is in glib / GSource.

But the TPM isn't like any of those resources, which can handle
multiple outstanding requests at once and get more efficient
utilisation doing so: it *is* single threaded in operation.  Meaning if
it's occupied doing something when you make your request, you wait
until it's finished up before your request goes through.  This means
that even for modern webby languages, the single worker thread
producer/consumer queue model is the best way of handling it because
it's the model that most closely corresponds to actual operation.  That
model just works today with the worker thread owning the open tpmrm
device.

I mean some of the actual devices are polled rather than interrupt
driven to give those watching some idea of how primitive our control
bus is.

> Hopefully this is sufficient but if it isn't it would help us to get
> additional details on what you're looking for.

OK, so I still don't see an actual use case for having poll, but on the
other hand the patches look simple enough so I don't see them adding
unmaintainable complexity either; on that measure, I'm OK with this as
long as you support it.

James

Re: [PATCH v2 2/2] tpm: add support for nonblocking operation

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

Hi Tadeusz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on next-20180608]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/tpm-add-ptr-to-the-tpm_space-struct-to-file_priv/20180612-082203
config: i386-randconfig-x008-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: data definition has no type or storage class
    __exitcall(tpm_dev_common_exit);
    ^~~~~~~~~~
>> drivers/char//tpm/tpm-dev-common.c:223:1: error: type defaults to 'int' in declaration of '__exitcall' [-Werror=implicit-int]
>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: parameter names (without types) in function declaration
   drivers/char//tpm/tpm-dev-common.c:215:20: warning: 'tpm_dev_common_exit' defined but not used [-Wunused-function]
    static void __exit tpm_dev_common_exit(void)
                       ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +223 drivers/char//tpm/tpm-dev-common.c

   222	
 > 223	__exitcall(tpm_dev_common_exit);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28143 bytes --]

[PATCH v2 2/2] tpm: add support for nonblocking operation

[kbuild test robot]

Hi Tadeusz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on next-20180608]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/tpm-add-ptr-to-the-tpm_space-struct-to-file_priv/20180612-082203
config: i386-randconfig-x008-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: data definition has no type or storage class
    __exitcall(tpm_dev_common_exit);
    ^~~~~~~~~~
>> drivers/char//tpm/tpm-dev-common.c:223:1: error: type defaults to 'int' in declaration of '__exitcall' [-Werror=implicit-int]
>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: parameter names (without types) in function declaration
   drivers/char//tpm/tpm-dev-common.c:215:20: warning: 'tpm_dev_common_exit' defined but not used [-Wunused-function]
    static void __exit tpm_dev_common_exit(void)
                       ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +223 drivers/char//tpm/tpm-dev-common.c

   222	
 > 223	__exitcall(tpm_dev_common_exit);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH v2 2/2] tpm: add support for nonblocking operation

[kbuild test robot]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: multipart/mixed; boundary="OXfL5xGRrasGEqWY", Size: 1628 bytes --]

Hi Tadeusz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on next-20180608]
[cannot apply to v4.17]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tadeusz-Struk/tpm-add-ptr-to-the-tpm_space-struct-to-file_priv/20180612-082203
config: i386-randconfig-x008-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: data definition has no type or storage class
    __exitcall(tpm_dev_common_exit);
    ^~~~~~~~~~
>> drivers/char//tpm/tpm-dev-common.c:223:1: error: type defaults to 'int' in declaration of '__exitcall' [-Werror=implicit-int]
>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: parameter names (without types) in function declaration
   drivers/char//tpm/tpm-dev-common.c:215:20: warning: 'tpm_dev_common_exit' defined but not used [-Wunused-function]
    static void __exit tpm_dev_common_exit(void)
                       ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +223 drivers/char//tpm/tpm-dev-common.c

   222	
 > 223	__exitcall(tpm_dev_common_exit);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation


    [ Part 2, Application/GZIP 28 KB. ]
    [ Unable to print this part. ]

Re: [PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/11/2018 07:53 PM, kbuild test robot wrote:
> Hi Tadeusz,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on char-misc/char-misc-testing]
> [also build test ERROR on next-20180608]
> [cannot apply to v4.17]

Hi,
Thanks for the report. This patch should go on top of
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/?h=next-tpm
I will check the config tomorrow.
Thanks,
-- 
Tadeusz

[PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/11/2018 07:53 PM, kbuild test robot wrote:
> Hi Tadeusz,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on char-misc/char-misc-testing]
> [also build test ERROR on next-20180608]
> [cannot apply to v4.17]

Hi,
Thanks for the report. This patch should go on top of
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/?h=next-tpm
I will check the config tomorrow.
Thanks,
-- 
Tadeusz
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/11/2018 07:53 PM, kbuild test robot wrote:
> Hi Tadeusz,
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: data definition has no type or storage class
>     __exitcall(tpm_dev_common_exit);
>     ^~~~~~~~~~
>>> drivers/char//tpm/tpm-dev-common.c:223:1: error: type defaults to 'int' in declaration of '__exitcall' [-Werror=implicit-int]
>>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: parameter names (without types) in function declaration
>    drivers/char//tpm/tpm-dev-common.c:215:20: warning: 'tpm_dev_common_exit' defined but not used [-Wunused-function]
>     static void __exit tpm_dev_common_exit(void)
>                        ^~~~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors
> 
> vim +223 drivers/char//tpm/tpm-dev-common.c
> 
>    222	
>  > 223	__exitcall(tpm_dev_common_exit);

It is complaining about __exitcall here, because there is a module_exit() call
in drivers/char/tpm/tpm-interface.c already and the two files are build into the same object.
It is strange that the problem only manifests itself when cross compiling with ARCH=i386. Native 64bit build works fine.
I will simply call the tpm_dev_common_exit() from the tpm_exit() and re-spin a v3.
Thanks again for the report.
-- 
Tadeusz

[PATCH v2 2/2] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 06/11/2018 07:53 PM, kbuild test robot wrote:
> Hi Tadeusz,
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: data definition has no type or storage class
>     __exitcall(tpm_dev_common_exit);
>     ^~~~~~~~~~
>>> drivers/char//tpm/tpm-dev-common.c:223:1: error: type defaults to 'int' in declaration of '__exitcall' [-Werror=implicit-int]
>>> drivers/char//tpm/tpm-dev-common.c:223:1: warning: parameter names (without types) in function declaration
>    drivers/char//tpm/tpm-dev-common.c:215:20: warning: 'tpm_dev_common_exit' defined but not used [-Wunused-function]
>     static void __exit tpm_dev_common_exit(void)
>                        ^~~~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors
> 
> vim +223 drivers/char//tpm/tpm-dev-common.c
> 
>    222	
>  > 223	__exitcall(tpm_dev_common_exit);

It is complaining about __exitcall here, because there is a module_exit() call
in drivers/char/tpm/tpm-interface.c already and the two files are build into the same object.
It is strange that the problem only manifests itself when cross compiling with ARCH=i386. Native 64bit build works fine.
I will simply call the tpm_dev_common_exit() from the tpm_exit() and re-spin a v3.
Thanks again for the report.
-- 
Tadeusz
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: add support for nonblocking operation

[Jarkko Sakkinen]

On Fri, Jun 08, 2018 at 12:36:18PM -0700, flihp wrote:
> On 06/04/2018 12:55 PM, Jarkko Sakkinen wrote:
> > On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
> >> The TCG SAPI specification [1] defines a set of functions, which allows
> >> applications to use the TPM device in either blocking or non-blocking fashion.
> >> Each command defined by the specification has a corresponding
> >> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
> >> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> >> mode of operation. Currently the driver supports only blocking calls, which
> >> doesn't allow asynchronous operation. This patch changes it and adds support
> >> for nonblocking write and a new poll function to enable applications using
> >> the API as designed by the spec.
> >> The new functionality can be tested using standard TPM tools implemented
> >> in [2], with modified TCTI from [3].
> > 
> > I would need some statistics before I have interest to take these
> > changes in any form eg use case where this matters in the end.
> 
> The use cases motivating this feature are the same ones that motivated
> the non-blocking behavior of other kernel interfaces (files, sockets and
> other hardware) that has the potential to block threads in a process. By
> implementing this same behavior in the TPM driver our goal is to enable
> use of the TPM in programming languages / frameworks implementing an
> "event-driven" model. There are a lot of them out there but since the
> TSS2 APIs are currently limited to C our example code is in glib / GSource.
> 
> Hopefully this is sufficient but if it isn't it would help us to get
> additional details on what you're looking for.

Thanks Philip. I'll look into the patch itself.

/Jarkko

Re: [PATCH] tpm: add support for nonblocking operation

[Jarkko Sakkinen]

On Mon, Jun 11, 2018 at 05:13:34PM -0700, Tadeusz Struk wrote:
> On 06/08/2018 12:36 PM, flihp wrote:
> > On 06/04/2018 12:55 PM, Jarkko Sakkinen wrote:
> >> On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
> >>> The TCG SAPI specification [1] defines a set of functions, which allows
> >>> applications to use the TPM device in either blocking or non-blocking fashion.
> >>> Each command defined by the specification has a corresponding
> >>> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
> >>> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
> >>> mode of operation. Currently the driver supports only blocking calls, which
> >>> doesn't allow asynchronous operation. This patch changes it and adds support
> >>> for nonblocking write and a new poll function to enable applications using
> >>> the API as designed by the spec.
> >>> The new functionality can be tested using standard TPM tools implemented
> >>> in [2], with modified TCTI from [3].
> >>
> >> I would need some statistics before I have interest to take these
> >> changes in any form eg use case where this matters in the end.
> > 
> > The use cases motivating this feature are the same ones that motivated
> > the non-blocking behavior of other kernel interfaces (files, sockets and
> > other hardware) that has the potential to block threads in a process. By
> > implementing this same behavior in the TPM driver our goal is to enable
> > use of the TPM in programming languages / frameworks implementing an
> > "event-driven" model. There are a lot of them out there but since the
> > TSS2 APIs are currently limited to C our example code is in glib / GSource.
> > 
> > Hopefully this is sufficient but if it isn't it would help us to get
> > additional details on what you're looking for.
> 
> I tried to run some tests w/r/t blocking to non-blocking.
> and for example for tpm2_createprimary (type RSA) it looks more less like this:
> blocking:
> real	0m3.194s
> user	0m0.052s
> sys	0m0.094s
> 
> vs nonblocking:
> real	0m1.031s
> user	0m0.043s
> sys	0m0.162s
> 
> that's on Minnowboard Turbot D0 PLATFORM
> running FW Version: MNW2MAX1.X64.0097.R01.1709211204
> 
> The number are different every time I run it though.
> Is this what you wanted to see?

Thanks, this and Philips explanation make it worth of looking into!

/Jarkko

Re: [PATCH] tpm: add support for nonblocking operation

[flihp]

On 06/18/2018 11:25 AM, Jarkko Sakkinen wrote:
> On Fri, Jun 08, 2018 at 12:36:18PM -0700, flihp wrote:
>> On 06/04/2018 12:55 PM, Jarkko Sakkinen wrote:
>>> On Thu, May 31, 2018 at 04:29:09PM -0700, Tadeusz Struk wrote:
>>>> The TCG SAPI specification [1] defines a set of functions, which allows
>>>> applications to use the TPM device in either blocking or non-blocking fashion.
>>>> Each command defined by the specification has a corresponding
>>>> Tss2_Sys_<COMMAND>_Prepare() and Tss2_Sys_<COMMAND>_Complete() call, which
>>>> together with Tss2_Sys_ExecuteAsync() is designed to allow asynchronous
>>>> mode of operation. Currently the driver supports only blocking calls, which
>>>> doesn't allow asynchronous operation. This patch changes it and adds support
>>>> for nonblocking write and a new poll function to enable applications using
>>>> the API as designed by the spec.
>>>> The new functionality can be tested using standard TPM tools implemented
>>>> in [2], with modified TCTI from [3].
>>>
>>> I would need some statistics before I have interest to take these
>>> changes in any form eg use case where this matters in the end.
>>
>> The use cases motivating this feature are the same ones that motivated
>> the non-blocking behavior of other kernel interfaces (files, sockets and
>> other hardware) that has the potential to block threads in a process. By
>> implementing this same behavior in the TPM driver our goal is to enable
>> use of the TPM in programming languages / frameworks implementing an
>> "event-driven" model. There are a lot of them out there but since the
>> TSS2 APIs are currently limited to C our example code is in glib / GSource.
>>
>> Hopefully this is sufficient but if it isn't it would help us to get
>> additional details on what you're looking for.
> 
> Thanks Philip. I'll look into the patch itself.

Thanks for reviewing the patch Jarkko. While you're doing that I took
some time to hack up code to demonstrate the utility of supporting this
feature. The code can be found here:

https://github.com/flihp/glib-tss2-async-example

In short, the example application `glib-tss2-event` uses a glib main
event loop to create an RSA 2048 primary key in the TPM2 NULL hierarchy
while using a glib timer event to time the operation. A GSource object
is used to generate an event when the FD underlying the tss2 function
call has data ready. While the application waits for an event indicating
that the CreatePrimary operation is complete, it counts timer events
that occur every 100ms. Once the CreatePrimary operation completes the
number of timer events that occurred is used to make a rough calculation
of the elapsed time. This value is then printed to the console.

This takes ~300 lines of C code and requires no management or
synchronization of threads. The glib GMainContext is "just a poll()
loop" according to the glib documentation here:

https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en

and so supporting 'poll' is the easiest way to integrate with glib /
gtk+. This is true of any other event system that relies on 'poll'
instead of worker threads.

I've tested this against the userspace resource management daemon (which
supports 'poll') as well as the kernel interface using Tadeusz's patch
currently under review here. If / when this gets merged feel free to add
a "tested-by" line for myself:
Tested-by: Philip Tricca <philip.b.tricca@intel.com>

Thanks,
Philip

Re: [PATCH] tpm: add support for nonblocking operation

[Jarkko Sakkinen]

On Thu, Jul 05, 2018 at 04:15:06PM -0700, flihp wrote:
> Thanks for reviewing the patch Jarkko. While you're doing that I took
> some time to hack up code to demonstrate the utility of supporting this
> feature. The code can be found here:
> 
> https://github.com/flihp/glib-tss2-async-example
> 
> In short, the example application `glib-tss2-event` uses a glib main
> event loop to create an RSA 2048 primary key in the TPM2 NULL hierarchy
> while using a glib timer event to time the operation. A GSource object
> is used to generate an event when the FD underlying the tss2 function
> call has data ready. While the application waits for an event indicating
> that the CreatePrimary operation is complete, it counts timer events
> that occur every 100ms. Once the CreatePrimary operation completes the
> number of timer events that occurred is used to make a rough calculation
> of the elapsed time. This value is then printed to the console.
> 
> This takes ~300 lines of C code and requires no management or
> synchronization of threads. The glib GMainContext is "just a poll()
> loop" according to the glib documentation here:
> 
> https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en
> 
> and so supporting 'poll' is the easiest way to integrate with glib /
> gtk+. This is true of any other event system that relies on 'poll'
> instead of worker threads.
> 
> I've tested this against the userspace resource management daemon (which
> supports 'poll') as well as the kernel interface using Tadeusz's patch
> currently under review here. If / when this gets merged feel free to add
> a "tested-by" line for myself:
> Tested-by: Philip Tricca <philip.b.tricca@intel.com>

I see the value now. I'll test this at early August when I'm back from
my leave.

Noticed now that the whole patch should be reposted because CC is missing
linux-kernel and linux-security-module (maybe with a link to your test
application in the cover letter).

/Jarkko

Re: [PATCH] tpm: add support for nonblocking operation

[Tadeusz Struk]

On 07/16/2018 10:34 AM, Jarkko Sakkinen wrote:
> On Thu, Jul 05, 2018 at 04:15:06PM -0700, flihp wrote:
>> Thanks for reviewing the patch Jarkko. While you're doing that I took
>> some time to hack up code to demonstrate the utility of supporting this
>> feature. The code can be found here:
>>
>> https://github.com/flihp/glib-tss2-async-example
>>
>> In short, the example application `glib-tss2-event` uses a glib main
>> event loop to create an RSA 2048 primary key in the TPM2 NULL hierarchy
>> while using a glib timer event to time the operation. A GSource object
>> is used to generate an event when the FD underlying the tss2 function
>> call has data ready. While the application waits for an event indicating
>> that the CreatePrimary operation is complete, it counts timer events
>> that occur every 100ms. Once the CreatePrimary operation completes the
>> number of timer events that occurred is used to make a rough calculation
>> of the elapsed time. This value is then printed to the console.
>>
>> This takes ~300 lines of C code and requires no management or
>> synchronization of threads. The glib GMainContext is "just a poll()
>> loop" according to the glib documentation here:
>>
>> https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en
>>
>> and so supporting 'poll' is the easiest way to integrate with glib /
>> gtk+. This is true of any other event system that relies on 'poll'
>> instead of worker threads.
>>
>> I've tested this against the userspace resource management daemon (which
>> supports 'poll') as well as the kernel interface using Tadeusz's patch
>> currently under review here. If / when this gets merged feel free to add
>> a "tested-by" line for myself:
>> Tested-by: Philip Tricca <philip.b.tricca@intel.com>
> 
> I see the value now. I'll test this at early August when I'm back from
> my leave.
> 
> Noticed now that the whole patch should be reposted because CC is missing
> linux-kernel and linux-security-module (maybe with a link to your test
> application in the cover letter).

I'll rebase and resend it after you will be back from vacation.
Thanks,
-- 
Tadeusz

Re: [PATCH] tpm: add support for nonblocking operation

[Jarkko Sakkinen]

On Mon, Jul 16, 2018 at 01:10:42PM -0700, Tadeusz Struk wrote:
> On 07/16/2018 10:34 AM, Jarkko Sakkinen wrote:
> > On Thu, Jul 05, 2018 at 04:15:06PM -0700, flihp wrote:
> >> Thanks for reviewing the patch Jarkko. While you're doing that I took
> >> some time to hack up code to demonstrate the utility of supporting this
> >> feature. The code can be found here:
> >>
> >> https://github.com/flihp/glib-tss2-async-example
> >>
> >> In short, the example application `glib-tss2-event` uses a glib main
> >> event loop to create an RSA 2048 primary key in the TPM2 NULL hierarchy
> >> while using a glib timer event to time the operation. A GSource object
> >> is used to generate an event when the FD underlying the tss2 function
> >> call has data ready. While the application waits for an event indicating
> >> that the CreatePrimary operation is complete, it counts timer events
> >> that occur every 100ms. Once the CreatePrimary operation completes the
> >> number of timer events that occurred is used to make a rough calculation
> >> of the elapsed time. This value is then printed to the console.
> >>
> >> This takes ~300 lines of C code and requires no management or
> >> synchronization of threads. The glib GMainContext is "just a poll()
> >> loop" according to the glib documentation here:
> >>
> >> https://developer.gnome.org/programming-guidelines/stable/main-contexts.html.en
> >>
> >> and so supporting 'poll' is the easiest way to integrate with glib /
> >> gtk+. This is true of any other event system that relies on 'poll'
> >> instead of worker threads.
> >>
> >> I've tested this against the userspace resource management daemon (which
> >> supports 'poll') as well as the kernel interface using Tadeusz's patch
> >> currently under review here. If / when this gets merged feel free to add
> >> a "tested-by" line for myself:
> >> Tested-by: Philip Tricca <philip.b.tricca@intel.com>
> > 
> > I see the value now. I'll test this at early August when I'm back from
> > my leave.
> > 
> > Noticed now that the whole patch should be reposted because CC is missing
> > linux-kernel and linux-security-module (maybe with a link to your test
> > application in the cover letter).
> 
> I'll rebase and resend it after you will be back from vacation.
> Thanks,
> -- 
> Tadeusz

Please add also the requested CC's. Thank you.

/Jarkko