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 .
> /*@ 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@xxxxxxxxxxxxxxx <selinux-owner@xxxxxxxxxxxxxxx> On Behalf Of Stephen Smalley
Sent: Monday, December 9, 2019 11:35 PM
To: rsiddoji@xxxxxxxxxxxxxx; selinux@xxxxxxxxxxxxxxx
Cc: paul@xxxxxxxxxxxxxx; linux-security-module@xxxxxxxxxxxxxxx
Subject: Re: Looks like issue in handling active_nodes count in 4.19 kernel .
On 12/9/19 10:55 AM, rsiddoji@xxxxxxxxxxxxxx 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?
