Date: Wed, 7 Feb 2024 10:10:15 +0530
From: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
To: john.johansen@...onical.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

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?


Thanks
Neeraj

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