Subject: Re: [v9,3/4] tpm: Initialize TPM and get durations and timeouts
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
References: <1459275554-12915-4-git-send-email-stefanb@linux.vnet.ibm.com>
 <20160331082435.GA9657@intel.com>
Cc: tpmdd-devel@lists.sourceforge.net, linux-doc@vger.kernel.org,
        linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
Message-ID: <56FD1F07.3010705@linux.vnet.ibm.com>
Date: Thu, 31 Mar 2016 08:58:47 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <20160331082435.GA9657@intel.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5461
Lines: 173

On 03/31/2016 04:24 AM, Jarkko Sakkinen wrote:
> On Tue, Mar 29, 2016 at 02:19:13PM -0400, Stefan Berger wrote:
>> Add the retrieval of TPM 1.2 durations and timeouts. Since this requires
>> the startup of the TPM, do this for TPM 1.2 and TPM 2.

>> @@ -343,6 +362,55 @@ static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
>>   };
>>   
>>   /*
>> + * Code related to the startup of the TPM 2 and startup of TPM 1.2 +
>> + * retrieval of timeouts and durations.
>> + */
>> +
>> +static void vtpm_proxy_work(struct work_struct *work)
>> +{
>> +	struct proxy_dev *proxy_dev = container_of(work, struct proxy_dev,
>> +						   work);
>> +	int rc;
>> +
>> +	if (proxy_dev->flags & VTPM_PROXY_FLAG_TPM2)
>> +		rc = tpm2_startup(proxy_dev->chip, TPM2_SU_CLEAR);
>> +	else
>> +		rc = tpm_get_timeouts(proxy_dev->chip);
>> +
>> +	if (rc)
>> +		goto err;
>> +
>> +	rc = tpm_chip_register(proxy_dev->chip);
>> +	if (rc)
>> +		goto err;
>> +
>> +	return;
>> +
>> +err:
>> +	vtpm_proxy_fops_undo_open(proxy_dev);
>> +}
>> +
>> +/*
>> + * vtpm_proxy_work_stop: make sure the work has finished
>> + *
>> + * This function is useful when user space closed the fd
>> + * while the driver still determines timeouts.
>> + */
>> +static void vtpm_proxy_work_stop(struct proxy_dev *proxy_dev)
>> +{
>> +	vtpm_proxy_fops_undo_open(proxy_dev);
>> +	flush_work(&proxy_dev->work);
> The main proxy driver patch should implement cancel() callback and then
> these could be swapped. This whole use of OPENED callback looks like a
> hack that is done because cancel is not implemented.

What OPENED callback are you referring to? We have a OPENED flag but not 
a callback.

The above handles the case where the vTPM process for example dies 
[during TPM_Startup()] and the file descriptor is closed. 
vtpm_proxy_fops_undo_open() ensures that the vtpm thread is not stuck 
waiting for the response to the TPM_Startup(). The subsequent 
flush_work() ensures that the thread has finished before we continue 
shutting down the instance. This cannot be swapped.

> A new flag CANCELED could be added and functions would return -ECANCEL
> if it is set. This should be part of original vTPM proxy driver
> functionally.

The above has nothing to do with cancellation from what I can see. We 
have an OPENED flag now which is set when the driver is fully 
operational and cleared when it is not. We could instead use a SHUTDOWN 
or CLOSED flag that works with the reverse meaning, clearing it where 
OPENED is set now and setting it where OPENED is cleared. Would this help?

>
> The current solution is unclean and hard to follow. It looks like "it
> could work" but the OPENED flag has too many hats that it wears IMHO.

It has only one meaning which can be replaced with a flag as indicated 
above.

That it is tested in vtpm_tpm_req_canceled is due to it indicating that 
the driver is not operational for the current command anymore, which 
gets it out of the loop in tpm_transmit. It would probably be worse to 
return a status flag from the status() callback and return a flag in the 
req_complete_mask that would then end up calling the recv() callback 
when there is nothing to receive. So the way it is now it triggers 
-ECANCELED in the tpm_transmit loop, which seems appropriate.

    Stefan


>
> /Jarkko
>
>> +}
>> +
>> +/*
>> + * vtpm_proxy_work_start: Schedule the work for TPM 1.2 & 2 initialization
>> + */
>> +static inline void vtpm_proxy_work_start(struct proxy_dev *proxy_dev)
>> +{
>> +	queue_work(workqueue, &proxy_dev->work);
>> +}
>> +
>> +/*
>>    * Code related to creation and deletion of device pairs
>>    */
>>   static struct proxy_dev *vtpm_proxy_create_proxy_dev(void)
>> @@ -357,6 +425,7 @@ static struct proxy_dev *vtpm_proxy_create_proxy_dev(void)
>>   
>>   	init_waitqueue_head(&proxy_dev->wq);
>>   	mutex_init(&proxy_dev->buf_lock);
>> +	INIT_WORK(&proxy_dev->work, vtpm_proxy_work);
>>   
>>   	chip = tpm_chip_alloc(NULL, &vtpm_proxy_tpm_ops);
>>   	if (IS_ERR(chip)) {
>> @@ -427,9 +496,7 @@ static struct file *vtpm_proxy_create_device(
>>   	if (proxy_dev->flags & VTPM_PROXY_FLAG_TPM2)
>>   		proxy_dev->chip->flags |= TPM_CHIP_FLAG_TPM2;
>>   
>> -	rc = tpm_chip_register(proxy_dev->chip);
>> -	if (rc)
>> -		goto err_vtpm_fput;
>> +	vtpm_proxy_work_start(proxy_dev);
>>   
>>   	vtpm_new_dev->fd = fd;
>>   	vtpm_new_dev->major = MAJOR(proxy_dev->chip->dev.devt);
>> @@ -438,12 +505,6 @@ static struct file *vtpm_proxy_create_device(
>>   
>>   	return file;
>>   
>> -err_vtpm_fput:
>> -	put_unused_fd(fd);
>> -	fput(file);
>> -
>> -	return ERR_PTR(rc);
>> -
>>   err_put_unused_fd:
>>   	put_unused_fd(fd);
>>   
>> @@ -458,6 +519,8 @@ err_delete_proxy_dev:
>>    */
>>   static void vtpm_proxy_delete_device(struct proxy_dev *proxy_dev)
>>   {
>> +	vtpm_proxy_work_stop(proxy_dev);
>> +
>>   	/*
>>   	 * A client may hold the 'ops' lock, so let it know that the server
>>   	 * side shuts down before we try to grab the 'ops' lock when
>> @@ -557,11 +620,24 @@ static int __init vtpm_module_init(void)
>>   		return rc;
>>   	}
>>   
>> +	workqueue = create_workqueue("tpm-vtpm");
>> +	if (!workqueue) {
>> +		pr_err("couldn't create workqueue\n");
>> +		rc = -ENOMEM;
>> +		goto err_vtpmx_cleanup;
>> +	}
>> +
>>   	return 0;
>> +
>> +err_vtpmx_cleanup:
>> +	vtpmx_cleanup();
>> +
>> +	return rc;
>>   }
>>   
>>   static void __exit vtpm_module_exit(void)
>>   {
>> +	destroy_workqueue(workqueue);
>>   	vtpmx_cleanup();
>>   }
>>