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

Paul E. McKenney
2009-02-22T15:29:09Z

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