Looks like issue in handling active_nodes count in 4.19 kernel .

[rsiddoji at codeaurora.org]

Thanks for tacking the patch fwd . On the  question :

Actually issue started when we were seeing most of the  time "avc_reclaim_node" in the stack . 
Which on debugging further  avc_cache.active_nodes was already in 7K+ nodes  and  as the logic  is 

As below . 
	if (atomic_inc_return(&avc->avc_cache.active_nodes) >   avc->avc_cache_threshold)
           			avc_reclaim_node(avc);

So if the  active_nodes count is  > 512  (if not configured) we will be always be calling   avc_reclaim_node() and eventually  for each  node insert we will be calling avc_reclaim_node  and might  be expansive then using 
cache  and advantage of cache might be null and void due to this overhead?

Thanks , 
Ravi

-----Original Message-----
From: selinux-owner at vger.kernel.org <selinux-owner at vger.kernel.org> On Behalf Of Stephen Smalley
Sent: Wednesday, December 11, 2019 8:18 PM
To: rsiddoji at codeaurora.org; selinux at vger.kernel.org
Cc: paul at paul-moore.com; linux-security-module at vger.kernel.org
Subject: Re: Looks like issue in handling active_nodes count in 4.19 kernel .

On 12/11/19 9:37 AM, Stephen Smalley wrote:
> On 12/9/19 1:30 PM, rsiddoji at codeaurora.org wrote:
>> Thanks for quick response , yes it will be helpful if you can raise 
>> the change .
>> On the second issue  in  avc_alloc_node we are trying to check the 
>> slot status  as    active_nodes  > 512 ( default ) Where  checking 
>> the occupancy  should be corrected as     active_nodes
>> > 80% of slots occupied  or 16*512 or
>> May be we need to use a different logic .
> 
> Are you seeing an actual problem with this in practice, and if so, 
> what exactly is it that you are seeing and do you have a reproducer?

BTW, on Linux distributions, there is an avcstat(8) utility that can be used to monitor the AVC statistics, or you can directly read the stats from the kernel via /sys/fs/selinux/avc/cache_stats

> 
>>
>>> /*@ static struct avc_node *avc_alloc_node(struct selinux_avc *avc) 
>>> */
>>>
>>>        if (atomic_inc_return(&avc->avc_cache.active_nodes) >
>>>            avc->avc_cache_threshold)      //  default  threshold is 
>>> 512
>>>            avc_reclaim_node(avc);
>>>
>>
>> Regards,
>> Ravi
>>
>> -----Original Message-----
>> From: selinux-owner at vger.kernel.org <selinux-owner at vger.kernel.org> 
>> On Behalf Of Stephen Smalley
>> Sent: Monday, December 9, 2019 11:35 PM
>> To: rsiddoji at codeaurora.org; selinux at vger.kernel.org
>> Cc: paul at paul-moore.com; linux-security-module at vger.kernel.org
>> Subject: Re: Looks like issue in handling active_nodes count in 4.19 
>> kernel .
>>
>> On 12/9/19 10:55 AM, rsiddoji at codeaurora.org wrote:
>>> Hi team ,
>>> Looks like we have  issue in handling the  "active_nodes" count in 
>>> the Selinux - avc.c file.
>>> Where  avc_cache.active_nodes increase more than slot array   and 
>>> code frequency calling of avc_reclaim_node()  from  avc_alloc_node() 
>>> ;
>>>
>>> Where following are the 2 instance which seem to  possible culprits 
>>> which are seen on 4.19 kernel . Can you  comment if my understand is 
>>> wrong.
>>>
>>>
>>> #1. if we see the  active_nodes count is incremented in 
>>> avc_alloc_node
>>> (avc) which is called in avc_insert() Where if the code take  
>>> failure path on  avc_xperms_populate  the code will not decrement 
>>> this counter .
>>>
>>>
>>> static struct avc_node *avc_insert(struct selinux_avc *avc,
>>>                    u32 ssid, u32 tsid, u16 tclass,
>>>                       struct av_decision *avd, ....
>>>     node = avc_alloc_node(avc);  //incremented here ....
>>>                 rc = avc_xperms_populate(node, xp_node);  // 
>>> possibilities of this getting failure is there .
>>>         if (rc) {
>>>             kmem_cache_free(avc_node_cachep, node);  // but on 
>>> failure we are not decrementing active_nodes ?
>>>             return NULL;
>>>            }
>>
>> I think you are correct; we should perhaps be calling avc_node_kill() 
>> here as we do in an earlier error path?
>>
>>>
>>> #2.  where it looks like the logic on comparing the  active_nodes 
>>> against avc_cache_threshold seems  wired  as the count of active 
>>> nodes is always going to be
>>>    more than 512 will may land in simply  removing /calling 
>>> avc_reclaim_node frequently much before the slots are full maybe we 
>>> are not using cache at best ?
>>>    we should be comparing with some high watermark ? or my 
>>> understanding wrong ?
>>> /*@ static struct avc_node *avc_alloc_node(struct selinux_avc *avc) 
>>> */
>>>
>>>        if (atomic_inc_return(&avc->avc_cache.active_nodes) >
>>>            avc->avc_cache_threshold)      //  default  threshold is 
>>> 512
>>>            avc_reclaim_node(avc);
>>>
>>
>> Not entirely sure what you are asking here.  avc_reclaim_node() 
>> should reclaim multiple nodes up to AVC_CACHE_RECLAIM.  Possibly that 
>> should be configurable via selinuxfs too, and/or calculated from 
>> avc_cache_threshold in some way?
>>
>> Were you interested in creating a patch to fix the first issue above 
>> or looking to us to do so?
>>
>>
>>
>