Messages in this thread | | | Date | Tue, 17 May 2022 16:32:02 -0700 | Subject | Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support | From | Sathyanarayanan Kuppuswamy <> |
| |
On 5/17/22 4:06 PM, Kai Huang wrote: > On Tue, 2022-05-17 at 13:08 -0700, Sathyanarayanan Kuppuswamy wrote: >> >> On 5/16/22 7:58 PM, Kai Huang wrote: >>> On Fri, 2022-05-13 at 12:29 -0700, Sathyanarayanan Kuppuswamy wrote: >>>>> >>>>> >>>>>> + >>>>>> + /* Wait for attestation completion */ >>>>>> + ret = wait_for_completion_interruptible(&entry->compl); >>>>>> + if (ret < 0) { >>>>>> + /* >>>>>> + * For premature termination, since VMM still owns the >>>>>> + * shared buffer, mark the request invalid to let >>>>>> + * quote_callback_handler() handle the memory cleanup >>>>>> + * function. >>>>>> + */ >>>>>> + invalidate_quote_request(entry); >>>>> >>>>> Interrupt can arrive after signal interrupt. So invalidate_quote_request() >>>>> should check if the request is already processed, and return 0 or -EINTR. >>>>> Probably check the state always and del_list under single lock/unlock pair. >>>> >>>> Agree. But I think we should return -EINTR for the interrupted case >>>> irrespective of the processed status (so no return 0). >>>> >>>> I will hold the lock and handle the cleanup for the processed >>>> status. >>> >>> Even if we check the buffer status in invalidate_quote_request(), there's no >>> guarantee the VMM won't change the buffer status _after_ we do the check, so >>> looks such check isn't necessary. >>> >> >> Consider the case where we get a callback interrupt, and before we >> complete the processing for it, user terminates the request. In this >> scenario, quote_callback_handler() will consider the request is >> still valid and no do the memory cleanup. To handle this case, >> we need to check the status in invalidate_quote_request() and do >> the cleanup if required. >> >> /* Handles early termination of GetQuote requests */ >> void invalidate_quote_request(struct quote_entry *entry) >> { >> struct tdx_quote_hdr *quote_hdr; >> >> /* >> * For early termination, if the request is not yet >> * processed by VMM (GET_QUOTE_IN_FLIGHT), the VMM >> * still owns the shared buffer, so mark the request >> * invalid to let quote_callback_handler() handle the >> * memory cleanup function. If the request is already >> * processed, then do the cleanup and return. >> */ >> >> mutex_lock("e_lock); >> quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr; >> if (quote_hdr->status == GET_QUOTE_IN_FLIGHT) { > > What prevents VMM from updating quote_hdr->status from IN_FLIGHT to DONE _after_ > this check?
If IN_FLIGHT -> DONE happens at this point, you will get a separate callback interrupt for it after this change. Since we hold quote_lock here, processing of that callback work will wait till we make the request invalid. For invalid requests, quote_callback_handler() will handle the cleanup.
This logic is mainly for the case where quote_callback_handler() in process and user terminates the request. In this case, quote_callback_handler() will not do the cleanup, and we should add logic here to handle it.
> > If you want to add such check, you should check against GET_QUOTE_DONE, but not > IN_FLIGHT. Only after status is DONE, VMM will not update the buffer. Perhaps > something like below: > > mutex_lock("e_lock); > /* Skip invalidate the buffer if VMM has done with the buffer */ > if (quote_hdr->status == GET_QUOTE_DONE) { > mutex_unlock("e_lock); > return 0; > }
There is no DONE status. Status can be either one of the following values.
GET_QUOTE_ERROR GET_QUOTE_SUCCESS GET_QUOTE_SERVICE_UNAVAILABLE GET_QUOTE_IN_FLIGHT
> > And in the IOCTL, you can perhaps to choose to return 0, instead of -EINTR in > this case, as the Quote has been finished already.
I think your logic above is to return 0 for the completed request even if it is interrupted. But IMO, since the user already interrupted the request, then I think it is more appropriate to just return -EINTR for this case.
> > But I am not sure whether this is necessary. The worst case is one finished > Quote is wasted I guess.
> >> entry->valid = false; >> mutex_unlock("e_lock); >> return; >> } >> _del_quote_entry(entry); >> mutex_unlock("e_lock); >> } >> >> >
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |