Re: [PATCH][SMACK] convert smack rule list to linux list

Re: [PATCH][SMACK] convert smack rule list to linux list

[Tetsuo Handa]

etienne wrote:
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index 2e0b83e..3dc312d 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -87,7 +87,6 @@ static u32 smack_next_secid = 10;
>  int smk_access(char *subject_label, char *object_label, int request)
>  {
>  	u32 may = MAY_NOT;
> -	struct smk_list_entry *sp;
>  	struct smack_rule *srp;
>  
>  	/*
> @@ -139,8 +138,8 @@ int smk_access(char *subject_label, char *object_label, int request)
>  	 * access (e.g. read is included in readwrite) it's
>  	 * good.
>  	 */
> -	for (sp = smack_list; sp != NULL; sp = sp->smk_next) {
> -		srp = &sp->smk_rule;
> +
> +	list_for_each_entry(srp, &smack_rule_list, list) {
>  
>  		if (srp->smk_subject == subject_label ||
>  		    strcmp(srp->smk_subject, subject_label) == 0) {

Use of standard doubly linked list requires a lock, doesn't it?
What lock protects smack_rule_list?

[PATCH][SMACK] convert smack rule list to linux list

[etienne]

Hello,

this patch convert the  smack_list list to standard linux list, struct smk_list_entry is also removed
(list_head is added directly in smack_rule, smk_list_entry has no practical use?)
this patch applies on top of 
[PATCH][SMACK][RFC] convert smack_netlbladdrs to standard list

Please have a more detailed look at smk_write_load

I'm running witch smack enabled, everything that should be allowed is still allowed, everything
that's should be denied is still denied, and i don't oops. :)

regards
Etienne 


Signed-off-by: Etienne Basset <etienne.basset@numericable.fr>
---
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 0b21ccd..d0b2646 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -59,18 +59,10 @@ struct inode_smack {
  * A label access rule.
  */
 struct smack_rule {
-	char	*smk_subject;
-	char	*smk_object;
-	int	smk_access;
-};
-
-/*
- * An entry in the table of permitted label accesses.
- */
-struct smk_list_entry {
-	struct smk_list_entry	*smk_next;
 	struct list_head	list;
-	struct smack_rule	smk_rule;
+	char			*smk_subject;
+	char			*smk_object;
+	int			smk_access;
 };
 
 /*
@@ -216,10 +208,10 @@ extern struct smack_known smack_known_invalid;
 extern struct smack_known smack_known_star;
 extern struct smack_known smack_known_web;
 
-extern struct smk_list_entry *smack_list;
 extern struct list_head smack_know_list;
 extern struct list_head smack_rule_list;
 extern struct list_head smk_netlbladdr_list;
+
 extern struct security_operations smack_ops;
 
 /*
diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 2e0b83e..3dc312d 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -87,7 +87,6 @@ static u32 smack_next_secid = 10;
 int smk_access(char *subject_label, char *object_label, int request)
 {
 	u32 may = MAY_NOT;
-	struct smk_list_entry *sp;
 	struct smack_rule *srp;
 
 	/*
@@ -139,8 +138,8 @@ int smk_access(char *subject_label, char *object_label, int request)
 	 * access (e.g. read is included in readwrite) it's
 	 * good.
 	 */
-	for (sp = smack_list; sp != NULL; sp = sp->smk_next) {
-		srp = &sp->smk_rule;
+
+	list_for_each_entry(srp, &smack_rule_list, list) {
 
 		if (srp->smk_subject == subject_label ||
 		    strcmp(srp->smk_subject, subject_label) == 0) {
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 876ab91..a5fbca5 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -82,9 +82,10 @@ char *smack_onlycap;
  */
 
 LIST_HEAD(smk_netlbladdr_list);
+LIST_HEAD(smack_rule_list);
+
 
 static int smk_cipso_doi_value = SMACK_CIPSO_DOI_DEFAULT;
-struct smk_list_entry *smack_list;
 
 #define	SEQ_READ_FINISHED	1
 
@@ -135,24 +136,27 @@ static void *load_seq_start(struct seq_file *s, loff_t *pos)
 {
 	if (*pos == SEQ_READ_FINISHED)
 		return NULL;
-
-	return smack_list;
+	if (list_empty(&smack_rule_list))
+		return NULL;
+	return &smack_rule_list;
 }
 
 static void *load_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
-	struct smk_list_entry *skp = ((struct smk_list_entry *) v)->smk_next;
+	struct list_head *list = v;
 
-	if (skp == NULL)
+	if (list_is_last(list->next, &smack_rule_list)) {
 		*pos = SEQ_READ_FINISHED;
-
-	return skp;
+		return NULL;
+	}
+	return list->next;
 }
 
 static int load_seq_show(struct seq_file *s, void *v)
 {
-	struct smk_list_entry *slp = (struct smk_list_entry *) v;
-	struct smack_rule *srp = &slp->smk_rule;
+	struct list_head *list = v;
+	struct smack_rule *srp =
+		 container_of(list->next, struct smack_rule, list);
 
 	seq_printf(s, "%s %s", (char *)srp->smk_subject,
 		   (char *)srp->smk_object);
@@ -213,32 +217,23 @@ static int smk_open_load(struct inode *inode, struct file *file)
  */
 static int smk_set_access(struct smack_rule *srp)
 {
-	struct smk_list_entry *sp;
-	struct smk_list_entry *newp;
+	struct smack_rule *sp;
 	int ret = 0;
-
+	int found;
 	mutex_lock(&smack_list_lock);
 
-	for (sp = smack_list; sp != NULL; sp = sp->smk_next)
-		if (sp->smk_rule.smk_subject == srp->smk_subject &&
-		    sp->smk_rule.smk_object == srp->smk_object) {
-			sp->smk_rule.smk_access = srp->smk_access;
+	found = 0;
+	list_for_each_entry(sp, &smack_rule_list, list) {
+		if (sp->smk_subject == srp->smk_subject &&
+		    sp->smk_object == srp->smk_object) {
+			found = 1;
+			sp->smk_access = srp->smk_access;
 			break;
 		}
-
-	if (sp == NULL) {
-		newp = kzalloc(sizeof(struct smk_list_entry), GFP_KERNEL);
-		if (newp == NULL) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		newp->smk_rule = *srp;
-		newp->smk_next = smack_list;
-		smack_list = newp;
 	}
+	if (found == 0)
+		list_add(&srp->list, &smack_rule_list);
 
-out:
 	mutex_unlock(&smack_list_lock);
 
 	return ret;
@@ -262,7 +257,7 @@ out:
 static ssize_t smk_write_load(struct file *file, const char __user *buf,
 			      size_t count, loff_t *ppos)
 {
-	struct smack_rule rule;
+	struct smack_rule *rule;
 	char *data;
 	int rc = -EINVAL;
 
@@ -273,9 +268,8 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 	 */
 	if (!capable(CAP_MAC_ADMIN))
 		return -EPERM;
-	if (*ppos != 0)
-		return -EINVAL;
-	if (count != SMK_LOADLEN)
+
+	if (*ppos != 0 || count != SMK_LOADLEN)
 		return -EINVAL;
 
 	data = kzalloc(count, GFP_KERNEL);
@@ -287,25 +281,31 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 		goto out;
 	}
 
-	rule.smk_subject = smk_import(data, 0);
-	if (rule.smk_subject == NULL)
+	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
+	if (rule == NULL) {
+		rc = -ENOMEM;
 		goto out;
+	}
 
-	rule.smk_object = smk_import(data + SMK_LABELLEN, 0);
-	if (rule.smk_object == NULL)
-		goto out;
+	rule->smk_subject = smk_import(data, 0);
+	if (rule->smk_subject == NULL)
+		goto out_free;
 
-	rule.smk_access = 0;
+	rule->smk_object = smk_import(data + SMK_LABELLEN, 0);
+	if (rule->smk_object == NULL)
+		goto out_free;
+
+	rule->smk_access = 0;
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN]) {
 	case '-':
 		break;
 	case 'r':
 	case 'R':
-		rule.smk_access |= MAY_READ;
+		rule->smk_access |= MAY_READ;
 		break;
 	default:
-		goto out;
+		goto out_free;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 1]) {
@@ -313,10 +313,10 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 		break;
 	case 'w':
 	case 'W':
-		rule.smk_access |= MAY_WRITE;
+		rule->smk_access |= MAY_WRITE;
 		break;
 	default:
-		goto out;
+		goto out_free;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 2]) {
@@ -324,10 +324,10 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 		break;
 	case 'x':
 	case 'X':
-		rule.smk_access |= MAY_EXEC;
+		rule->smk_access |= MAY_EXEC;
 		break;
 	default:
-		goto out;
+		goto out_free;
 	}
 
 	switch (data[SMK_LABELLEN + SMK_LABELLEN + 3]) {
@@ -335,17 +335,20 @@ static ssize_t smk_write_load(struct file *file, const char __user *buf,
 		break;
 	case 'a':
 	case 'A':
-		rule.smk_access |= MAY_APPEND;
+		rule->smk_access |= MAY_APPEND;
 		break;
 	default:
-		goto out;
+		goto out_free;
 	}
 
-	rc = smk_set_access(&rule);
+	rc = smk_set_access(rule);
 
 	if (!rc)
 		rc = count;
+	goto out;
 
+out_free:
+	kfree(rule);
 out:
 	kfree(data);
 	return rc;

Re: [PATCH][SMACK] convert smack rule list to linux list

[Tetsuo Handa]

Paul, would you review this locking?

> static DEFINE_MUTEX(smack_known_lock);
> 
> /**
>  * smk_import_entry - import a label, return the list entry
>  * @string: a text string that might be a Smack label
>  * @len: the maximum size, or zero if it is NULL terminated.
>  *
>  * Returns a pointer to the entry in the label list that
>  * matches the passed string, adding it if necessary.
>  */
> struct smack_known *smk_import_entry(const char *string, int len)
> {
> 	struct smack_known *skp;
> 	char smack[SMK_LABELLEN];
> 	int found;
> 	int i;
> 
> 	if (len <= 0 || len > SMK_MAXLEN)
> 		len = SMK_MAXLEN;
> 
> 	for (i = 0, found = 0; i < SMK_LABELLEN; i++) {
> 		if (found)
> 			smack[i] = '\0';
> 		else if (i >= len || string[i] > '~' || string[i] <= ' ' ||
> 			 string[i] == '/') {
> 			smack[i] = '\0';
> 			found = 1;
> 		} else
> 			smack[i] = string[i];
> 	}
> 
> 	if (smack[0] == '\0')
> 		return NULL;
> 
> 	mutex_lock(&smack_known_lock);
> 
> 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
> 		if (strncmp(skp->smk_known, smack, SMK_MAXLEN) == 0)
> 			break;
> 
> 	if (skp == NULL) {
> 		skp = kzalloc(sizeof(struct smack_known), GFP_KERNEL);
> 		if (skp != NULL) {
> 			skp->smk_next = smack_known;
> 			strncpy(skp->smk_known, smack, SMK_MAXLEN);
> 			skp->smk_secid = smack_next_secid++;
> 			skp->smk_cipso = NULL;
> 			spin_lock_init(&skp->smk_cipsolock);
> 			/*
> 			 * Make sure that the entry is actually
> 			 * filled before putting it on the list.
> 			 */
> 			smp_mb();
> 			smack_known = skp;
> 		}
> 	}
> 
> 	mutex_unlock(&smack_known_lock);
> 
> 	return skp;
> }
> 
> /**
>  * smack_from_secid - find the Smack label associated with a secid
>  * @secid: an integer that might be associated with a Smack label
>  *
>  * Returns a pointer to the appropraite Smack label if there is one,
>  * otherwise a pointer to the invalid Smack label.
>  */
> char *smack_from_secid(const u32 secid)
> {
> 	struct smack_known *skp;
> 
> 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
> 		if (skp->smk_secid == secid)
> 			return skp->smk_known;
> 
> 	/*
> 	 * If we got this far someone asked for the translation
> 	 * of a secid that is not on the list.
> 	 */
> 	return smack_known_invalid.smk_known;
> }

I think this is a case called "dependency ordering".
This function needs rcu_dereference(), doesn't it?

Regards.

Re: [PATCH][SMACK] convert smack rule list to linux list

[etienne]

Tetsuo Handa wrote:
> etienne wrote:
>> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
>> index 2e0b83e..3dc312d 100644
>> --- a/security/smack/smack_access.c
>> +++ b/security/smack/smack_access.c
>> @@ -87,7 +87,6 @@ static u32 smack_next_secid = 10;
>>  int smk_access(char *subject_label, char *object_label, int request)
>>  {
>>  	u32 may = MAY_NOT;
>> -	struct smk_list_entry *sp;
>>  	struct smack_rule *srp;
>>  
>>  	/*
>> @@ -139,8 +138,8 @@ int smk_access(char *subject_label, char *object_label, int request)
>>  	 * access (e.g. read is included in readwrite) it's
>>  	 * good.
>>  	 */
>> -	for (sp = smack_list; sp != NULL; sp = sp->smk_next) {
>> -		srp = &sp->smk_rule;
>> +
>> +	list_for_each_entry(srp, &smack_rule_list, list) {
>>  
>>  		if (srp->smk_subject == subject_label ||
>>  		    strcmp(srp->smk_subject, subject_label) == 0) {
> 
> Use of standard doubly linked list requires a lock, doesn't it?
> What lock protects smack_rule_list?
> 
you're right; 

what's the best way, using a rcu variant for "list_for_each, container_of ...etc" ?
(concurrent list insertion are already protected with a mutex, so rcu must the good idea for the read side) 


thanks,
Etienne

Re: [PATCH][SMACK] convert smack rule list to linux list

[Tetsuo Handa]

etienne wrote:
> what's the best way, using a rcu variant for "list_for_each, container_of ...etc" ?
> (concurrent list insertion are already protected with a mutex, so rcu must the good idea for the read side) 
Read side will need a read lock.
I think you need to change from "mutex" to "rw_semaphore" (on the assumption
that SMACK is safe against read locks that may sleep.  If not, change from
"mutex" to "rw_spinlock").

Regards.

Re: [PATCH][SMACK] convert smack rule list to linux list

[etienne]

Tetsuo Handa wrote:
> etienne wrote:
>> what's the best way, using a rcu variant for "list_for_each, container_of ...etc" ?
>> (concurrent list insertion are already protected with a mutex, so rcu must the good idea for the read side) 
> Read side will need a read lock.
> I think you need to change from "mutex" to "rw_semaphore" (on the assumption
> that SMACK is safe against read locks that may sleep.  If not, change from
> "mutex" to "rw_spinlock").
> 
> Regards.
> 
or a rcu_read_lock?
since it disables preemption it means no user process will be able to change the list while it's read?

regards,
Etienne

 

Re: [PATCH][SMACK] convert smack rule list to linux list

[Paul E. McKenney]

On Sun, Feb 22, 2009 at 10:13:49PM +0900, Tetsuo Handa wrote:
> Paul, would you review this locking?
> 
> > static DEFINE_MUTEX(smack_known_lock);
> > 
> > /**
> >  * smk_import_entry - import a label, return the list entry
> >  * @string: a text string that might be a Smack label
> >  * @len: the maximum size, or zero if it is NULL terminated.
> >  *
> >  * Returns a pointer to the entry in the label list that
> >  * matches the passed string, adding it if necessary.
> >  */
> > struct smack_known *smk_import_entry(const char *string, int len)
> > {
> > 	struct smack_known *skp;
> > 	char smack[SMK_LABELLEN];
> > 	int found;
> > 	int i;
> > 
> > 	if (len <= 0 || len > SMK_MAXLEN)
> > 		len = SMK_MAXLEN;
> > 
> > 	for (i = 0, found = 0; i < SMK_LABELLEN; i++) {
> > 		if (found)
> > 			smack[i] = '\0';
> > 		else if (i >= len || string[i] > '~' || string[i] <= ' ' ||
> > 			 string[i] == '/') {
> > 			smack[i] = '\0';
> > 			found = 1;
> > 		} else
> > 			smack[i] = string[i];
> > 	}
> > 
> > 	if (smack[0] == '\0')
> > 		return NULL;
> > 
> > 	mutex_lock(&smack_known_lock);
> > 
> > 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
> > 		if (strncmp(skp->smk_known, smack, SMK_MAXLEN) == 0)
> > 			break;
> > 
> > 	if (skp == NULL) {
> > 		skp = kzalloc(sizeof(struct smack_known), GFP_KERNEL);
> > 		if (skp != NULL) {
> > 			skp->smk_next = smack_known;
> > 			strncpy(skp->smk_known, smack, SMK_MAXLEN);
> > 			skp->smk_secid = smack_next_secid++;
> > 			skp->smk_cipso = NULL;
> > 			spin_lock_init(&skp->smk_cipsolock);
> > 			/*
> > 			 * Make sure that the entry is actually
> > 			 * filled before putting it on the list.
> > 			 */
> > 			smp_mb();
> > 			smack_known = skp;

If the read side is not acquiring smack_known_lock, then the above
assignment to smack_known needs to be:

			rcu_assign_pointer(smack_known, skp);

Otherwise, both CPU and compiler are within their rights to reorder
the assignment to smack_known ahead of the initialization code.

Alternatively, if you make this list use a standard struct list_head,
you could just use list_add_rcu().

> > 		}
> > 	}
> > 
> > 	mutex_unlock(&smack_known_lock);
> > 
> > 	return skp;
> > }
> > 
> > /**
> >  * smack_from_secid - find the Smack label associated with a secid
> >  * @secid: an integer that might be associated with a Smack label
> >  *
> >  * Returns a pointer to the appropraite Smack label if there is one,
> >  * otherwise a pointer to the invalid Smack label.
> >  */
> > char *smack_from_secid(const u32 secid)
> > {
> > 	struct smack_known *skp;
> > 
> > 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
> > 		if (skp->smk_secid == secid)
> > 			return skp->smk_known;
> > 
> > 	/*
> > 	 * If we got this far someone asked for the translation
> > 	 * of a secid that is not on the list.
> > 	 */
> > 	return smack_known_invalid.smk_known;
> > }
> 
> I think this is a case called "dependency ordering".
> This function needs rcu_dereference(), doesn't it?

Indeed!  The "for" loop needs to be:

	for (skp = rcu_dereference(smack_known); skp != NULL; skp = rcu_dereference(skp->smk_next))

Alternatively, if you switch to struct list_head, you could use
list_for_each_entry_rcu().

There also need to be rcu_read_lock() and rcu_read_unlock() in here
somewhere.  Where they must be depends on how (or whether) you are
ever removing any elements.  If the string referenced by smk_known
gets freed up, then the caller will need to surround the call to
smack_from_secid() and the use of the return value with rcu_read_lock()
and rcu_read_unlock().  Otherwise, only the smack_known structures are
ever freed up, then just the "for" loop above needs to be so protected.

If these structure are never freed, then please add a comment.

							Thanx, Paul

Re: [PATCH][SMACK] convert smack rule list to linux list

[Paul E. McKenney]

On Sun, Feb 22, 2009 at 02:14:38PM +0100, etienne wrote:
> Tetsuo Handa wrote:
> > etienne wrote:
> >> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> >> index 2e0b83e..3dc312d 100644
> >> --- a/security/smack/smack_access.c
> >> +++ b/security/smack/smack_access.c
> >> @@ -87,7 +87,6 @@ static u32 smack_next_secid = 10;
> >>  int smk_access(char *subject_label, char *object_label, int request)
> >>  {
> >>  	u32 may = MAY_NOT;
> >> -	struct smk_list_entry *sp;
> >>  	struct smack_rule *srp;
> >>  
> >>  	/*
> >> @@ -139,8 +138,8 @@ int smk_access(char *subject_label, char *object_label, int request)
> >>  	 * access (e.g. read is included in readwrite) it's
> >>  	 * good.
> >>  	 */
> >> -	for (sp = smack_list; sp != NULL; sp = sp->smk_next) {
> >> -		srp = &sp->smk_rule;
> >> +
> >> +	list_for_each_entry(srp, &smack_rule_list, list) {
> >>  
> >>  		if (srp->smk_subject == subject_label ||
> >>  		    strcmp(srp->smk_subject, subject_label) == 0) {
> > 
> > Use of standard doubly linked list requires a lock, doesn't it?
> > What lock protects smack_rule_list?
> > 
> you're right; 
> 
> what's the best way, using a rcu variant for "list_for_each, container_of ...etc" ?
> (concurrent list insertion are already protected with a mutex, so rcu must the good idea for the read side) 

You want list_for_each_entry_rcu() above.  You will need list_add_rcu()
when adding elements to the list.

Again, if these elements are ever removed, you will need rcu_read_lock()
and rcu_read_unlock() surrounding their use.  Otherwise, an element can
be freed out from under a reader who is still referencing it.

							Thanx, Paul

Re: [PATCH][SMACK] convert smack rule list to linux list

[etienne]

Paul E. McKenney wrote:
> On Sun, Feb 22, 2009 at 10:13:49PM +0900, Tetsuo Handa wrote:
>> Paul, would you review this locking?
>>
>>> static DEFINE_MUTEX(smack_known_lock);
>>>
>>> /**
>>>  * smk_import_entry - import a label, return the list entry
>>>  * @string: a text string that might be a Smack label
>>>  * @len: the maximum size, or zero if it is NULL terminated.
>>>  *
>>>  * Returns a pointer to the entry in the label list that
>>>  * matches the passed string, adding it if necessary.
>>>  */
>>> struct smack_known *smk_import_entry(const char *string, int len)
>>> {
>>> 	struct smack_known *skp;
>>> 	char smack[SMK_LABELLEN];
>>> 	int found;
>>> 	int i;
>>>
>>> 	if (len <= 0 || len > SMK_MAXLEN)
>>> 		len = SMK_MAXLEN;
>>>
>>> 	for (i = 0, found = 0; i < SMK_LABELLEN; i++) {
>>> 		if (found)
>>> 			smack[i] = '\0';
>>> 		else if (i >= len || string[i] > '~' || string[i] <= ' ' ||
>>> 			 string[i] == '/') {
>>> 			smack[i] = '\0';
>>> 			found = 1;
>>> 		} else
>>> 			smack[i] = string[i];
>>> 	}
>>>
>>> 	if (smack[0] == '\0')
>>> 		return NULL;
>>>
>>> 	mutex_lock(&smack_known_lock);
>>>
>>> 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
>>> 		if (strncmp(skp->smk_known, smack, SMK_MAXLEN) == 0)
>>> 			break;
>>>
>>> 	if (skp == NULL) {
>>> 		skp = kzalloc(sizeof(struct smack_known), GFP_KERNEL);
>>> 		if (skp != NULL) {
>>> 			skp->smk_next = smack_known;
>>> 			strncpy(skp->smk_known, smack, SMK_MAXLEN);
>>> 			skp->smk_secid = smack_next_secid++;
>>> 			skp->smk_cipso = NULL;
>>> 			spin_lock_init(&skp->smk_cipsolock);
>>> 			/*
>>> 			 * Make sure that the entry is actually
>>> 			 * filled before putting it on the list.
>>> 			 */
>>> 			smp_mb();
>>> 			smack_known = skp;
> 
> If the read side is not acquiring smack_known_lock, then the above
> assignment to smack_known needs to be:
> 
> 			rcu_assign_pointer(smack_known, skp);
> 
> Otherwise, both CPU and compiler are within their rights to reorder
> the assignment to smack_known ahead of the initialization code.
> 
> Alternatively, if you make this list use a standard struct list_head,
> you could just use list_add_rcu().
>
that's what i was going to do ;)

 
>>> 		}
>>> 	}
>>>
>>> 	mutex_unlock(&smack_known_lock);
>>>
>>> 	return skp;
>>> }
>>>
>>> /**
>>>  * smack_from_secid - find the Smack label associated with a secid
>>>  * @secid: an integer that might be associated with a Smack label
>>>  *
>>>  * Returns a pointer to the appropraite Smack label if there is one,
>>>  * otherwise a pointer to the invalid Smack label.
>>>  */
>>> char *smack_from_secid(const u32 secid)
>>> {
>>> 	struct smack_known *skp;
>>>
>>> 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
>>> 		if (skp->smk_secid == secid)
>>> 			return skp->smk_known;
>>>
>>> 	/*
>>> 	 * If we got this far someone asked for the translation
>>> 	 * of a secid that is not on the list.
>>> 	 */
>>> 	return smack_known_invalid.smk_known;
>>> }
>> I think this is a case called "dependency ordering".
>> This function needs rcu_dereference(), doesn't it?
> 
> Indeed!  The "for" loop needs to be:
> 
> 	for (skp = rcu_dereference(smack_known); skp != NULL; skp = rcu_dereference(skp->smk_next))
> 
> Alternatively, if you switch to struct list_head, you could use
> list_for_each_entry_rcu().
> 
> There also need to be rcu_read_lock() and rcu_read_unlock() in here
> somewhere.  Where they must be depends on how (or whether) you are
> ever removing any elements.  If the string referenced by smk_known
> gets freed up, then the caller will need to surround the call to
> smack_from_secid() and the use of the return value with rcu_read_lock()
> and rcu_read_unlock().  Otherwise, only the smack_known structures are
> ever freed up, then just the "for" loop above needs to be so protected.
> 
> If these structure are never freed, then please add a comment.
> 

for the time being there are not freed; but if think it's safer to add the 
"rcu_read_lock()/rcu_read_unlock()" anyway (in case someone want to implement a del in the future)
I don't think they are any downside?

thanks for the explanations!

Re: [PATCH][SMACK] convert smack rule list to linux list

[Casey Schaufler]

Paul E. McKenney wrote:
> On Sun, Feb 22, 2009 at 02:14:38PM +0100, etienne wrote:
>   
>> Tetsuo Handa wrote:
>>     
>>> etienne wrote:
>>>       
>>>> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
>>>> index 2e0b83e..3dc312d 100644
>>>> --- a/security/smack/smack_access.c
>>>> +++ b/security/smack/smack_access.c
>>>> @@ -87,7 +87,6 @@ static u32 smack_next_secid = 10;
>>>>  int smk_access(char *subject_label, char *object_label, int request)
>>>>  {
>>>>  	u32 may = MAY_NOT;
>>>> -	struct smk_list_entry *sp;
>>>>  	struct smack_rule *srp;
>>>>  
>>>>  	/*
>>>> @@ -139,8 +138,8 @@ int smk_access(char *subject_label, char *object_label, int request)
>>>>  	 * access (e.g. read is included in readwrite) it's
>>>>  	 * good.
>>>>  	 */
>>>> -	for (sp = smack_list; sp != NULL; sp = sp->smk_next) {
>>>> -		srp = &sp->smk_rule;
>>>> +
>>>> +	list_for_each_entry(srp, &smack_rule_list, list) {
>>>>  
>>>>  		if (srp->smk_subject == subject_label ||
>>>>  		    strcmp(srp->smk_subject, subject_label) == 0) {
>>>>         
>>> Use of standard doubly linked list requires a lock, doesn't it?
>>> What lock protects smack_rule_list?
>>>
>>>       
>> you're right; 
>>
>> what's the best way, using a rcu variant for "list_for_each, container_of ...etc" ?
>> (concurrent list insertion are already protected with a mutex, so rcu must the good idea for the read side) 
>>     
>
> You want list_for_each_entry_rcu() above.  You will need list_add_rcu()
> when adding elements to the list.
>
> Again, if these elements are ever removed, you will need rcu_read_lock()
> and rcu_read_unlock() surrounding their use.  Otherwise, an element can
> be freed out from under a reader who is still referencing it.
>
> 							Thanx, Paul
>   

You'll also need to be very careful that the locking is safe to use
in the networking hooks, in particular smack_socket_sock_rcv_skb. The
amount of care required to get the locking correct is a major factor
in the current list implementation.

Re: [PATCH][SMACK] convert smack rule list to linux list

[Paul E. McKenney]

On Sun, Feb 22, 2009 at 05:30:08PM +0100, etienne wrote:
> Paul E. McKenney wrote:
> > On Sun, Feb 22, 2009 at 10:13:49PM +0900, Tetsuo Handa wrote:
> >> Paul, would you review this locking?
> >>
> >>> static DEFINE_MUTEX(smack_known_lock);
> >>>
> >>> /**
> >>>  * smk_import_entry - import a label, return the list entry
> >>>  * @string: a text string that might be a Smack label
> >>>  * @len: the maximum size, or zero if it is NULL terminated.
> >>>  *
> >>>  * Returns a pointer to the entry in the label list that
> >>>  * matches the passed string, adding it if necessary.
> >>>  */
> >>> struct smack_known *smk_import_entry(const char *string, int len)
> >>> {
> >>> 	struct smack_known *skp;
> >>> 	char smack[SMK_LABELLEN];
> >>> 	int found;
> >>> 	int i;
> >>>
> >>> 	if (len <= 0 || len > SMK_MAXLEN)
> >>> 		len = SMK_MAXLEN;
> >>>
> >>> 	for (i = 0, found = 0; i < SMK_LABELLEN; i++) {
> >>> 		if (found)
> >>> 			smack[i] = '\0';
> >>> 		else if (i >= len || string[i] > '~' || string[i] <= ' ' ||
> >>> 			 string[i] == '/') {
> >>> 			smack[i] = '\0';
> >>> 			found = 1;
> >>> 		} else
> >>> 			smack[i] = string[i];
> >>> 	}
> >>>
> >>> 	if (smack[0] == '\0')
> >>> 		return NULL;
> >>>
> >>> 	mutex_lock(&smack_known_lock);
> >>>
> >>> 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
> >>> 		if (strncmp(skp->smk_known, smack, SMK_MAXLEN) == 0)
> >>> 			break;
> >>>
> >>> 	if (skp == NULL) {
> >>> 		skp = kzalloc(sizeof(struct smack_known), GFP_KERNEL);
> >>> 		if (skp != NULL) {
> >>> 			skp->smk_next = smack_known;
> >>> 			strncpy(skp->smk_known, smack, SMK_MAXLEN);
> >>> 			skp->smk_secid = smack_next_secid++;
> >>> 			skp->smk_cipso = NULL;
> >>> 			spin_lock_init(&skp->smk_cipsolock);
> >>> 			/*
> >>> 			 * Make sure that the entry is actually
> >>> 			 * filled before putting it on the list.
> >>> 			 */
> >>> 			smp_mb();
> >>> 			smack_known = skp;
> > 
> > If the read side is not acquiring smack_known_lock, then the above
> > assignment to smack_known needs to be:
> > 
> > 			rcu_assign_pointer(smack_known, skp);
> > 
> > Otherwise, both CPU and compiler are within their rights to reorder
> > the assignment to smack_known ahead of the initialization code.
> > 
> > Alternatively, if you make this list use a standard struct list_head,
> > you could just use list_add_rcu().
> >
> that's what i was going to do ;)
> 
> 
> >>> 		}
> >>> 	}
> >>>
> >>> 	mutex_unlock(&smack_known_lock);
> >>>
> >>> 	return skp;
> >>> }
> >>>
> >>> /**
> >>>  * smack_from_secid - find the Smack label associated with a secid
> >>>  * @secid: an integer that might be associated with a Smack label
> >>>  *
> >>>  * Returns a pointer to the appropraite Smack label if there is one,
> >>>  * otherwise a pointer to the invalid Smack label.
> >>>  */
> >>> char *smack_from_secid(const u32 secid)
> >>> {
> >>> 	struct smack_known *skp;
> >>>
> >>> 	for (skp = smack_known; skp != NULL; skp = skp->smk_next)
> >>> 		if (skp->smk_secid == secid)
> >>> 			return skp->smk_known;
> >>>
> >>> 	/*
> >>> 	 * If we got this far someone asked for the translation
> >>> 	 * of a secid that is not on the list.
> >>> 	 */
> >>> 	return smack_known_invalid.smk_known;
> >>> }
> >> I think this is a case called "dependency ordering".
> >> This function needs rcu_dereference(), doesn't it?
> > 
> > Indeed!  The "for" loop needs to be:
> > 
> > 	for (skp = rcu_dereference(smack_known); skp != NULL; skp = rcu_dereference(skp->smk_next))
> > 
> > Alternatively, if you switch to struct list_head, you could use
> > list_for_each_entry_rcu().
> > 
> > There also need to be rcu_read_lock() and rcu_read_unlock() in here
> > somewhere.  Where they must be depends on how (or whether) you are
> > ever removing any elements.  If the string referenced by smk_known
> > gets freed up, then the caller will need to surround the call to
> > smack_from_secid() and the use of the return value with rcu_read_lock()
> > and rcu_read_unlock().  Otherwise, only the smack_known structures are
> > ever freed up, then just the "for" loop above needs to be so protected.
> > 
> > If these structure are never freed, then please add a comment.
> > 
> 
> for the time being there are not freed; but if think it's safer to add the 
> "rcu_read_lock()/rcu_read_unlock()" anyway (in case someone want to implement a del in the future)
> I don't think they are any downside?

The overhead of rcu_read_lock() and rcu_read_unlock() is quite low, and
they are immune from deadlock (aside from doing something blatantly
illegal like putting a synchronize_rcu() under an rcu_read_lock()).

So the downside is quite small.

> thanks for the explanations!

NP, hope it works well.

							Thanx, Paul

Re: [PATCH][SMACK] convert smack rule list to linux list

[Paul E. McKenney]

On Sun, Feb 22, 2009 at 09:54:00AM -0800, Casey Schaufler wrote:
> Paul E. McKenney wrote:
> > On Sun, Feb 22, 2009 at 02:14:38PM +0100, etienne wrote:
> >   
> >> Tetsuo Handa wrote:
> >>     
> >>> etienne wrote:
> >>>       
> >>>> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> >>>> index 2e0b83e..3dc312d 100644
> >>>> --- a/security/smack/smack_access.c
> >>>> +++ b/security/smack/smack_access.c
> >>>> @@ -87,7 +87,6 @@ static u32 smack_next_secid = 10;
> >>>>  int smk_access(char *subject_label, char *object_label, int request)
> >>>>  {
> >>>>  	u32 may = MAY_NOT;
> >>>> -	struct smk_list_entry *sp;
> >>>>  	struct smack_rule *srp;
> >>>>  
> >>>>  	/*
> >>>> @@ -139,8 +138,8 @@ int smk_access(char *subject_label, char *object_label, int request)
> >>>>  	 * access (e.g. read is included in readwrite) it's
> >>>>  	 * good.
> >>>>  	 */
> >>>> -	for (sp = smack_list; sp != NULL; sp = sp->smk_next) {
> >>>> -		srp = &sp->smk_rule;
> >>>> +
> >>>> +	list_for_each_entry(srp, &smack_rule_list, list) {
> >>>>  
> >>>>  		if (srp->smk_subject == subject_label ||
> >>>>  		    strcmp(srp->smk_subject, subject_label) == 0) {
> >>>>         
> >>> Use of standard doubly linked list requires a lock, doesn't it?
> >>> What lock protects smack_rule_list?
> >>>
> >>>       
> >> you're right; 
> >>
> >> what's the best way, using a rcu variant for "list_for_each, container_of ...etc" ?
> >> (concurrent list insertion are already protected with a mutex, so rcu must the good idea for the read side) 
> >>     
> >
> > You want list_for_each_entry_rcu() above.  You will need list_add_rcu()
> > when adding elements to the list.
> >
> > Again, if these elements are ever removed, you will need rcu_read_lock()
> > and rcu_read_unlock() surrounding their use.  Otherwise, an element can
> > be freed out from under a reader who is still referencing it.
> 
> You'll also need to be very careful that the locking is safe to use
> in the networking hooks, in particular smack_socket_sock_rcv_skb. The
> amount of care required to get the locking correct is a major factor
> in the current list implementation.

I must defer to you on this one!

							Thanx, Paul