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

[Etienne Basset <etienne.basset@numericable.fr>]

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