[PATCH] evtchn/Flask: pre-allocate node on send path

[PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

xmalloc() & Co may not be called with IRQs off, or else check_lock()
will have its assertion trigger about locks getting acquired
inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
very reasonable, especially since the per-channel lock was introduced to
avoid acquiring the per-domain event lock on the send paths. Issue a
second call to xsm_evtchn_send() instead, before acquiring the lock, to
give XSM / Flask a chance to pre-allocate whatever it may need.

As these nodes are used merely for caching earlier decisions' results,
allocate just one node in AVC code despite two potentially being needed.
Things will merely be not as performant if a second allocation was
wanted, just like when the pre-allocation fails.

Fixes: c0ddc8634845 ("evtchn: convert per-channel lock to be IRQ-safe")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: An even easier fix could be to simply guard xzalloc() by a
     conditional checking local_irq_is_enabled(), but for a domain
     sending only interdomain events this would mean AVC's node caching
     would never take effect on the sending path, as allocation would
     then always be avoided.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -723,6 +723,12 @@ int evtchn_send(struct domain *ld, unsig
     if ( !port_is_valid(ld, lport) )
         return -EINVAL;
 
+    /*
+     * As the call further down needs to avoid allocations (due to running
+     * with IRQs off), give XSM a chance to pre-allocate if needed.
+     */
+    xsm_evtchn_send(XSM_HOOK, ld, NULL);
+
     lchn = evtchn_from_port(ld, lport);
 
     spin_lock_irqsave(&lchn->lock, flags);
--- a/xen/xsm/flask/avc.c
+++ b/xen/xsm/flask/avc.c
@@ -24,7 +24,9 @@
 #include <xen/prefetch.h>
 #include <xen/kernel.h>
 #include <xen/sched.h>
+#include <xen/cpu.h>
 #include <xen/init.h>
+#include <xen/percpu.h>
 #include <xen/rcupdate.h>
 #include <asm/atomic.h>
 #include <asm/current.h>
@@ -341,17 +343,79 @@ static inline int avc_reclaim_node(void)
     return ecx;
 }
 
+static struct avc_node *new_node(void)
+{
+    struct avc_node *node = xzalloc(struct avc_node);
+
+    if ( node )
+    {
+        INIT_RCU_HEAD(&node->rhead);
+        INIT_HLIST_NODE(&node->list);
+        avc_cache_stats_incr(allocations);
+    }
+
+    return node;
+}
+
+/*
+ * avc_has_perm_noaudit() may consume up to two nodes, which we may not be
+ * able to obtain from the allocator at that point. Since the is merely
+ * about caching earlier decisions, allow for (just) one pre-allocated node.
+ */
+static DEFINE_PER_CPU(struct avc_node *, prealloc_node);
+
+void avc_prealloc(void)
+{
+    struct avc_node **prealloc = &this_cpu(prealloc_node);
+
+    if ( !*prealloc )
+        *prealloc = new_node();
+}
+
+static int cpu_callback(struct notifier_block *nfb, unsigned long action,
+                        void *hcpu)
+{
+    unsigned int cpu = (unsigned long)hcpu;
+    struct avc_node **prealloc = &per_cpu(prealloc_node, cpu);
+
+    if ( action == CPU_DEAD && *prealloc )
+    {
+        xfree(*prealloc);
+        *prealloc = NULL;
+        avc_cache_stats_incr(frees);
+    }
+
+    return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_nfb = {
+    .notifier_call = cpu_callback,
+    .priority = 99
+};
+
+static int __init cpu_nfb_init(void)
+{
+    register_cpu_notifier(&cpu_nfb);
+    return 0;
+}
+__initcall(cpu_nfb_init);
+
 static struct avc_node *avc_alloc_node(void)
 {
-    struct avc_node *node;
+    struct avc_node *node, **prealloc = &this_cpu(prealloc_node);
 
-    node = xzalloc(struct avc_node);
-    if (!node)
-        goto out;
-
-    INIT_RCU_HEAD(&node->rhead);
-    INIT_HLIST_NODE(&node->list);
-    avc_cache_stats_incr(allocations);
+    node = *prealloc;
+    *prealloc = NULL;
+
+    if ( !node )
+    {
+        /* Must not call xmalloc() & Co with IRQs off. */
+        if ( !local_irq_is_enabled() )
+            goto out;
+        node = new_node();
+        if ( !node )
+            goto out;
+    }
 
     atomic_inc(&avc_cache.active_nodes);
     if ( atomic_read(&avc_cache.active_nodes) > avc_cache_threshold )
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -281,6 +281,16 @@ static int flask_evtchn_send(struct doma
 {
     int rc;
 
+    /*
+     * When called with non-NULL chn, memory allocation may not be permitted.
+     * Allow AVC to preallocate nodes as necessary upon early notification.
+     */
+    if ( !chn )
+    {
+        avc_prealloc();
+        return 0;
+    }
+
     switch ( chn->state )
     {
     case ECS_INTERDOMAIN:
--- a/xen/xsm/flask/include/avc.h
+++ b/xen/xsm/flask/include/avc.h
@@ -91,6 +91,8 @@ int avc_has_perm_noaudit(u32 ssid, u32 t
 int avc_has_perm(u32 ssid, u32 tsid, u16 tclass, u32 requested,
                                              struct avc_audit_data *auditdata);
 
+void avc_prealloc(void);
+
 /* Exported to selinuxfs */
 struct xen_flask_hash_stats;
 int avc_get_hash_stats(struct xen_flask_hash_stats *arg);

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Julien Grall]

Hi Jan,

On 24/09/2020 11:53, Jan Beulich wrote:
> xmalloc() & Co may not be called with IRQs off, or else check_lock()
> will have its assertion trigger about locks getting acquired
> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
> very reasonable, especially since the per-channel lock was introduced to
> avoid acquiring the per-domain event lock on the send paths. Issue a
> second call to xsm_evtchn_send() instead, before acquiring the lock, to
> give XSM / Flask a chance to pre-allocate whatever it may need.

This is the sort of fall-out I was expecting when we decide to turn off 
the interrupts for big chunk of code. I couldn't find any at the time 
though...

Can you remind which caller of send_guest{global, vcpu}_virq() will call 
them with interrupts off?

Would it be possible to consider deferring the call to a softirq 
taslket? If so, this would allow us to turn the interrupts again.

Cheers,

-- 
Julien Grall

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 25.09.2020 12:34, Julien Grall wrote:
> On 24/09/2020 11:53, Jan Beulich wrote:
>> xmalloc() & Co may not be called with IRQs off, or else check_lock()
>> will have its assertion trigger about locks getting acquired
>> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
>> very reasonable, especially since the per-channel lock was introduced to
>> avoid acquiring the per-domain event lock on the send paths. Issue a
>> second call to xsm_evtchn_send() instead, before acquiring the lock, to
>> give XSM / Flask a chance to pre-allocate whatever it may need.
> 
> This is the sort of fall-out I was expecting when we decide to turn off 
> the interrupts for big chunk of code. I couldn't find any at the time 
> though...
> 
> Can you remind which caller of send_guest{global, vcpu}_virq() will call 
> them with interrupts off?

I don't recall which one of the two it was that I hit; we wanted
both to use the lock anyway. send_guest_pirq() very clearly also
gets called with IRQs off.

> Would it be possible to consider deferring the call to a softirq 
> taslket? If so, this would allow us to turn the interrupts again.

Of course this is in principle possible; the question is how
involved this is going to get. However, on x86 oprofile's call to
send_guest_vcpu_virq() can't easily be replaced - it's dangerous
enough already that in involves locks in NMI context. I don't
fancy seeing it use more commonly used ones.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Julien Grall]

Hi Jan,

On 25/09/2020 13:21, Jan Beulich wrote:
> On 25.09.2020 12:34, Julien Grall wrote:
>> On 24/09/2020 11:53, Jan Beulich wrote:
>>> xmalloc() & Co may not be called with IRQs off, or else check_lock()
>>> will have its assertion trigger about locks getting acquired
>>> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
>>> very reasonable, especially since the per-channel lock was introduced to
>>> avoid acquiring the per-domain event lock on the send paths. Issue a
>>> second call to xsm_evtchn_send() instead, before acquiring the lock, to
>>> give XSM / Flask a chance to pre-allocate whatever it may need.
>>
>> This is the sort of fall-out I was expecting when we decide to turn off
>> the interrupts for big chunk of code. I couldn't find any at the time
>> though...
>>
>> Can you remind which caller of send_guest{global, vcpu}_virq() will call
>> them with interrupts off?
> 
> I don't recall which one of the two it was that I hit; we wanted
> both to use the lock anyway. send_guest_pirq() very clearly also
> gets called with IRQs off.
> 
>> Would it be possible to consider deferring the call to a softirq
>> taslket? If so, this would allow us to turn the interrupts again.
> 
> Of course this is in principle possible; the question is how
> involved this is going to get. 
> However, on x86 oprofile's call to
> send_guest_vcpu_virq() can't easily be replaced - it's dangerous
> enough already that in involves locks in NMI context. I don't
> fancy seeing it use more commonly used ones.

Fair enough. I would still like to consider a way where we could avoid 
to hack xsm_* because we have the interrupts disabled.

AFAICT, we don't need send_guest_global_virq() and evtchn_send() to be 
mutually exclusive. Is that correct?

So how about splitting the lock in two? One would be used when the 
interrupts have to be disabled while the other would be used when we can 
keep interrupts enabled.

The two locks would have to be taken when the event channel needs to be 
modified.

Cheers,

-- 
Julien Grall

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 25.09.2020 15:16, Julien Grall wrote:
> Hi Jan,
> 
> On 25/09/2020 13:21, Jan Beulich wrote:
>> On 25.09.2020 12:34, Julien Grall wrote:
>>> On 24/09/2020 11:53, Jan Beulich wrote:
>>>> xmalloc() & Co may not be called with IRQs off, or else check_lock()
>>>> will have its assertion trigger about locks getting acquired
>>>> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
>>>> very reasonable, especially since the per-channel lock was introduced to
>>>> avoid acquiring the per-domain event lock on the send paths. Issue a
>>>> second call to xsm_evtchn_send() instead, before acquiring the lock, to
>>>> give XSM / Flask a chance to pre-allocate whatever it may need.
>>>
>>> This is the sort of fall-out I was expecting when we decide to turn off
>>> the interrupts for big chunk of code. I couldn't find any at the time
>>> though...
>>>
>>> Can you remind which caller of send_guest{global, vcpu}_virq() will call
>>> them with interrupts off?
>>
>> I don't recall which one of the two it was that I hit; we wanted
>> both to use the lock anyway. send_guest_pirq() very clearly also
>> gets called with IRQs off.
>>
>>> Would it be possible to consider deferring the call to a softirq
>>> taslket? If so, this would allow us to turn the interrupts again.
>>
>> Of course this is in principle possible; the question is how
>> involved this is going to get. 
>> However, on x86 oprofile's call to
>> send_guest_vcpu_virq() can't easily be replaced - it's dangerous
>> enough already that in involves locks in NMI context. I don't
>> fancy seeing it use more commonly used ones.
> 
> Fair enough. I would still like to consider a way where we could avoid 
> to hack xsm_* because we have the interrupts disabled.

Well, from a conceptual pov it's at least questionable for XSM to
need any memory allocations at all when merely being asked for
permission. And indeed the need for it arises solely from its
desire to cache the result, for the sake of subsequent lookups.

I also find it odd that there's an XSM check on the send path in
the first place. This isn't just because it would seem to me that
it should be decided at binding time whether sending is permitted
- I may easily be missing something in the conceptual model here.
It's also because __domain_finalise_shutdown() too uses
evtchn_send(), and I didn't think this one should be subject to
any XSM check (just like send_guest_*() aren't).

> AFAICT, we don't need send_guest_global_virq() and evtchn_send() to be 
> mutually exclusive. Is that correct?

Yes, any number of sends (even to the same port) could in principle
run concurrently, I think. Or else the FIFO code would have been
broken from the very point where the per-channel lock was
introduced and acquiring of the per-domain one then dropped from
evtchn_send() (other sending paths weren't using the per-domain one
anyway already before that).

> So how about splitting the lock in two? One would be used when the 
> interrupts have to be disabled while the other would be used when we can 
> keep interrupts enabled.

Now that's an interesting proposal. I thought one lock per channel
was already pretty fine-grained. Now you propose making it two.

> The two locks would have to be taken when the event channel needs to be 
> modified.

Requiring a total of 6 locks to be acquired when fiddling with
interdomain channels... Wow. Definitely more intrusive overall than
the change here.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Julien Grall]

Hi Jan,

On 25/09/2020 14:58, Jan Beulich wrote:
> On 25.09.2020 15:16, Julien Grall wrote:
>> Hi Jan,
>>
>> On 25/09/2020 13:21, Jan Beulich wrote:
>>> On 25.09.2020 12:34, Julien Grall wrote:
>>>> On 24/09/2020 11:53, Jan Beulich wrote:
>>>>> xmalloc() & Co may not be called with IRQs off, or else check_lock()
>>>>> will have its assertion trigger about locks getting acquired
>>>>> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
>>>>> very reasonable, especially since the per-channel lock was introduced to
>>>>> avoid acquiring the per-domain event lock on the send paths. Issue a
>>>>> second call to xsm_evtchn_send() instead, before acquiring the lock, to
>>>>> give XSM / Flask a chance to pre-allocate whatever it may need.
>>>>
>>>> This is the sort of fall-out I was expecting when we decide to turn off
>>>> the interrupts for big chunk of code. I couldn't find any at the time
>>>> though...
>>>>
>>>> Can you remind which caller of send_guest{global, vcpu}_virq() will call
>>>> them with interrupts off?
>>>
>>> I don't recall which one of the two it was that I hit; we wanted
>>> both to use the lock anyway. send_guest_pirq() very clearly also
>>> gets called with IRQs off.
>>>
>>>> Would it be possible to consider deferring the call to a softirq
>>>> taslket? If so, this would allow us to turn the interrupts again.
>>>
>>> Of course this is in principle possible; the question is how
>>> involved this is going to get.
>>> However, on x86 oprofile's call to
>>> send_guest_vcpu_virq() can't easily be replaced - it's dangerous
>>> enough already that in involves locks in NMI context. I don't
>>> fancy seeing it use more commonly used ones.
>>
>> Fair enough. I would still like to consider a way where we could avoid
>> to hack xsm_* because we have the interrupts disabled.
> 
> Well, from a conceptual pov it's at least questionable for XSM to
> need any memory allocations at all when merely being asked for
> permission. And indeed the need for it arises solely from its
> desire to cache the result, for the sake of subsequent lookups.
> 
> I also find it odd that there's an XSM check on the send path in
> the first place. This isn't just because it would seem to me that
> it should be decided at binding time whether sending is permitted
> - I may easily be missing something in the conceptual model here.
> It's also because __domain_finalise_shutdown() too uses
> evtchn_send(), and I didn't think this one should be subject to
> any XSM check (just like send_guest_*() aren't).

Maybe this is the first question we need to answer?

> 
>> AFAICT, we don't need send_guest_global_virq() and evtchn_send() to be
>> mutually exclusive. Is that correct?
> 
> Yes, any number of sends (even to the same port) could in principle
> run concurrently, I think. Or else the FIFO code would have been
> broken from the very point where the per-channel lock was
> introduced and acquiring of the per-domain one then dropped from
> evtchn_send() (other sending paths weren't using the per-domain one
> anyway already before that).
> 
>> So how about splitting the lock in two? One would be used when the
>> interrupts have to be disabled while the other would be used when we can
>> keep interrupts enabled.
> 
> Now that's an interesting proposal. I thought one lock per channel
> was already pretty fine-grained. Now you propose making it two.
> 
>> The two locks would have to be taken when the event channel needs to be
>> modified.
> 
> Requiring a total of 6 locks to be acquired when fiddling with
> interdomain channels... Wow. Definitely more intrusive overall than
> the change here.

Well hacks are always more self-contained but long term they are a 
nightmare to maintain :).

6 locks is indeed a lot, but the discussion here shows that the current 
locking is probably not suitable for the current uses.

I don't have a better solution so far, but I will have a think about it. 
Meanwhile, it would be good to figure out why xsm_evtchn_send() is needed.

Cheers,

-- 
Julien Grall

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jürgen Groß]

On 25.09.20 14:21, Jan Beulich wrote:
> On 25.09.2020 12:34, Julien Grall wrote:
>> On 24/09/2020 11:53, Jan Beulich wrote:
>>> xmalloc() & Co may not be called with IRQs off, or else check_lock()
>>> will have its assertion trigger about locks getting acquired
>>> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
>>> very reasonable, especially since the per-channel lock was introduced to
>>> avoid acquiring the per-domain event lock on the send paths. Issue a
>>> second call to xsm_evtchn_send() instead, before acquiring the lock, to
>>> give XSM / Flask a chance to pre-allocate whatever it may need.
>>
>> This is the sort of fall-out I was expecting when we decide to turn off
>> the interrupts for big chunk of code. I couldn't find any at the time
>> though...
>>
>> Can you remind which caller of send_guest{global, vcpu}_virq() will call
>> them with interrupts off?
> 
> I don't recall which one of the two it was that I hit; we wanted
> both to use the lock anyway. send_guest_pirq() very clearly also
> gets called with IRQs off.
> 
>> Would it be possible to consider deferring the call to a softirq
>> taslket? If so, this would allow us to turn the interrupts again.
> 
> Of course this is in principle possible; the question is how
> involved this is going to get. However, on x86 oprofile's call to
> send_guest_vcpu_virq() can't easily be replaced - it's dangerous
> enough already that in involves locks in NMI context. I don't
> fancy seeing it use more commonly used ones.

Is it really so hard to avoid calling send_guest_vcpu_virq() in NMI
context?

Today it is called only if the NMI happened inside the guest, so the
main Xen stack is unused at this time. It should be rather straight
forward to mimic a stack frame on the main stack and iret to a special
handler from NMI context. This handler would then call
send_guest_vcpu_virq() and return to the guest.

This would basically be similar to the Linux kernel's
__run_on_irqstack().


Juergen

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 25.09.2020 16:57, Jürgen Groß wrote:
> On 25.09.20 14:21, Jan Beulich wrote:
>> On 25.09.2020 12:34, Julien Grall wrote:
>>> On 24/09/2020 11:53, Jan Beulich wrote:
>>>> xmalloc() & Co may not be called with IRQs off, or else check_lock()
>>>> will have its assertion trigger about locks getting acquired
>>>> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
>>>> very reasonable, especially since the per-channel lock was introduced to
>>>> avoid acquiring the per-domain event lock on the send paths. Issue a
>>>> second call to xsm_evtchn_send() instead, before acquiring the lock, to
>>>> give XSM / Flask a chance to pre-allocate whatever it may need.
>>>
>>> This is the sort of fall-out I was expecting when we decide to turn off
>>> the interrupts for big chunk of code. I couldn't find any at the time
>>> though...
>>>
>>> Can you remind which caller of send_guest{global, vcpu}_virq() will call
>>> them with interrupts off?
>>
>> I don't recall which one of the two it was that I hit; we wanted
>> both to use the lock anyway. send_guest_pirq() very clearly also
>> gets called with IRQs off.
>>
>>> Would it be possible to consider deferring the call to a softirq
>>> taslket? If so, this would allow us to turn the interrupts again.
>>
>> Of course this is in principle possible; the question is how
>> involved this is going to get. However, on x86 oprofile's call to
>> send_guest_vcpu_virq() can't easily be replaced - it's dangerous
>> enough already that in involves locks in NMI context. I don't
>> fancy seeing it use more commonly used ones.
> 
> Is it really so hard to avoid calling send_guest_vcpu_virq() in NMI
> context?
> 
> Today it is called only if the NMI happened inside the guest, so the
> main Xen stack is unused at this time. It should be rather straight
> forward to mimic a stack frame on the main stack and iret to a special
> handler from NMI context. This handler would then call
> send_guest_vcpu_virq() and return to the guest.

Quite possible that it's not overly difficult to arrange for. But
even with this out of the way I don't really view this softirq
tasklet route as viable; I could be proven wrong by demonstrating
that it's sufficiently straightforward.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 25.09.2020 16:33, Julien Grall wrote:
> On 25/09/2020 14:58, Jan Beulich wrote:
>> On 25.09.2020 15:16, Julien Grall wrote:
>>> Fair enough. I would still like to consider a way where we could avoid
>>> to hack xsm_* because we have the interrupts disabled.
>>
>> Well, from a conceptual pov it's at least questionable for XSM to
>> need any memory allocations at all when merely being asked for
>> permission. And indeed the need for it arises solely from its
>> desire to cache the result, for the sake of subsequent lookups.
>>
>> I also find it odd that there's an XSM check on the send path in
>> the first place. This isn't just because it would seem to me that
>> it should be decided at binding time whether sending is permitted
>> - I may easily be missing something in the conceptual model here.
>> It's also because __domain_finalise_shutdown() too uses
>> evtchn_send(), and I didn't think this one should be subject to
>> any XSM check (just like send_guest_*() aren't).
> 
> Maybe this is the first question we need to answer?

Indeed that was the question I asked myself before putting together
the patch. Yet I have no idea who could answer it, with Daniel
having gone silent for quite long a time now. Hence I didn't even
put up the question, but instead tried to find a halfway reasonable
solution. After all it's not just the master branch which is stuck
right now. And from a backporting perspective having the "fix"
touch code which hasn't had much churn over the last years may even
be beneficial. Plus, as said, the minimal change of making Flask
avoid xmalloc() when IRQs are off is a change that ought to be made
anyway imo, in order to favor proper functioning over performance.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Julien Grall]

Hi Jan,

On 25/09/2020 16:36, Jan Beulich wrote:
> On 25.09.2020 16:33, Julien Grall wrote:
>> On 25/09/2020 14:58, Jan Beulich wrote:
>>> On 25.09.2020 15:16, Julien Grall wrote:
>>>> Fair enough. I would still like to consider a way where we could avoid
>>>> to hack xsm_* because we have the interrupts disabled.
>>>
>>> Well, from a conceptual pov it's at least questionable for XSM to
>>> need any memory allocations at all when merely being asked for
>>> permission. And indeed the need for it arises solely from its
>>> desire to cache the result, for the sake of subsequent lookups.
>>>
>>> I also find it odd that there's an XSM check on the send path in
>>> the first place. This isn't just because it would seem to me that
>>> it should be decided at binding time whether sending is permitted
>>> - I may easily be missing something in the conceptual model here.
>>> It's also because __domain_finalise_shutdown() too uses
>>> evtchn_send(), and I didn't think this one should be subject to
>>> any XSM check (just like send_guest_*() aren't).
>>
>> Maybe this is the first question we need to answer?
> 
> Indeed that was the question I asked myself before putting together
> the patch. Yet I have no idea who could answer it, with Daniel
> having gone silent for quite long a time now. Hence I didn't even
> put up the question, but instead tried to find a halfway reasonable
> solution. 

IIRC, XSM is used by OpenXT and QubeOS. So I have CCed them to get some 
input.

> After all it's not just the master branch which is stuck
> right now.

> And from a backporting perspective having the "fix"
> touch code which hasn't had much churn over the last years may even
> be beneficial.

Right, but dropping xsm_evtchn_send() (if it is not possible) is going 
to be even better. So lets explore this solution first.

> Plus, as said, the minimal change of making Flask
> avoid xmalloc() when IRQs are off is a change that ought to be made
> anyway imo, in order to favor proper functioning over performance.
If there are other use, then yes. What I don't like in the current 
approach is we are hijacking xsm_event_send() for pre-allocating memory.

Cheers,

-- 
Julien Grall

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 25.09.2020 18:00, Julien Grall wrote:
> On 25/09/2020 16:36, Jan Beulich wrote:
>> Plus, as said, the minimal change of making Flask
>> avoid xmalloc() when IRQs are off is a change that ought to be made
>> anyway imo, in order to favor proper functioning over performance.
> If there are other use, then yes. What I don't like in the current 
> approach is we are hijacking xsm_event_send() for pre-allocating memory.

I first had a designated function for this, but the need to wire
this through ops, dummy.h, etc made me pretty quickly turn to this
lower churn variant. But that other one hasn't been ruled out if
the "hijacking" is deemed too bad.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jason Andryuk]

On Fri, Sep 25, 2020 at 12:02 PM Julien Grall <julien@xen.org> wrote:
>
> Hi Jan,
>
> On 25/09/2020 16:36, Jan Beulich wrote:
> > On 25.09.2020 16:33, Julien Grall wrote:
> >> On 25/09/2020 14:58, Jan Beulich wrote:
> >>> On 25.09.2020 15:16, Julien Grall wrote:
> >>>> Fair enough. I would still like to consider a way where we could avoid
> >>>> to hack xsm_* because we have the interrupts disabled.
> >>>
> >>> Well, from a conceptual pov it's at least questionable for XSM to
> >>> need any memory allocations at all when merely being asked for
> >>> permission. And indeed the need for it arises solely from its
> >>> desire to cache the result, for the sake of subsequent lookups.
> >>>
> >>> I also find it odd that there's an XSM check on the send path in
> >>> the first place. This isn't just because it would seem to me that
> >>> it should be decided at binding time whether sending is permitted
> >>> - I may easily be missing something in the conceptual model here.
> >>> It's also because __domain_finalise_shutdown() too uses
> >>> evtchn_send(), and I didn't think this one should be subject to
> >>> any XSM check (just like send_guest_*() aren't).
> >>
> >> Maybe this is the first question we need to answer?
> >
> > Indeed that was the question I asked myself before putting together
> > the patch. Yet I have no idea who could answer it, with Daniel
> > having gone silent for quite long a time now. Hence I didn't even
> > put up the question, but instead tried to find a halfway reasonable
> > solution.
>
> IIRC, XSM is used by OpenXT and QubeOS. So I have CCed them to get some
> input.

I think the send hook exists because send is its own distinct
operation.  While most common usage could be handled by just checking
at bind time, the send hoor provides more flexibility.  For instance,
the send hook can be used to restrict signalling to only one
direction.  Also, a domain label can transition (change) at runtime.
Dropping the send check would latch the permission at bind time which
would not necessarily be valid for the security policy.

Am I correct that the assertion mentioned in the patch description
would only be seen in debug builds?

Thanks,
Jason

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 25.09.2020 20:08, Jason Andryuk wrote:
> On Fri, Sep 25, 2020 at 12:02 PM Julien Grall <julien@xen.org> wrote:
>>
>> Hi Jan,
>>
>> On 25/09/2020 16:36, Jan Beulich wrote:
>>> On 25.09.2020 16:33, Julien Grall wrote:
>>>> On 25/09/2020 14:58, Jan Beulich wrote:
>>>>> On 25.09.2020 15:16, Julien Grall wrote:
>>>>>> Fair enough. I would still like to consider a way where we could avoid
>>>>>> to hack xsm_* because we have the interrupts disabled.
>>>>>
>>>>> Well, from a conceptual pov it's at least questionable for XSM to
>>>>> need any memory allocations at all when merely being asked for
>>>>> permission. And indeed the need for it arises solely from its
>>>>> desire to cache the result, for the sake of subsequent lookups.
>>>>>
>>>>> I also find it odd that there's an XSM check on the send path in
>>>>> the first place. This isn't just because it would seem to me that
>>>>> it should be decided at binding time whether sending is permitted
>>>>> - I may easily be missing something in the conceptual model here.
>>>>> It's also because __domain_finalise_shutdown() too uses
>>>>> evtchn_send(), and I didn't think this one should be subject to
>>>>> any XSM check (just like send_guest_*() aren't).
>>>>
>>>> Maybe this is the first question we need to answer?
>>>
>>> Indeed that was the question I asked myself before putting together
>>> the patch. Yet I have no idea who could answer it, with Daniel
>>> having gone silent for quite long a time now. Hence I didn't even
>>> put up the question, but instead tried to find a halfway reasonable
>>> solution.
>>
>> IIRC, XSM is used by OpenXT and QubeOS. So I have CCed them to get some
>> input.
> 
> I think the send hook exists because send is its own distinct
> operation.  While most common usage could be handled by just checking
> at bind time, the send hoor provides more flexibility.  For instance,
> the send hook can be used to restrict signalling to only one
> direction.

I did realize this is a special case, but it could still be dealt
with at binding time, via a boolean in struct evtchn.

>  Also, a domain label can transition (change) at runtime.
> Dropping the send check would latch the permission at bind time which
> would not necessarily be valid for the security policy.

I did realize this as a possibility too, but there the immediate
question is: Why for interdomain channels, but then not also for
vIRQ-s, for example? In fact, unless I'm overlooking something,
for this specific case there's not even any check in the binding
logic, not even for global vIRQ-s. (After all there are two
aspects in the permissions here: One is to be eligible to send,
which ought to not matter when the sender is Xen, while the
other is permission to learn / know of certain events, i.e. in
particular global vIRQ-s.)

The fundamental issue here is that the sending path should be
fast and hence lightweight. This means (to me) that in
particular no memory allocations should occur, and (more
generally) no global or domain wide locks should be taken (for
rw ones: no write locks).

> Am I correct that the assertion mentioned in the patch description
> would only be seen in debug builds?

Yes. Release builds would instead have deadlock potential, which
may get realized at any time (or never, if you're really lucky).
Catching such cases early, and in an easy to recognize manner,
is - after all - what the extra checking in debug builds is for.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jason Andryuk]

On Mon, Sep 28, 2020 at 3:49 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.09.2020 20:08, Jason Andryuk wrote:
> > On Fri, Sep 25, 2020 at 12:02 PM Julien Grall <julien@xen.org> wrote:
> >>
> >> Hi Jan,
> >>
> >> On 25/09/2020 16:36, Jan Beulich wrote:
> >>> On 25.09.2020 16:33, Julien Grall wrote:
> >>>> On 25/09/2020 14:58, Jan Beulich wrote:
> >>>>> On 25.09.2020 15:16, Julien Grall wrote:
> >>>>>> Fair enough. I would still like to consider a way where we could avoid
> >>>>>> to hack xsm_* because we have the interrupts disabled.
> >>>>>
> >>>>> Well, from a conceptual pov it's at least questionable for XSM to
> >>>>> need any memory allocations at all when merely being asked for
> >>>>> permission. And indeed the need for it arises solely from its
> >>>>> desire to cache the result, for the sake of subsequent lookups.
> >>>>>
> >>>>> I also find it odd that there's an XSM check on the send path in
> >>>>> the first place. This isn't just because it would seem to me that
> >>>>> it should be decided at binding time whether sending is permitted
> >>>>> - I may easily be missing something in the conceptual model here.
> >>>>> It's also because __domain_finalise_shutdown() too uses
> >>>>> evtchn_send(), and I didn't think this one should be subject to
> >>>>> any XSM check (just like send_guest_*() aren't).
> >>>>
> >>>> Maybe this is the first question we need to answer?
> >>>
> >>> Indeed that was the question I asked myself before putting together
> >>> the patch. Yet I have no idea who could answer it, with Daniel
> >>> having gone silent for quite long a time now. Hence I didn't even
> >>> put up the question, but instead tried to find a halfway reasonable
> >>> solution.
> >>
> >> IIRC, XSM is used by OpenXT and QubeOS. So I have CCed them to get some
> >> input.
> >
> > I think the send hook exists because send is its own distinct
> > operation.  While most common usage could be handled by just checking
> > at bind time, the send hoor provides more flexibility.  For instance,
> > the send hook can be used to restrict signalling to only one
> > direction.
>
> I did realize this is a special case, but it could still be dealt
> with at binding time, via a boolean in struct evtchn.
>
> >  Also, a domain label can transition (change) at runtime.
> > Dropping the send check would latch the permission at bind time which
> > would not necessarily be valid for the security policy.
>
> I did realize this as a possibility too, but there the immediate
> question is: Why for interdomain channels, but then not also for
> vIRQ-s, for example? In fact, unless I'm overlooking something,
> for this specific case there's not even any check in the binding
> logic, not even for global vIRQ-s. (After all there are two
> aspects in the permissions here: One is to be eligible to send,
> which ought to not matter when the sender is Xen, while the
> other is permission to learn / know of certain events, i.e. in
> particular global vIRQ-s.)

I'm not familiar with vIRQ-s, but I did a little bit of review.  A
vIRQ source is always Xen and its destination is a domain, correct?
They don't allow a data flow between domains, so maybe that is why
they weren't hooked originally?  Hmmm, even for non-XSM, there is no
restriction on binding the "dom0" vIRQ-s?  Like you say, maybe the
"dom0" vIRQ-s should be behind a permission check?

> The fundamental issue here is that the sending path should be
> fast and hence lightweight. This means (to me) that in
> particular no memory allocations should occur, and (more
> generally) no global or domain wide locks should be taken (for
> rw ones: no write locks).

Yes, that all seems good and reasonable.  With XSM/Flask you also need
the AVC entry for send to be lightweight.

It wouldn't help with the domain transition case, but you could run
the xsm send hooks at bind time to pre-populate the cache.  That would
still require avc code to bypass the memory allocation when holding a
lock in case the entry isn't found.  Your preallocation idea could be
generalized to have avc maintain a reserve of nodes for use when it
cannot allocate.  When it can allocate, it would refill the reserve in
addition to whatever regular allocation it would perform.  But if it's
only evtchn_send that needs special handling, then the complexity may
not be worth adding.

> > Am I correct that the assertion mentioned in the patch description
> > would only be seen in debug builds?
>
> Yes. Release builds would instead have deadlock potential, which
> may get realized at any time (or never, if you're really lucky).
> Catching such cases early, and in an easy to recognize manner,
> is - after all - what the extra checking in debug builds is for.

Thanks for the confirmation.  I tested the XSA patches before seeing
this patch, and it "worked".  Catching the issue now is definitely
better than a mysterious deadlock later.

Thanks,
Jason

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jan Beulich]

On 29.09.2020 19:20, Jason Andryuk wrote:
> On Mon, Sep 28, 2020 at 3:49 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 25.09.2020 20:08, Jason Andryuk wrote:
>>>  Also, a domain label can transition (change) at runtime.
>>> Dropping the send check would latch the permission at bind time which
>>> would not necessarily be valid for the security policy.
>>
>> I did realize this as a possibility too, but there the immediate
>> question is: Why for interdomain channels, but then not also for
>> vIRQ-s, for example? In fact, unless I'm overlooking something,
>> for this specific case there's not even any check in the binding
>> logic, not even for global vIRQ-s. (After all there are two
>> aspects in the permissions here: One is to be eligible to send,
>> which ought to not matter when the sender is Xen, while the
>> other is permission to learn / know of certain events, i.e. in
>> particular global vIRQ-s.)
> 
> I'm not familiar with vIRQ-s, but I did a little bit of review.  A
> vIRQ source is always Xen and its destination is a domain, correct?
> They don't allow a data flow between domains,

Yes and yes.

> so maybe that is why they weren't hooked originally?

Not so much, I assume. Looking a little more closely I find that ...

> Hmmm, even for non-XSM, there is no restriction on binding the "dom0"
> vIRQ-s?

... while binding is allowed, an event would never be received unless
the domain was designated as the receiver via
XEN_DOMCTL_set_virq_handler.

>> The fundamental issue here is that the sending path should be
>> fast and hence lightweight. This means (to me) that in
>> particular no memory allocations should occur, and (more
>> generally) no global or domain wide locks should be taken (for
>> rw ones: no write locks).
> 
> Yes, that all seems good and reasonable.  With XSM/Flask you also need
> the AVC entry for send to be lightweight.
> 
> It wouldn't help with the domain transition case, but you could run
> the xsm send hooks at bind time to pre-populate the cache.

Question is for how long such an entry would remain in the cache,
i.e. whether pre-filling is useful at all. After all pre-filling
has the downside of potentially masking real issues when testing
(as opposed to running in the wild).

>  That would
> still require avc code to bypass the memory allocation when holding a
> lock in case the entry isn't found.  Your preallocation idea could be
> generalized to have avc maintain a reserve of nodes for use when it
> cannot allocate.  When it can allocate, it would refill the reserve in
> addition to whatever regular allocation it would perform.  But if it's
> only evtchn_send that needs special handling, then the complexity may
> not be worth adding.

It was this last aspect which made me not introduce a general
mechanism.

Jan

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jason Andryuk]

On Wed, Sep 30, 2020 at 2:20 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 29.09.2020 19:20, Jason Andryuk wrote:
> > On Mon, Sep 28, 2020 at 3:49 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 25.09.2020 20:08, Jason Andryuk wrote:
> >>>  Also, a domain label can transition (change) at runtime.
> >>> Dropping the send check would latch the permission at bind time which
> >>> would not necessarily be valid for the security policy.
> >>
> >> I did realize this as a possibility too, but there the immediate
> >> question is: Why for interdomain channels, but then not also for
> >> vIRQ-s, for example? In fact, unless I'm overlooking something,
> >> for this specific case there's not even any check in the binding
> >> logic, not even for global vIRQ-s. (After all there are two
> >> aspects in the permissions here: One is to be eligible to send,
> >> which ought to not matter when the sender is Xen, while the
> >> other is permission to learn / know of certain events, i.e. in
> >> particular global vIRQ-s.)
> >
> > I'm not familiar with vIRQ-s, but I did a little bit of review.  A
> > vIRQ source is always Xen and its destination is a domain, correct?
> > They don't allow a data flow between domains,
>
> Yes and yes.
>
> > so maybe that is why they weren't hooked originally?
>
> Not so much, I assume. Looking a little more closely I find that ...
>
> > Hmmm, even for non-XSM, there is no restriction on binding the "dom0"
> > vIRQ-s?
>
> ... while binding is allowed, an event would never be received unless
> the domain was designated as the receiver via
> XEN_DOMCTL_set_virq_handler.

And all of those default to the hardware_domain.  This permission
isn't granted in the default policy, so I was initially confused as to
how it worked.

> >> The fundamental issue here is that the sending path should be
> >> fast and hence lightweight. This means (to me) that in
> >> particular no memory allocations should occur, and (more
> >> generally) no global or domain wide locks should be taken (for
> >> rw ones: no write locks).
> >
> > Yes, that all seems good and reasonable.  With XSM/Flask you also need
> > the AVC entry for send to be lightweight.
> >
> > It wouldn't help with the domain transition case, but you could run
> > the xsm send hooks at bind time to pre-populate the cache.
>
> Question is for how long such an entry would remain in the cache,
> i.e. whether pre-filling is useful at all. After all pre-filling
> has the downside of potentially masking real issues when testing
> (as opposed to running in the wild).

Yes, good point.

> >  That would
> > still require avc code to bypass the memory allocation when holding a
> > lock in case the entry isn't found.  Your preallocation idea could be
> > generalized to have avc maintain a reserve of nodes for use when it
> > cannot allocate.  When it can allocate, it would refill the reserve in
> > addition to whatever regular allocation it would perform.  But if it's
> > only evtchn_send that needs special handling, then the complexity may
> > not be worth adding.
>
> It was this last aspect which made me not introduce a general
> mechanism.

I think we go with this patch since it doesn't restrict the use of XSM
and tries to populate the avc.

Tested-by: Jason Andryuk <jandryuk@gmail.com>

Thanks,
Jason

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Julien Grall]

Hi,

On 24/09/2020 11:53, Jan Beulich wrote:
> xmalloc() & Co may not be called with IRQs off, or else check_lock()
> will have its assertion trigger about locks getting acquired
> inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
> very reasonable, especially since the per-channel lock was introduced to
> avoid acquiring the per-domain event lock on the send paths. Issue a
> second call to xsm_evtchn_send() instead, before acquiring the lock, to
> give XSM / Flask a chance to pre-allocate whatever it may need.
> 
> As these nodes are used merely for caching earlier decisions' results,
> allocate just one node in AVC code despite two potentially being needed.
> Things will merely be not as performant if a second allocation was
> wanted, just like when the pre-allocation fails.
> 
> Fixes: c0ddc8634845 ("evtchn: convert per-channel lock to be IRQ-safe")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

As discussed on the community call with one comment below:

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
> TBD: An even easier fix could be to simply guard xzalloc() by a
>       conditional checking local_irq_is_enabled(), but for a domain
>       sending only interdomain events this would mean AVC's node caching
>       would never take effect on the sending path, as allocation would
>       then always be avoided.
> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -723,6 +723,12 @@ int evtchn_send(struct domain *ld, unsig
>       if ( !port_is_valid(ld, lport) )
>           return -EINVAL;
>   
> +    /*
> +     * As the call further down needs to avoid allocations (due to running
> +     * with IRQs off), give XSM a chance to pre-allocate if needed.
> +     */
> +    xsm_evtchn_send(XSM_HOOK, ld, NULL);

I would suggest to add a comment on top of the evtchn_send callback in 
the XSM hook. This would be helpful for any developer of a new XSM policy.

Cheers,

-- 
Julien Grall

Re: [PATCH] evtchn/Flask: pre-allocate node on send path

[Jason Andryuk]

On Thu, Oct 1, 2020 at 12:05 PM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 24/09/2020 11:53, Jan Beulich wrote:
> > xmalloc() & Co may not be called with IRQs off, or else check_lock()
> > will have its assertion trigger about locks getting acquired
> > inconsistently. Re-arranging the locking in evtchn_send() doesn't seem
> > very reasonable, especially since the per-channel lock was introduced to
> > avoid acquiring the per-domain event lock on the send paths. Issue a
> > second call to xsm_evtchn_send() instead, before acquiring the lock, to
> > give XSM / Flask a chance to pre-allocate whatever it may need.
> >
> > As these nodes are used merely for caching earlier decisions' results,
> > allocate just one node in AVC code despite two potentially being needed.
> > Things will merely be not as performant if a second allocation was
> > wanted, just like when the pre-allocation fails.
> >
> > Fixes: c0ddc8634845 ("evtchn: convert per-channel lock to be IRQ-safe")
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> As discussed on the community call with one comment below:
>
> Acked-by: Julien Grall <jgrall@amazon.com>

Tested-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Thanks,
Jason