* George S. Coker, II (george.coker@xxxxxxxxx) wrote:
> >Thanks, I looked at that quickly, only comment I had there was it
> >could be:
> >
> >struct xsm_ops {
> >generic ones...
> >struct xsm_arch_ops arch_ops;
> >}
> >
> I guess I really don't see how this makes it any better since there
> will likely always be a mix of XSM "supported" ops and module specific
> ops. If you are trying to tighten down the interface, any command,
> support or specific could be abused. Please remind me what advantage
> you are thinking about here.
I think we are talking about 2 different things. I just mean the code
that is currently #ifdef CONFIG_X86 in xsm_operations structure. Typical
is to embed arch specific data within the toplevel generic structure.
It offers no benefit in code generation or tightening of the interface,
etc. It's simply a readability/maintainability thing to avoid:
struct xsm_operations{
generic bits
...
#ifdef CONFIG_X86
...
#endif
#ifdef CONFIG_IA64
...
#endif
#ifdef CONFIG_PPC
...
#endif
}
> >To avoid a bunch of ifdefs there. Some of them looked arch neutral
> >(like add_to_physmap), but I can see they aren't exactly.
> >
> >Still be nice to make the do_xsm_op hypercall be more than
> >direct pass-thru to module.
I think this is what you were referring to. The advantage only
comes about if there are things that are common to modules (likely)
and really shines if there are functions that also use common data
structures (unlikely) for type safety and general interface sanity.
> >Other than that, a quick review got me looking at evtchn calls.
> >
> >> diff -r e370c94bd6fd -r 62b752969edf xen/common/event_channel.c
> >> --- a/xen/common/event_channel.c Sat May 05 13:48:05 2007 +0100
> >> +++ b/xen/common/event_channel.c Mon May 07 14:50:16 2007 -0400
> >> @@ -30,6 +30,7 @@
> >> #include <public/xen.h>
> >> #include <public/event_channel.h>
> >> #include <acm/acm_hooks.h>
> >> +#include <xsm/xsm.h>
> >>
> >> #define bucket_from_port(d,p) \
> >> ((d)->evtchn[(p)/EVTCHNS_PER_BUCKET])
> >> @@ -89,8 +90,15 @@ static int get_free_port(struct domain *
> >> chn = xmalloc_array(struct evtchn, EVTCHNS_PER_BUCKET);
> >> if ( unlikely(chn == NULL) )
> >> return -ENOMEM;
> >> +
> >> memset(chn, 0, EVTCHNS_PER_BUCKET * sizeof(*chn));
> >> bucket_from_port(d, port) = chn;
> >> +
> >> + if ( xsm_alloc_security_evtchn(chn) )
> >> + {
> >> + xfree(chn);
> >> + return -ENOMEM;
> >> + }
> >
> >Oops, this is a domain triggerable use-after free bug-in-waiting.
> >Now the bucket is perceived valid, but the memory isn't.
> >
> >In fact, this is not the right interface. You are only allocating the
> >an opaque security blob per bucket, not per channel. In effect it's like:
> >
> >struct evtchn chn[EVTCHNS_PER_BUCKET];
> >xsm_alloc_security(&chn[0]);
> >
> >When I believe you want smth effectively like:
> >
> >struct evtchn chn[EVTCHNS_PER_BUCKET];
> >for (i=0; i < EVTCHNS_PER_BUCKET; i++)
> > xsm_alloc_security(&chn[i]);
> >
> >> return port;
> >> }
>
> This is a mistake in the framework because the behavior between alloc
> and free is inconsistent. The Flask module does this correctly, but
> this is a gotcha for a new module. I'll fix this and repost tomorrow.
Actually Flask got it wrong as well (you probably saw that email later).
I agree, the framework is inconsistent, and I think it makes most sense
to have the alloc/free operate on a single channel. You may need
to introduce a simple helper that does the loop and handles the error
condition in the framework which is fine.
> >> @@ -120,6 +128,10 @@ static long evtchn_alloc_unbound(evtchn_
> >> if ( (port = get_free_port(d)) < 0 )
> >> ERROR_EXIT(port);
> >> chn = evtchn_from_port(d, port);
> >> +
> >> + rc = xsm_evtchn_unbound(d, chn, alloc->remote_dom);
> >> + if ( rc )
> >> + goto out;
> >>
> >> chn->state = ECS_UNBOUND;
> >> if ( (chn->u.unbound.remote_domid = alloc->remote_dom) ==
> >DOMID_SELF )
> >> @@ -176,6 +188,10 @@ static long evtchn_bind_interdomain(evtc
> >> if ( (rchn->state != ECS_UNBOUND) ||
> >> (rchn->u.unbound.remote_domid != ld->domain_id) )
> >> ERROR_EXIT(-EINVAL);
> >> +
> >> + rc = xsm_evtchn_interdomain(ld, lchn, rd, rchn);
> >> + if ( rc )
> >> + goto out;
> >>
> >> lchn->u.interdomain.remote_dom = rd;
> >> lchn->u.interdomain.remote_port = (u16)rport;
> >> @@ -231,6 +247,7 @@ static long evtchn_bind_virq(evtchn_bind
> >> ERROR_EXIT(port);
> >>
> >> chn = evtchn_from_port(d, port);
> >> +
> >> chn->state = ECS_VIRQ;
> >> chn->notify_vcpu_id = vcpu;
> >> chn->u.virq = virq;
> >> @@ -261,14 +278,15 @@ static long evtchn_bind_ipi(evtchn_bind_
> >> ERROR_EXIT(port);
> >>
> >> chn = evtchn_from_port(d, port);
> >> +
> >> chn->state = ECS_IPI;
> >> chn->notify_vcpu_id = vcpu;
> >>
> >> bind->port = port;
> >>
> >> + spin_unlock(&d->evtchn_lock);
> >> +
> >> out:
> >> - spin_unlock(&d->evtchn_lock);
> >> -
> >
> >This is incorrect, leaves domain locked on error path (yes, ERROR_EXIT
> >is mean macro abuse!).
> >
> Absolutely this is a problem, when did ERROR_EXIT come about?
It's been there a long time. I think the issue is about your followup.
Given a couple gratuitous newlines, and this one, etc. it looks like
there's just a small bit of leftover cruft in the diff.
thanks,
-chris
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home