Messages in this thread | | | Subject | Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support | From | Kai Huang <> | Date | Thu, 05 May 2022 22:50:09 +1200 |
| |
> + /* 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).
Since normally userspace application just ignore signals, and in this particular case, asking userspace to retry just makes things more complicated to handle, I think you can just use wait_for_completion_killable(), which only returns when the application receives signal that it is going to be killed.
> + > + /* 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).
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.
-- Thanks, -Kai
| |