| Message ID | 20240715014337.11625-1-guocanfeng@uniontech.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RPC] Topic: Issues and Testing Regarding SELinx AVC Cache Modification | expand |
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 >
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.
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++;
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(+)