Re: [PATCH 2/2] security/smack implement logging V2

From: Etienne Basset
Date: Mon Apr 06 2009 - 02:17:55 EST


Casey Schaufler wrote:
> Etienne Basset wrote:
>>> It looks like the only difference between these are their non-logging
>>> versions is the logging. I say go ahead and add the auditdata parameter
>>> to smk_access and smk_curacc and allow for the case where it is NULL.
>>>
>>>
>> i would prefer keeping a logging and a non-logging variant
>> maybe rename smk_access & smk_curacc to smk_access_nolog & smk_curacc_nolog
>> Or you really prefer that we pass a NULL when we want the non-logging?
>> I guess its a matter of taste, so i let you decide :)
>>
>
> I dislike functions that do nothing but set up another function.
>
>
fair enough

>>> It would be nice if audit data was only manipulated if audit is
>>> installed, but I don't like the idea of ifdeffing everything
>>> very much either. How about a static inline in smack.h that is
>>> ifdeffed for audit? smack_audit_init? There would need to be one
>>> for each field, too.
>>>
>>>
>> why not, but instead of a "initialiser" for each field i'll prefer a macro
>> To save some stack we could also conditionnaly define the "common_audit_data"
>>
>> something like :
>>
>> #ifdef CONFIG_AUDIT
>> #define DEFINE_SMACK_AUDIT_DATA(ad) struct common_audit_data ad
>> void smack_audit_init(..) {
>> COMMON_AUDIT_DATA_INIT(...)
>> }
>> #define SMACK_AUDIT_DATA_SET_FIELD(_ad, _field, _value) (_ad._field = _value)
>> ...
>> #else
>> #define DEFINE_SMACK_AUDIT_DATA(ad)
>> void smack_audit_init(..)
>> {
>> }
>> #define SMACK_AUDIT_DATA_SET_FIELD(_ad, _field, _value)
>> #endif
>>
>
> I'm not convinced. #defines are good for assigning names to constants,
> but I saw the original Bourne shell code, where defines were used to
> make the code look like ALGOL68, so you have to overcome the fear that
> was instilled in me when I was young.
>
>
hum... it's already everywhere :)
to save stack space in the NON-AUDIT case, maybe something like :

struct smk_audit_data {
#ifdef CONFIG_AUDIT
struct common_audit_data;
#endif
}

(which i don't really like either, but we have to find a compromise between
"bloat" in the NON-AUDIT case and clean-looking code)

sure, i could make the "initialisers" static inline, the only downside is more C code
(and more homework for me ;) )

>
>>> Now this is tricky. You'll get an audit record for single-label
>>> hosts, but not for those that use CIPSO. The former is good, the
>>> latter is bad.
>>>
>>>
>> gargll you're right
>>
>
> I love those french idioms.
>
:)

> Actually, I don't think there's much point in worrying about it.
> The audit you do here is correct, and until you can get more subject
> information over the wire there isn't much point doing it on the
> receiving end.
>
ok
thanks
Etienne
--
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/