Re: RCU issue with SELinux (Re: SELINUX performance issues)

[Kaigai Kohei]

Hi Stephen, thanks for your comments.

> A few other comments on the patch:
> 
> + new = kmalloc(sizeof(struct avc_node), GFP_ATOMIC);
> + if (!new)
> + return NULL;
> 
> Dynamically allocating the nodes at runtime (rather than pre-allocating
> them and then just reclaiming them as necessary as in the current AVC)
> worries me, as it introduces a new failure case for avc_has_perm. 
> Denying permission to a resource due to transient memory shortage is not
> good for robustness.  And changing the GFP_ATOMIC is not an option, as
> calling context may not allow blocking.  Hence, pre-allocation seems
> desirable, regardless of the locking scheme.

In my understanding, your worry about robustness is the execution path
when kmalloc() returns NULL.
The most significant point is there.
--[ in the original kernel-2.6.8.1 ]--
    :
  rc = avc_insert(ssid,tsid,tclass,&entry,aeref);
  if (rc) {
    spin_unlock_irqrestore(&avc_lock,flags);
    goto out;
  }
    :
--------------------------------------
In the original implementation, avc_insert() returns -ENOMEM when we can't
hold a avc_entry by avc_claim_node(), then avc_has_perm_noaudit() denies
the requested permition check by the reason an avc_node is not allocated.
(But avc_insert() always returns 0, because avc_insert() reclaim a avc_node
 under the spinlock when free_list is empty.)
However, allocating an avc_node and making the decision according to
the policy of SELinux should be separated in considering.

In my approach with RCU, avc_insert() can actually return NULL.
Currently, avc_has_perm_noaudit() returns -ENOMEM without the decision-making
like as the original implementation.
But we can make an decision according to the policy of SELinux by a suitable
recover processing.

My proposal is as follows:
- Normaly, decision-making is performed to the entry on AVC.
  These entries are allocated by kmalloc()
- When kmalloc() called by the extension of avc_insert() returns NULL,
  the decision-making is performed to the entry on stack variable,
  then the entry is not hold on AVC.

-- e.g. in avc_has_perm_noaudit() --------------
    :
  struct avc_entry local_ae, *ae;
    :
  node = avc_insert(....);
  if (!node) {
    SETUP_LOCAL_AE(&local_ae,&entry.avd);
    ae = &local_ae;
  } else {
    ae = &node->ae;
  }
  
  if (avd)
    memcpy(avd, &ae->avd, sizeof(*avd));
    :
------------------------------------------------
By this method, the decision-making is available irrespective of
the result of kmalloc(). Is it robustless?
The original implementation has too many lock contensitons in Big-SMP
environment. It is more positive to consider the method using RCU.

> In trying to merge the logic related to latest_notif, you've introduced
> a bug - latest_notif should only be increased, never decreased.  See the
> original logic from avc_control and avc_ss_reset prior to your patch. 
> Those functions update the latest notif based on a policy change event. 
> In the insert case, you are checking that the entry is not stale, i.e.
> has a smaller seqno than the latest notification due to an interleaving
> with a policy change event.

Ooooooops!
It is toooooo trivial BUG!
I'll fix them very soon.

> + if (node->ae.avd.allowed != (node->ae.avd.allowed|requested))
> + avc_update_node(AVC_CALLBACK_GRANT
> +                 ,requested,ssid,tsid,tclass);
>   }
> 
> The test seems unnecessary, as the function has already determined that
> not all of the requested permissions were granted, so you should be able
> to just unconditionally call avc_update_node here, and you only need to
> pass it the denied set that has already been computed, as any other
> permissions in requested were already allowed.

Indeed, it is right. I'll fix them soon.
avc_update_node() is in the section between rcu_read_lock() and rcu_read_unlock().

> - rc = -ENOENT;
>  out:
> - return rc;
> + return node;
> +}
> 
> Ah, I think the bug is here.

Oops!
I agree, and fix this soon.

Please wait for a patch, thanks.
--------
Kai Gai <kaigai@xxxxxxxxxxxxx>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/