Re: Looks like issue in handling active_nodes count in 4.19 kernel .

[Stephen Smalley <sds@tycho.nsa.gov>]

On 12/9/19 10:55 AM, rsiddoji@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?