| Message ID | 1457376161-24982-2-git-send-email-cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 07.03.16 at 19:42, <cardoe@cardoe.com> wrote: > Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ > to use the Kconfig variable. Same question here: What's the benefit of doing it this way? > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -23,6 +23,12 @@ config FLASK > > If unsure, say N. > > +config FLASK_AVC_STATS > + def_bool y if FLASK > + depends on FLASK With this "depends" the "if FLASK" is pointless. Also (again) - indentation. Jan
On 03/08/2016 04:46 AM, Jan Beulich wrote: >>>> On 07.03.16 at 19:42, <cardoe@cardoe.com> wrote: >> Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ >> to use the Kconfig variable. > > Same question here: What's the benefit of doing it this way? This removes the stats tracking, which might (I have not tested) speed up the security server by avoiding the __get_cpu_var call and increment. The corresponding SELinux knob is a Kconfig option in Linux. Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> On 08.03.16 at 17:22, <dgdegra@tycho.nsa.gov> wrote: > On 03/08/2016 04:46 AM, Jan Beulich wrote: >>>>> On 07.03.16 at 19:42, <cardoe@cardoe.com> wrote: >>> Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ >>> to use the Kconfig variable. >> >> Same question here: What's the benefit of doing it this way? > > This removes the stats tracking, which might (I have not tested) speed up > the security server by avoiding the __get_cpu_var call and increment. No, I don not think the patch removes anything. The Kconfig option doesn't have a prompt. But anyway, ... > The > corresponding SELinux knob is a Kconfig option in Linux. > > Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> ... if you're fine with it, we'll put it in (once the mechanical issues got addressed). Jan
On 03/08/2016 11:51 AM, Jan Beulich wrote: >>>> On 08.03.16 at 17:22, <dgdegra@tycho.nsa.gov> wrote: >> On 03/08/2016 04:46 AM, Jan Beulich wrote: >>>>>> On 07.03.16 at 19:42, <cardoe@cardoe.com> wrote: >>>> Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ >>>> to use the Kconfig variable. >>> >>> Same question here: What's the benefit of doing it this way? >> >> This removes the stats tracking, which might (I have not tested) speed up >> the security server by avoiding the __get_cpu_var call and increment. > > No, I don not think the patch removes anything. The Kconfig option > doesn't have a prompt. But anyway, ... Ah, I missed that: I saw the --help-- line and assumed it was the prompt. Either way, this #define is a configuration-like knob that doesn't need to be hard-coded in a header as it currently is. > >> The >> corresponding SELinux knob is a Kconfig option in Linux. >> >> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > > ... if you're fine with it, we'll put it in (once the mechanical issues > got addressed).
On 3/8/16 12:01 PM, Daniel De Graaf wrote: > On 03/08/2016 11:51 AM, Jan Beulich wrote: >>>>> On 08.03.16 at 17:22, <dgdegra@tycho.nsa.gov> wrote: >>> On 03/08/2016 04:46 AM, Jan Beulich wrote: >>>>>>> On 07.03.16 at 19:42, <cardoe@cardoe.com> wrote: >>>>> Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with >>>>> CONFIG_ >>>>> to use the Kconfig variable. >>>> >>>> Same question here: What's the benefit of doing it this way? >>> >>> This removes the stats tracking, which might (I have not tested) >>> speed up >>> the security server by avoiding the __get_cpu_var call and increment. >> >> No, I don not think the patch removes anything. The Kconfig option >> doesn't have a prompt. But anyway, ... > > Ah, I missed that: I saw the --help-- line and assumed it was the prompt. > Either way, this #define is a configuration-like knob that doesn't need to > be hard-coded in a header as it currently is. > >> >>> The >>> corresponding SELinux knob is a Kconfig option in Linux. >>> >>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> >> ... if you're fine with it, we'll put it in (once the mechanical issues >> got addressed). > Daniel, Would you like me to make this a real configuration option? Or proceed with the current path and we can make a configuration option later?
On 3/14/16 9:05 AM, Doug Goldstein wrote: > On 3/8/16 12:01 PM, Daniel De Graaf wrote: >> On 03/08/2016 11:51 AM, Jan Beulich wrote: >>>>>> On 08.03.16 at 17:22, <dgdegra@tycho.nsa.gov> wrote: >>>> On 03/08/2016 04:46 AM, Jan Beulich wrote: >>>>>>>> On 07.03.16 at 19:42, <cardoe@cardoe.com> wrote: >>>>>> Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with >>>>>> CONFIG_ >>>>>> to use the Kconfig variable. >>>>> >>>>> Same question here: What's the benefit of doing it this way? >>>> >>>> This removes the stats tracking, which might (I have not tested) >>>> speed up >>>> the security server by avoiding the __get_cpu_var call and increment. >>> >>> No, I don not think the patch removes anything. The Kconfig option >>> doesn't have a prompt. But anyway, ... >> >> Ah, I missed that: I saw the --help-- line and assumed it was the prompt. >> Either way, this #define is a configuration-like knob that doesn't need to >> be hard-coded in a header as it currently is. >> >>> >>>> The >>>> corresponding SELinux knob is a Kconfig option in Linux. >>>> >>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> >>> >>> ... if you're fine with it, we'll put it in (once the mechanical issues >>> got addressed). >> > > Daniel, > > Would you like me to make this a real configuration option? Or proceed > with the current path and we can make a configuration option later? > I'll resubmit as is and we can address making that a user facing option in a follow on.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index d661da3..db23edc 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -23,6 +23,12 @@ config FLASK If unsure, say N. +config FLASK_AVC_STATS + def_bool y if FLASK + depends on FLASK + ---help--- + Maintain statistics on the access vector cache + # Select HAS_DEVICE_TREE if device tree is supported config HAS_DEVICE_TREE bool @@ -117,7 +123,7 @@ config XSM config XSM_MAGIC hex default 0xf97cff8c if FLASK - default 0 if !FLASK + default 0 ---help--- Identifies a FLASK XSM policy start point diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h index 3f8c53d..ef6e5ee 100644 --- a/xen/include/xen/config.h +++ b/xen/include/xen/config.h @@ -78,11 +78,6 @@ #define __STR(...) #__VA_ARGS__ #define STR(...) __STR(__VA_ARGS__) -#ifdef CONFIG_FLASK -/* Maintain statistics on the access vector cache */ -#define FLASK_AVC_STATS 1 -#endif - /* allow existing code to work with Kconfig variable */ #define NR_CPUS CONFIG_NR_CPUS diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c index 31bc702..7764379 100644 --- a/xen/xsm/flask/avc.c +++ b/xen/xsm/flask/avc.c @@ -56,7 +56,7 @@ const struct selinux_class_perm selinux_class_perm = { #define AVC_DEF_CACHE_THRESHOLD 512 #define AVC_CACHE_RECLAIM 16 -#ifdef FLASK_AVC_STATS +#ifdef CONFIG_FLASK_AVC_STATS #define avc_cache_stats_incr(field) \ do { \ __get_cpu_var(avc_cache_stats).field++; \ @@ -101,7 +101,7 @@ struct avc_callback_node { /* Exported via Flask hypercall */ unsigned int avc_cache_threshold = AVC_DEF_CACHE_THRESHOLD; -#ifdef FLASK_AVC_STATS +#ifdef CONFIG_FLASK_AVC_STATS DEFINE_PER_CPU(struct avc_cache_stats, avc_cache_stats); #endif diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c index f4f5dd1..3c9c99e 100644 --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -469,7 +469,7 @@ static int flask_security_make_bools(void) return ret; } -#ifdef FLASK_AVC_STATS +#ifdef CONFIG_FLASK_AVC_STATS static int flask_security_avc_cachestats(struct xen_flask_cache_stats *arg) { @@ -761,7 +761,7 @@ ret_t do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op) rv = avc_get_hash_stats(&op.u.hash_stats); break; -#ifdef FLASK_AVC_STATS +#ifdef CONFIG_FLASK_AVC_STATS case FLASK_AVC_CACHESTATS: rv = flask_security_avc_cachestats(&op.u.cache_stats); break; diff --git a/xen/xsm/flask/include/avc.h b/xen/xsm/flask/include/avc.h index 4283562..729856e 100644 --- a/xen/xsm/flask/include/avc.h +++ b/xen/xsm/flask/include/avc.h @@ -108,7 +108,7 @@ struct xen_flask_hash_stats; int avc_get_hash_stats(struct xen_flask_hash_stats *arg); extern unsigned int avc_cache_threshold; -#ifdef FLASK_AVC_STATS +#ifdef CONFIG_FLASK_AVC_STATS DECLARE_PER_CPU(struct avc_cache_stats, avc_cache_stats); #endif
Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ to use the Kconfig variable. Signed-off-by: Doug Goldstein <cardoe@cardoe.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- xen/common/Kconfig | 8 +++++++- xen/include/xen/config.h | 5 ----- xen/xsm/flask/avc.c | 4 ++-- xen/xsm/flask/flask_op.c | 4 ++-- xen/xsm/flask/include/avc.h | 2 +- 5 files changed, 12 insertions(+), 11 deletions(-)