Date: Fri, 9 Feb 2024 09:33:17 -0800
From: John Johansen <john.johansen@...onical.com>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>, paul@...l-moore.com,
 jmorris@...ei.org, serge@...lyn.com
Cc: linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
 "Gautham R. Shenoy" <gautham.shenoy@....com>,
 "Shukla, Santosh" <Santosh.Shukla@....com>,
 "Narayan, Ananth" <Ananth.Narayan@....com>,
 raghavendra.kodsarathimmappa@....com, koverstreet@...gle.com,
 paulmck@...nel.org, boqun.feng@...il.com, vinicius.gomes@...el.com,
 mjguzik@...il.com
Subject: Re: [RFC 0/9] Nginx refcount scalability issue with Apparmor enabled
 and potential solutions

On 2/6/24 20:40, Neeraj Upadhyay wrote:
> Gentle ping.
>   
> John,
> 
> Could you please confirm that:
> 
> a. The AppArmor refcount usage described in the RFC is correct?
> b. Approach taken to fix the scalability issue is valid/correct?
> 

Hi Neeraj,

I know your patchset has been waiting on review for a long time.
Unfortunately I have been very, very busy lately. I will try to
get to it this weekend, but I can't promise that I will be able
to get the review fully done.

john


> 
> On 1/10/2024 4:41 PM, Neeraj Upadhyay wrote:
>> Problem Statement
>> =================
>>
>> Nginx performance testing with Apparmor enabled (with nginx
>> running in unconfined profile), on kernel versions 6.1 and 6.5
>> show significant drop in throughput scalability, when Nginx
>> workers are scaled to higher number of CPUs across various
>> L3 cache domains.
>>
>> Below is one sample data on the throughput scalability loss,
>> based on results on AMD Zen4 system with 96 CPUs with SMT
>> core count 2; so, overall, 192 CPUs:
>>
>> Config      Cache Domains     apparmor=off        apparmor=on
>>                               scaling eff (%)      scaling eff (%)
>> 8C16T          1                  100%             100%
>> 16C32T         2                   95%              94%
>> 24C48T         3                   94%              93%
>> 48C96T         6                   92%              88%
>> 96C192T        12                  85%              68%
>>
>> If we look at above data, there is a significant drop in
>> scaling efficiency, when we move to 96 CPUs/192 SMT threads.
>>
>> Perf tool shows most of the contention coming from below
>> 6.56%     nginx  [kernel.vmlinux]      [k] apparmor_current_getsecid_subj
>> 6.22%     nginx  [kernel.vmlinux]      [k] apparmor_file_open
>>
>> The majority of the CPU cycles is found to be due to memory contention
>> in atomic_fetch_add and atomic_fetch_sub operations from kref_get() and
>> kref_put() operations on label.
>>
>> Commit 2516fde1fa00 ("apparmor: Optimize retrieving current task secid"),
>> from 6.7 alleviates the issue to an extent, but not completely:
>>
>> Config      Cache Domains     apparmor=on        apparmor=on (patched)
>>                               scaling eff (%)      scaling eff (%)
>> 8C16T          1                  100%             100%
>> 16C32T         2                   97%              93%
>> 24C48T         3                   94%              92%
>> 48C96T         6                   88%              88%
>> 96C192T        12                  65%              79%
>>
>> This adverse impact gets more pronounced when we move to >192 CPUs.
>> The memory contention and impact increases with high frequency label
>> update operations and labels are marked stale more frequently.
>>
>>
>> Label Refcount Management
>> =========================
>>
>> Apparmor uses label objects (struct aa_label) to manage refcounts for
>> below set of objects:
>>
>> - Applicable profiles
>> - Namespaces (unconfined profile)
>> - Other non-profile references
>>
>> These label references are acquired on various apparmor lsm hooks,
>> on operations such as file open, task kill operations, socket bind,
>> and other file, socket, misc operations which use current task's cred,
>> when the label for the current cred, has been marked stale. This is
>> done to check these operations against the set of allowed operations
>> for the task performing them.
>>
>> Use Percpu refcount for ref management?
>> =======================================
>>
>> The ref put operations (percpu_ref_put()) in percpu refcount,
>> in active mode, do not check whether ref count has dropped to
>> 0. The users of the percpu_ref need to explicitly invoke
>> a percpu_ref_kill() operation, to drop the initial reference,
>> at shutdown paths. After the percpu_ref_kill() operation, ref
>> switches to atomic mode and any new percpu_ref_put() operation
>> checks for the drop to 0 case and invokes the release operation
>> on that label.
>>
>> Labels are marked stale is situations like profile removal,
>> profile updates. For a namespace, the unconfined label reference
>> is dropped when the namespace is destroyed. These points
>> are potential shutdown points for labels. However, killing
>> the percpu ref from these points has few issues:
>>
>> - The label could still be referenced by tasks, which are
>>    still holding the reference to the now stale label.
>>    Killing the label ref while these operations are in progress
>>    will make all subsequent ref-put operations on the stale label
>>    to be atomic, which would still result in memory contention.
>>    Also, any new reference to the stale label, which is acquired
>>    with the elevated refcount will have atomic op contention.
>>
>> - The label is marked stale using a non-atomic write operation.
>>    It is possible that new operations do not observe this flag
>>    and still reference it for quite some time.
>>
>> - Explicitly tracking the shutdown points might not be maintainable
>>    at a per label granularity, as there can be various paths where
>>    label reference could get dropped, such as, before the label has
>>    gone live - object initialization error paths. Also, tracking
>>    the shutdown points for labels which reference other labels -
>>    subprofiles, merged labels requires careful analysis, and adds
>>    heavy burden on ensuring the memory contention is not introduced
>>    by these ref kill points.
>>
>>
>> Proposed Solution
>> =================
>>
>> One potential solution to the refcount scalability problem is to
>> convert the label refcount to a percpu refcount, and manage
>> the initial reference from kworker context. The kworker
>> keeps an extra reference to the label and periodically scans
>> labels and release them if their refcount drops to 0.
>>
>> Below is the sequence of operations, which shows the refcount
>> management with this approach:
>>
>> 1. During label initialization, the percpu ref is initialized in
>>     atomic mode. This is done to ensure that, for cases where the
>>     label hasn't gone live (->ns isn't assigned), mostly during
>>     initialization error paths.
>>
>> 2. Labels are switched to percpu mode at various points -
>>     when a label is added to labelset tree, when a unconfined profile
>>     has been assigned a namespace.
>>
>> 3. As part of the initial prototype, only the in tree labels
>>     are managed by the kworker. These labels are added to a lockless
>>     list. The unconfined labels invoke a percpu_ref_kill() operation
>>     when the namespace is destroyed.
>>
>> 4. The kworker does a periodic scan of all the labels in the
>>     llist. It does below sequence of operations:
>>
>>     a. Enqueue a dummy node to mark the start of scan. This dummy
>>        node is used as start point of scan and ensures that we
>>        there is no additional synchronization required with new
>>        label node additions to the llist. Any new labels will
>>        be processed in next run of the kworker.
>>
>>                        SCAN START PTR
>>                            |
>>                            v
>>        +----------+     +------+    +------+    +------+
>>        |          |     |      |    |      |    |      |
>>        |   head   ------> dummy|--->|label |--->| label|--->NULL
>>        |          |     | node |    |      |    |      |
>>        +----------+     +------+    +------+    +------+
>>
>>
>>        New label addition:
>>
>>                              SCAN START PTR
>>                                   |
>>                                   v
>>        +----------+  +------+  +------+    +------+    +------+
>>        |          |  |      |  |      |    |      |    |      |
>>        |   head   |--> label|--> dummy|--->|label |--->| label|--->NULL
>>        |          |  |      |  | node |    |      |    |      |
>>        +----------+  +------+  +------+    +------+    +------+
>>
>>      b. Traverse through the llist, starting from dummy->next.
>>         If the node is a dummy node, mark it free.
>>         If the node is a label node, do,
>>
>>         i) Switch the label ref to atomic mode. The ref switch wait
>>            for the existing percpu_ref_get() and percpu_ref_put()
>>            operations to complete, by waiting for a RCU grace period.
>>
>>            Once the switch is complete, from this point onwards, any
>>            percpu_ref_get(), percpu_ref_put() operations use
>>            atomic operations.
>>
>>        ii) Drop the initial reference, which was taken while adding
>>            the label node to the llist.
>>
>>       iii) Use a percpu_ref_tryget() increment operation on the
>>            ref, to see if we dropped the last ref count. if we
>>            dropped the last count, we remove the node from the llist.
>>
>>            All of these operations are done inside a RCU critical
>>            section, to avoid race with the release operations,
>>            which can potentially trigger, as soon as we drop
>>            the initial ref count.
>>
>>        iv) If we didn't drop the last ref, switch back the counter
>>            to percpu mode.
>>
>> Using this approach, to move the atomic refcount manipulation out of the
>> contended paths, there is a significant scalability improvement seen on
>> nginx test, and scalability efficiency is close to apparmor-off case.
>>
>> Config      Cache Domains     apparmor=on (percpuref)
>>                                 scaling eff (%)
>> 8C16T          1                  100%
>> 16C32T         2                   96%
>> 24C48T         3                   94%
>> 48C96T         6                   93%
>> 96C192T        12                  90%
>>
>> Limitations
>> ===========
>>
>> 1. Switching to percpu refcount increases memory size overhead, as
>>     percpu memory is allocated for all labels.
>>
>> 2. Deferring labels reclaim could potentially result in memory
>>     pressure, when there are high frequency of label update operations.
>>
>> 3. Percpu refcount uses call_rcu_hurry() to complete switch operations.
>>     These can impact energy efficiency, due to back to back hurry
>>     callbacks. Using deferrable workqueue partly mitigates this.
>>     However, deferring kworker can delay reclaims.
>>
>> 4. Back to back label switches can delay other percpu users, as
>>     there is a single global switch spinlock used by percpu refcount
>>     lib.
>>
>> 5. Long running kworker can delay other use cases like system suspend.
>>     This is mitigated using freezable workqueue and litming node
>>     scans to a max count.
>>
>> 6. There is a window where label operates is atomic mode, when its
>>     counter is being checked for zero. This can potentially result
>>     in high memory contention, during this window which spans RCU
>>     grace period (plus callback execution). For example, when
>>     scanning label corresponding to unconfined profile, all
>>     applications which use unconfined profile would be using
>>     atomic ref increment and decrement operations.
>>
>>     There are a few apparoaches which were tried to mitigate this issue:
>>
>>     a. At a lower time interval, check if scanned label's counter
>>        has changed since the start of label scan. If there is a change
>>        in count, terminate the switch to atomic mode. Below shows the
>>        apparoch using rcuwait.
>>
>>        static void aa_label_switch_atomic_confirm(struct percpu_ref *label_ref)
>>        {
>>           WRITE_ONCE(aa_atomic_switch_complete, true);
>>           rcuwait_wake_up(&aa_reclaim_rcuwait);
>>        }
>>
>>        rcuwait_init(&aa_reclaim_rcuwait);
>>        percpu_ref_switch_to_atomic(&label->count, aa_label_switch_atomic_confirm);
>>
>>        atomic_count = percpu_ref_count_read(&label->count);
>>        do {
>>          rcuwait_wait_event_timeout(&aa_reclaim_rcuwait,
>>                             (switch_complete = READ_ONCE(aa_atomic_switch_complete)),
>>                             TASK_IDLE,
>>                             msecs_to_jiffies(5));
>>          if (percpu_ref_count_read(&label->count) != atomic_count)
>>                  break;
>>         } while (!READ_ONCE(switch_complete));
>>
>>         However, this approach does not work, as percpu refcount lib does not
>>         allow termination of an ongoing switch operation. Also, the counter
>>         can return to the original value with set of get() and put() operations
>>         before we check the current value.
>>
>>     b. Approaches to notify the reclaim kworker from ref get and put operations
>>        can potentially disturb cache line state between the various CPU
>>        contexts, which are referncing the label, and can potentially impact
>>        scalability again.
>>
>>     c. Swith the label to an immortal percpu ref, while the scan operates
>>        on the current counter.
>>
>>        Below is the sequence of operations to do this:
>>
>>        1. Ensure that both immortal ref and label ref are in percpu mode.
>>           Reinit the immortal ref in percpu mode.
>>
>>           Swap percpu and atomic counters of label refcount and immortal ref
>> 	                          percpu-ref
>>        	                  +-------------------+
>>        +-------+           |  percpu-ctr-addr1 |
>>        | label | --------->|-------------------|    +----------------+
>>        +-------+           |   data            |--->| Atomic counter1|
>>                            +-------------------+    +----------------+
>>        +-------+           +-------------------+
>>        |ImmLbl |---------->|  percpu-ctr-addr2 |    +----------------+
>>        +-------+           |-------------------|--->| Atomic counter2|
>>                            |    data           |    +----------------+
>>                            +-------------------+
>>
>>            label ->percpu-ctr-addr  = percpu-ctr-addr2
>>            ImmLbl ->percpu-ctr-addr = percpu-ctr-addr1
>>            label ->data->count      = Atomic counter2
>>            ImmLbl ->data->count     = Atomic counter1
>>    
>>    
>>        2. Check the counters collected in immortal label, by switch it
>>           to atomic mode.
>>
>>        3. If the count is 0, do,
>>           a. Switch immortal counter to percpu again, giving it an
>>              initial count of 1.
>>           b. Swap the label and immortal counters again. The immortal
>>              ref now has the counter values from new percpu ref get
>>              and get operations on the label ref, from the point
>>              when we did the initial swap operation.
>>           c. Transfer the percpu counts in immortal ref to atomic
>>              counter of label percpu refcount.
>>           d. Kill immortal ref, for reinit on next iteration.
>>           e. Switch label percpu ref to atomic mode.
>>           f. If the counter is 1, drop the initial ref.
>>
>>         4. If the count is not 0, re-swap the counters.
>>            a. Switch immortal counter to percpu again, giving it an
>>               initial count of 1.
>>            b. Swap the label and immortal counters again. The immortal
>>               ref now has the counter values from new percpu ref get
>>               and get operations on the label ref, from the point
>>               when we did the initial swap operation.
>>            c. Transfer the percpu counts in immortal ref to atomic
>>               counter of label percpu refcount.
>>            d. Kill immortal ref, for reinit on next iteration.
>>
>>
>>            Using this approach, we ensure that, label ref users do not switch
>>            to atomic mode, while there are active references on the label.
>>            However, this approach requires multiple percpu ref mode switches
>>            and adds high overhead and complexity to the scanning code.
>>
>> Extended/Future Work
>> ====================
>>
>> 1. Look for ways to fix the limitations, as described in the "Limitations"
>>     section.
>>
>> 2. Generalize the approach to percpu rcuref, which is used for contexts
>>     where release path uses RCU grace period for release operations. Patch
>>     7 creates an initial prototype for this.
>>
>> 3. Explore hazard pointers for scalable refcounting of labels.
>>
>> Highly appreciate any feedback/suggestions on the design approach.
>>
>> The patches of this patchset introduce following changes:
>>
>> 1.      Documentation of Apparmor Refcount management.
>>
>> 2.      Switch labels to percpu refcount in atomic mode.
>>
>>          Use percpu refcount for apparmor labels. Initial patch to init
>>          the percpu ref in atomic mode, to evaluate the potential
>>          impact of percpuref on top of kref based implementation.
>>
>> 3.      Switch unconfined namespaces refcount to percpu mode.
>>
>>          Switch unconfined ns labels to percpu mode, and kill the
>>          initial refcount from namespace destroy path.
>>
>> 4.      Add infrastructure to reclaim percpu labels.
>>
>>          Add a label reclaim infrastructure for labels which are
>>          in percpu mode, for managing their inital refcount.
>>
>> 5.      Switch intree labels to percpu mode.
>>
>>          Use label reclaim infrastruture to manage intree labels.
>>
>> 6.      Initial prototype for optimizing ref switch.
>>
>>          Prototype for reducing the time window when a label
>>          scan switches the label ref to atomic mode.
>>
>> 7.      percpu-rcuref: Add basic infrastructure.
>>
>>          Prototype for Percpu refcounts for objects, which protect
>>          their object reclaims using RCU grace period.
>>
>> 8.      Switch labels to percpu rcurefcount in unmanaged mode.
>>
>>          Use percpu rcuref for labels. Start with unmanaged/atomic
>>          mode.
>>
>> 9.      Switch unconfined and in tree labels to managed ref mode.
>>
>>          Use percpu mode with manager worker for unconfined and intree
>>          labels.
>>
>>
>> ------------------------------------------------------------------------
>>
>>   b/Documentation/admin-guide/LSM/ApparmorRefcount.rst |  351 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   b/Documentation/admin-guide/LSM/index.rst            |    1
>>   b/Documentation/admin-guide/kernel-parameters.txt    |    8 +
>>   b/include/linux/percpu-rcurefcount.h                 |  115 ++++++++++++++++
>>   b/include/linux/percpu-refcount.h                    |    2
>>   b/lib/Makefile                                       |    2
>>   b/lib/percpu-rcurefcount.c                           |  336 +++++++++++++++++++++++++++++++++++++++++++++++
>>   b/lib/percpu-refcount.c                              |   93 +++++++++++++
>>   b/security/apparmor/include/label.h                  |   16 +-
>>   b/security/apparmor/include/policy.h                 |    8 -
>>   b/security/apparmor/include/policy_ns.h              |   24 +++
>>   b/security/apparmor/label.c                          |   11 +
>>   b/security/apparmor/lsm.c                            |  145 ++++++++++++++++++++
>>   b/security/apparmor/policy_ns.c                      |    6
>>   include/linux/percpu-refcount.h                      |    2
>>   lib/percpu-refcount.c                                |   93 -------------
>>   security/apparmor/include/label.h                    |   17 +-
>>   security/apparmor/include/policy.h                   |   56 +++----
>>   security/apparmor/include/policy_ns.h                |   24 ---
>>   security/apparmor/label.c                            |   11 -
>>   security/apparmor/lsm.c                              |  325 ++++++++++++----------------------------------
>>   security/apparmor/policy_ns.c                        |    8 -
>>   22 files changed, 1237 insertions(+), 417 deletions(-)
>>
>> base-commit: ab27740f7665

Powered by blists - more mailing lists