[RPC] Topic: Issues and Testing Regarding SELinx AVC Cache Modification

[RPC] Topic: Issues and Testing Regarding SELinx AVC Cache Modification

[Canfeng Guo]

When calling avc_insert to add nodes to the avc cache, they are inserted into
the head of the hash chain. Similarly, avc_calim_node removes nodes from
the head of the same chain. so, SElinux will delete the latest added cache
infromation.

I question whether the deletion logic proposed in the patch is more appropriate
than the current implementation, or whether alternative mechanisms such as
LRU caching are beneficial.

In my testing environment, I applied the above patch when avc_cache.solt and
cache_threshold were both set to 512 by default. I only have over 280 nodes
in my cache, and the longest observation length of the AVC cache linked list
is only 7 entries. Considering this small size, the cost of traversing the
list is minimal, and such modifications may not incur additional costs.

However, I don't know how to design a test case to verify its cost.
And I cannot prove that this patch is beneficial.

I attempted to simulate a more demanding scenario by increasing the cache_threshold
to 2048 in order to establish a longer linked list of AVC caches, but
I was unable to generate more than 2048 AVC records, possibly due to the need
for a highly complex environment with numerous different SID interactions.

Therefore, I have two questions:
The necessity of modification:
     Considering its potential impact on the cache performance of SELinx AVC,
     is it worth investing effort into this modification?, i think that in most cases,
     this modification is not necessart.
Verification method:
     If making such modifications is reasonable, how can I effectively
     measure its impact on system performance?

Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
---
 security/selinux/avc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 32eb67fb3e42..9999028660c9 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -477,6 +477,9 @@ static inline int avc_reclaim_node(void)
 
 		rcu_read_lock();
 		hlist_for_each_entry(node, head, list) {
+			while(node->next){
+				node = node->next;
+			}
 			avc_node_delete(node);
 			avc_cache_stats_incr(reclaims);
 			ecx++;
-- 
2.20.1

Re: [RPC] Topic: Issues and Testing Regarding SELinx AVC Cache Modification

[Stephen Smalley]

On Sun, Jul 14, 2024 at 9:44 PM Canfeng Guo <guocanfeng@uniontech.com> wrote:
>
> When calling avc_insert to add nodes to the avc cache, they are inserted into
> the head of the hash chain. Similarly, avc_calim_node removes nodes from
> the head of the same chain. so, SElinux will delete the latest added cache
> infromation.
>
> I question whether the deletion logic proposed in the patch is more appropriate
> than the current implementation, or whether alternative mechanisms such as
> LRU caching are beneficial.
>
> In my testing environment, I applied the above patch when avc_cache.solt and
> cache_threshold were both set to 512 by default. I only have over 280 nodes
> in my cache, and the longest observation length of the AVC cache linked list
> is only 7 entries. Considering this small size, the cost of traversing the
> list is minimal, and such modifications may not incur additional costs.
>
> However, I don't know how to design a test case to verify its cost.
> And I cannot prove that this patch is beneficial.
>
> I attempted to simulate a more demanding scenario by increasing the cache_threshold
> to 2048 in order to establish a longer linked list of AVC caches, but
> I was unable to generate more than 2048 AVC records, possibly due to the need
> for a highly complex environment with numerous different SID interactions.
>
> Therefore, I have two questions:
> The necessity of modification:
>      Considering its potential impact on the cache performance of SELinx AVC,
>      is it worth investing effort into this modification?, i think that in most cases,
>      this modification is not necessart.

I don't think it is desirable or necessary. The current logic prunes
the least recently used bucket and intentionally reclaims multiple
nodes at a time.

> Verification method:
>      If making such modifications is reasonable, how can I effectively
>      measure its impact on system performance?

The selinux-testsuite exercises many security contexts and thus should
enable reaching higher numbers of AVC nodes.

>
> Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
> ---
>  security/selinux/avc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 32eb67fb3e42..9999028660c9 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -477,6 +477,9 @@ static inline int avc_reclaim_node(void)
>
>                 rcu_read_lock();
>                 hlist_for_each_entry(node, head, list) {
> +                       while(node->next){
> +                               node = node->next;
> +                       }
>                         avc_node_delete(node);
>                         avc_cache_stats_incr(reclaims);
>                         ecx++;
> --
> 2.20.1
>

Re: [RPC] Topic: Issues and Testing Regarding SELinx AVC Cache Modification

[Stephen Smalley]

On Mon, Jul 15, 2024 at 9:46 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Sun, Jul 14, 2024 at 9:44 PM Canfeng Guo <guocanfeng@uniontech.com> wrote:
> >
> > When calling avc_insert to add nodes to the avc cache, they are inserted into
> > the head of the hash chain. Similarly, avc_calim_node removes nodes from
> > the head of the same chain. so, SElinux will delete the latest added cache
> > infromation.
> >
> > I question whether the deletion logic proposed in the patch is more appropriate
> > than the current implementation, or whether alternative mechanisms such as
> > LRU caching are beneficial.
> >
> > In my testing environment, I applied the above patch when avc_cache.solt and
> > cache_threshold were both set to 512 by default. I only have over 280 nodes
> > in my cache, and the longest observation length of the AVC cache linked list
> > is only 7 entries. Considering this small size, the cost of traversing the
> > list is minimal, and such modifications may not incur additional costs.
> >
> > However, I don't know how to design a test case to verify its cost.
> > And I cannot prove that this patch is beneficial.
> >
> > I attempted to simulate a more demanding scenario by increasing the cache_threshold
> > to 2048 in order to establish a longer linked list of AVC caches, but
> > I was unable to generate more than 2048 AVC records, possibly due to the need
> > for a highly complex environment with numerous different SID interactions.
> >
> > Therefore, I have two questions:
> > The necessity of modification:
> >      Considering its potential impact on the cache performance of SELinx AVC,
> >      is it worth investing effort into this modification?, i think that in most cases,
> >      this modification is not necessart.
>
> I don't think it is desirable or necessary. The current logic prunes
> the least recently used bucket and intentionally reclaims multiple
> nodes at a time.
>
> > Verification method:
> >      If making such modifications is reasonable, how can I effectively
> >      measure its impact on system performance?
>
> The selinux-testsuite exercises many security contexts and thus should
> enable reaching higher numbers of AVC nodes.

Other, more real-world ways of exercising many security contexts would
be to launch many containers or VMs on a Fedora or RHEL system using
their integrated support for per-container or per-VM SELinux security
contexts.