Looks like issue in handling active_nodes count in 4.19 kernel .

[Stephen Smalley]

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?

> 
>> /*@ 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?
> 
> 
>