Re: [PATCH v2 02/10] xsm: refactor xsm_ops handling

[Jan Beulich <jbeulich@suse.com>]

On 13.07.2021 01:39, Andrew Cooper wrote:
> On 12/07/2021 21:32, Daniel P. Smith wrote:
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index ad3cddbf7d..a8805f514b 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -747,16 +747,14 @@ extern int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size);
>>  extern bool has_xsm_magic(paddr_t);
>>  #endif
>>  
>> -extern int register_xsm(struct xsm_operations *ops);
>> -
>> -extern struct xsm_operations dummy_xsm_ops;
>>  extern void xsm_fixup_ops(struct xsm_operations *ops);
>>  
>>  #ifdef CONFIG_XSM_FLASK
>> -extern void flask_init(const void *policy_buffer, size_t policy_size);
>> +extern struct xsm_operations *flask_init(const void *policy_buffer, size_t policy_size);
>>  #else
>> -static inline void flask_init(const void *policy_buffer, size_t policy_size)
>> +static inline struct xsm_operations *flask_init(const void *policy_buffer, size_t policy_size)
>>  {
>> +    return NULL;
>>  }
>>  #endif
> 
> As you touch almost every current user of xsm_operations and introduce
> quite a few more, can I recommend taking the opportunity to shorten the
> name to struct xsm_ops.

+1

>> --- a/xen/xsm/flask/flask_op.c
>> +++ b/xen/xsm/flask/flask_op.c
>> @@ -226,6 +226,7 @@ static int flask_security_sid(struct xen_flask_sid_context *arg)
>>  static int flask_disable(void)
>>  {
>>      static int flask_disabled = 0;
>> +    struct xsm_operations default_ops;
>>  
>>      if ( ss_initialized )
>>      {
>> @@ -244,7 +245,8 @@ static int flask_disable(void)
>>      flask_disabled = 1;
>>  
>>      /* Reset xsm_ops to the original module. */
>> -    xsm_ops = &dummy_xsm_ops;
>> +    xsm_fixup_ops(&default_ops);
>> +    xsm_ops = default_ops;
>>  
>>      return 0;
>>  }
> 
> These two hunks will disappear when patch 3 is reordered with respect to
> this one.
> 
> ... which is good because you've just pointed xsm_ops at a
> soon-to-be-out-of-scope local variable on the stack.

Not really, it's a structure copy now. What I'm concerned about is
the size of that on-stack variable and its lack of an initializer,
undermining the many set_to_dummy_if_null() that xsm_fixup_ops()
uses. Can't xsm_fixup_ops() act on xsm_ops directly, perhaps after
memset()-ing it here first (if nothing else then just to be on the
safe side)?

>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index f1a1217c98..a3a88aa8ed 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1883,7 +1883,8 @@ static struct xsm_operations flask_ops = {
>>  #endif
>>  };
> 
> flask and silo ops should become:
> 
> static const struct xsm_ops __initconst {flask,silo}_ops = {
> 
> as they're neither modified, nor needed after init, following this change.
> 
>>  
>> -void __init flask_init(const void *policy_buffer, size_t policy_size)
>> +struct xsm_operations __init *flask_init(const void *policy_buffer,
>> +					 size_t policy_size)
> 
> struct xsm_ops *__init flask_init(...)
> 
> Otherwise you've got the __init in the middle of the return type, and
> some compilers are more picky than others.

Also, even if {flask,silo}_ops couldn't be const for some reason,
these init functions now want to return a pointer-to-const, as
the caller isn't supposed to modify the struct-s any further.

Jan