Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

Hi Kai,

On 5/5/22 3:50 AM, Kai Huang wrote:
> 
>> +       /* Submit GetQuote Request */
>> +       ret = tdx_get_quote_hypercall(buf);
>> +       if (ret) {
>> +               pr_err("GetQuote hypercall failed, status:%lx\n", ret);
>> +               ret = -EIO;
>> +               goto free_entry;
>> +       }
>> +
>> +       /* Add current quote entry to quote_list */
>> +       add_quote_entry(entry);
>> +
>> +       /* Wait for attestation completion */
>> +       ret = wait_for_completion_interruptible(&entry->compl);
>> +       if (ret < 0) {
>> +               ret = -EIO;
>> +               goto del_entry;
>> +       }
> 
> This is misuse of wait_for_completion_interruptible().
> 
> xxx_interruptible() essentially means this operation can be interrupted by
> signal.  Using xxx_interruptible() in driver IOCTL essentially means when it
> returns due to signal, the IOCTL should return -EINTR to let userspace know that
> your application received some signal needs handling, and this IOCTL isn't
> finished and you should retry.  So here we should return -EINTR (and cleanup all
> staff has been done) when wait_for_completion_interruptible() returns -
> ERESTARTSYS (in fact, it returns only -ERESTARTSYS or 0).


But in this case, I was expecting the user agent to check the Quote
buffer status code to understand the IN_FLIGHT, SUCCESS or FAILURE
status and handle it appropriately. So IMO, it should not matter what
we return for the failure case. For the IN_FLIGHT case, they can retry
if they want after checking the status code.

But I agree that EINTR is the appropriate return value for an
interrupted case. So, I will change it.

> 
> Since normally userspace application just ignore signals, and in this particular
> case, asking userspace to retry just makes things more complicated to handle, I

I am not sure how the user agent is going to be implemented. So I don't
want to make any assumptions. In this case, we are not asking user space
to implement the retry support using signals. But we are just giving 
them option to do it. It is up to them if they want to use it.

> think you can just use wait_for_completion_killable(), which only returns when
> the application receives signal that it is going to be killed.

If you agree with the above point, we can leave just it as 
*_interruptible(). But if you still see other issues, please let me
know.

> 
>> +
>> +       /* Copy output data back to user buffer */
>> +       if (copy_to_user((void __user *)quote_req.buf, buf->vmaddr,
>> quote_req.len))
>> +               ret = -EFAULT;
>> +
>> +del_entry:
>> +       del_quote_entry(entry);
>> +free_entry:
>> +       free_quote_entry(entry);
> 
> As I (and Isaku) mentioned before, when wait_for_completion_killable() returns
> with error, you cannot just convert the buffer to private and free it.  The VMM
> is still owning it (IN_FLIGHT).

Do you know what happens when VMM writes to a page which already marked
private? Will MapGPA fail during shared-private conversion?

> 
> One way to handle is you can put those buffers that are still owned by VMM to a
> new list, and have some kernel thread to periodically check buffer's status and
> free those are already released by VMM.  I haven't thought thoroughly, so maybe
> there's better way to handle, though.

Instead of adding new thread to just handle the cleanup, maybe I
can move the entire callback interrupt logic (contents of
attestation_callback_handler()) to a work queue and wake up this
work queue whenever we get the callback interrupt.

We can let the same work queue handle the cleanup for interrupted
requests. As for as how to identify the interrupted request, we
can add a bit in queue_entry for it and set it when we exit
wait_for_completion*() due to signals.

I will do a sample logic and get back to you.



> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer