|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] mem_access: sanitize code around sending vm_event request
On 03/08/16 17:31, Tamas K Lengyel wrote:
> On Wed, Aug 3, 2016 at 10:10 AM, George Dunlap <george.dunlap@xxxxxxxxxx>
> wrote:
>> On 03/08/16 16:58, Tamas K Lengyel wrote:
>>> On Wed, Aug 3, 2016 at 9:44 AM, George Dunlap <george.dunlap@xxxxxxxxxx>
>>> wrote:
>>>> On 03/08/16 16:40, Tamas K Lengyel wrote:
>>>>> On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@xxxxxxxxxx>
>>>>> wrote:
>>>>>> On 03/08/16 16:18, Tamas K Lengyel wrote:
>>>>>>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap
>>>>>>> <george.dunlap@xxxxxxxxxx> wrote:
>>>>>>>> On 01/08/16 17:52, Tamas K Lengyel wrote:
>>>>>>>>> The two functions monitor_traps and mem_access_send_req duplicate
>>>>>>>>> some of the
>>>>>>>>> same functionality. The mem_access_send_req however leaves a lot of
>>>>>>>>> the
>>>>>>>>> standard vm_event fields to be filled by other functions.
>>>>>>>>>
>>>>>>>>> Remove mem_access_send_req() completely, making use of
>>>>>>>>> monitor_traps() to put
>>>>>>>>> requests into the monitor ring. This in turn causes some cleanup
>>>>>>>>> around the
>>>>>>>>> old callsites of mem_access_send_req(), and on ARM, the introduction
>>>>>>>>> of the
>>>>>>>>> __p2m_mem_access_send_req() helper to fill in common mem_access
>>>>>>>>> information.
>>>>>>>>> We also update monitor_traps to now include setting the common
>>>>>>>>> vcpu_id field
>>>>>>>>> so that all other call-sites can ommit this step.
>>>>>>>>>
>>>>>>>>> Finally, this change identifies that errors from
>>>>>>>>> mem_access_send_req() were
>>>>>>>>> never checked. As errors constitute a problem with the monitor ring,
>>>>>>>>> crashing the domain is the most appropriate action to take.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> This appears to be v3, not v2?
>>>>>>>
>>>>>>> No, it's still just v2.
>>>>>>>
>>>>>>>>
>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>>>>>> index 812dbf6..27f9d26 100644
>>>>>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>>>>>> @@ -1728,13 +1728,8 @@ bool_t p2m_mem_access_check(paddr_t gpa,
>>>>>>>>> unsigned long gla,
>>>>>>>>> if ( req )
>>>>>>>>> {
>>>>>>>>> *req_ptr = req;
>>>>>>>>> - req->reason = VM_EVENT_REASON_MEM_ACCESS;
>>>>>>>>> -
>>>>>>>>> - /* Pause the current VCPU */
>>>>>>>>> - if ( p2ma != p2m_access_n2rwx )
>>>>>>>>> - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>>>>>>>>>
>>>>>>>>> - /* Send request to mem event */
>>>>>>>>> + req->reason = VM_EVENT_REASON_MEM_ACCESS;
>>>>>>>>> req->u.mem_access.gfn = gfn;
>>>>>>>>> req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
>>>>>>>>> if ( npfec.gla_valid )
>>>>>>>>> @@ -1750,23 +1745,10 @@ bool_t p2m_mem_access_check(paddr_t gpa,
>>>>>>>>> unsigned long gla,
>>>>>>>>> req->u.mem_access.flags |= npfec.read_access ?
>>>>>>>>> MEM_ACCESS_R : 0;
>>>>>>>>> req->u.mem_access.flags |= npfec.write_access ?
>>>>>>>>> MEM_ACCESS_W : 0;
>>>>>>>>> req->u.mem_access.flags |= npfec.insn_fetch ?
>>>>>>>>> MEM_ACCESS_X : 0;
>>>>>>>>> - req->vcpu_id = v->vcpu_id;
>>>>>>>>> -
>>>>>>>>> - vm_event_fill_regs(req);
>>>>>>>>> -
>>>>>>>>> - if ( altp2m_active(v->domain) )
>>>>>>>>> - {
>>>>>>>>> - req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
>>>>>>>>> - req->altp2m_idx = vcpu_altp2m(v).p2midx;
>>>>>>>>> - }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> - /* Pause the current VCPU */
>>>>>>>>> - if ( p2ma != p2m_access_n2rwx )
>>>>>>>>> - vm_event_vcpu_pause(v);
>>>>>>>>> -
>>>>>>>>> - /* VCPU may be paused, return whether we promoted automatically
>>>>>>>>> */
>>>>>>>>> - return (p2ma == p2m_access_n2rwx);
>>>>>>>>> + /* Return whether vCPU pause is required (aka. sync event) */
>>>>>>>>> + return (p2ma != p2m_access_n2rwx);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static inline
>>>>>>>>
>>>>>>>> p2m-bits:
>>>>>>>>
>>>>>>>> Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> But I agree with Julien -- this patch has several independent changes
>>>>>>>> which makes it quite difficult to tell what's going on. I'm sure it's
>>>>>>>> taken the two of us a lot more time together to figure out what is and
>>>>>>>> is not happening than it would have for you to break it down into
>>>>>>>> several little chunks.
>>>>>>>>
>>>>>>>> If you're not already familiar with it, I would recommend looking into
>>>>>>>> stackgit. My modus operandi for things like this is to get things
>>>>>>>> working in one big patch, then pop it off the stack and apply bits of
>>>>>>>> it
>>>>>>>> at a time to make a series.
>>>>>>>>
>>>>>>>> It's not only more considerate of your reviewers, but it's also a
>>>>>>>> helpful exercise for yourself.
>>>>>>>>
>>>>>>>
>>>>>>> The extra work doesn't just come from splitting the code itself
>>>>>>> (although I don't know which bits would really make sense to split
>>>>>>> here that would worth the effort) but testing a series on various
>>>>>>> platforms.
>>>>>>
>>>>>> I don't understand this statement -- why is testing a 3-patch series
>>>>>> more difficult than testing a one-patch series? Are you testing each
>>>>>> individual patch?
>>>>>>
>>>>>
>>>>> Yes, I do. And when a patch touches multiple archs it adds up quite fast.
>>>>
>>>> Yes, I can imagine it does. :-)
>>>>
>>>> But the next question is, why do you feel the need to test every patch
>>>> of a series individually, rather than just testing the whole series?
>>>>
>>>
>>> Well, you never know when your series gets split up and have only bits
>>> of it merged. So having each patch tested individually is a necessity
>>> to ensure nothing gets broken midway through. Especially since
>>> mem_access/monitor/vm_event is not tested automatically in the Xen
>>> test system.
>>
>> I don't know anyone else who does that (more than perhaps
>> compile-testing), and I don't think anyone expects you to.
>>
>> Obviously *in general* you should try to avoid breaking things in the
>> middle of a series -- not primarily because it may be only partially
>> applied, but because it makes bisecting other issues more difficult.
>> But we generally rely on code review to catch things like that. And
>> that's also why we have the push gate.
>>
>> If the choice is between subtly broken patches that get checked in
>> because they were too complicated for reviewers to catch the errors, and
>> occasionally broken bisections because reviewers didn't notice a bug
>> introduced in one patch and fixed in a later patch, I'd much rather have
>> the latter.
>>
>> Or to say the same thing a different way: I would much rather have a
>> clean series where each patch wasn't tested (but the final patch was),
>> than to have a difficult-to-review patch.
>>
>
> I understand and that's fine. However, for our components we may
> prefer a different style of workflow on occasion as our definition of
> what constitutes difficult-to-review might be different. That's why
> moving it out from the common p2m codebase should help with avoiding
> this type of arguments in the future and also remove the burden of
> having to have maintainers of other components review it either though
> it doesn't touch anything outside of what we are maintaining.
Well you still need to get an ack from someone even if it only touches
code that you maintain. And in the bigger picture, breaking down your
patches into easy-to-review chunks makes it more likely for *you* to
catch mistakes before you send it, and also makes it more likely that
someone else will catch your mistakes before you check it in. Unless
you never make coding mistakes, I would think that you'd want to do
everything you could to make sure the code was as bug-free as possible,
particularly as you're developing a security-oriented product.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |