On 01/02/2021 13:03, Jan Beulich wrote: > On 01.02.2021 12:11, Andrew Cooper wrote: >> On 01/02/2021 10:10, Roger Pau Monné wrote: >>> On Sat, Jan 30, 2021 at 02:58:42AM +0000, Andrew Cooper wrote: >>>> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd, >>>> XEN_GUEST_HANDLE_PARAM(void) compat) >>>> compat_frame_list[i] = frame; >>>> } >>>> >>>> - if ( __copy_to_compat_offset(cmp.mar.frame_list, 0, >>>> + if ( __copy_to_compat_offset(cmp.mar.frame_list, >>>> start_extent, >>>> compat_frame_list, >>>> - cmp.mar.nr_frames) ) >>>> + done) ) >>>> return -EFAULT; >>> Is it fine to return with a possibly pending continuation already >>> encoded here? >>> >>> Other places seem to crash the domain when that happens. >> Hmm. This is all a total mess. (Elsewhere the error handling is also >> broken - a caller who receives an error can't figure out how to recover) >> >> But yes - I think you're right - the only thing we can do here is `goto >> crash;` and woe betide any 32bit kernel which passes a pointer to a >> read-only buffer. > I'd like to ask you to reconsider the "goto crash", both the one > you mention above and the other one already present in the patch. > Wiring all the cases where we mean to crash the guest into a > single domain_crash() invocation has the downside that when > observing such a case one can't remotely know which path has led > there. Therefore I'd like to suggest individual domain_crash() > invocations on every affected path. Elsewhere in the file there > already is such an instance, commented "Cannot cancel the > continuation...".
But they're all logically the same, are they not? ~Andrew
