On 02/20/2018 04:58 PM, Stephen Smalley wrote:
> On Tue, 2018-02-20 at 08:59 -0500, Stephen Smalley wrote:
>> On Mon, 2018-02-19 at 16:18 +0100, Peter Enderborg wrote:
>>> From: Peter <peter.enderborg@xxxxxxxx>
>>>
>>> The locks are moved to dynamic allocation, we need to
>>> help the lockdep system to classify the locks.
>>> This adds to lockdep annotation for the page mutex and
>>> for the ss lock.
>>>
>>> Signed-off-by: Peter Enderborg <peter.enderborg@xxxxxxxx>
>>> ---
>>> This is the rebase of suggested patches from selinuxns tree
>>> and are intended to be applyed on top of:
>>> selinux: wrap global selinux state
>>> from Stephen Smalley
>>>
>>> security/selinux/ss/services.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/security/selinux/ss/services.c
>>> b/security/selinux/ss/services.c
>>> index 3698352213d7..a741552e22b5 100644
>>> --- a/security/selinux/ss/services.c
>>> +++ b/security/selinux/ss/services.c
>>> @@ -81,11 +81,15 @@ char
>>> *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
>>> };
>>>
>>> static struct selinux_ss selinux_ss;
>>> +static struct lock_class_key selinux_ss_class_key;
>>> +static struct lock_class_key selinux_status_class_key;
>>>
>>> void selinux_ss_init(struct selinux_ss **ss)
>>> {
>>> rwlock_init(&selinux_ss.policy_rwlock);
>>> + lockdep_set_class(&selinux_ss.policy_rwlock,
>>> &selinux_ss_class_key);
>>> mutex_init(&selinux_ss.status_lock);
>>> + lockdep_set_class(&selinux_ss.status_lock,
>>> &selinux_status_class_key);
>>> *ss = &selinux_ss;
>>> }
>> Pardon my ignorance, but can you explain why we need an explicit call
>> to lockdep_set_class() here? I see it used for e.g. the inode
>> i_lock,
>> but there the class is per-file_system_type. It doesn't seem to be
>> always be used for all locks when they are dynamically initialized or
>> allocated, e.g. get_empty_filp does not call lockdep_set_class() for
>> struct file's f_owner.lock or f_lock even though they are dynamically
>> allocated and initialized. What makes this case different?
> Also, your explanation in the patch description was because the locks
> are moved to dynamic allocation. That was true of the original selinux
> namespace patch. But it isn't true for the wrap global selinux state
> patch; selinux_ss is statically allocated and there is only one
> instance of it in this patch. So do we need this lockdep annotation
> yet?
>
>
I think you are right. I dont get any warnings whey trying to use them, and lockdep
get a useful name for them.
