[selinux-next] selinux: Annotate lockdep for services locks

Commit Message

Peter Enderborg Feb. 19, 2018, 3:18 p.m. UTC

From: Peter <[e-mail]>

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 <[e-mail]>
---
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(+)

Comments

Stephen Smalley Feb. 20, 2018, 1:59 p.m. UTC | #1

On Mon, 2018-02-19 at 16:18 +0100, Peter Enderborg wrote:
> From: Peter <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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?

Stephen Smalley Feb. 20, 2018, 3:58 p.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> > 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?

Peter Enderborg Feb. 21, 2018, 9:31 a.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>> 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.

Patch

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;
 }