| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090222182438.GG6860@linux.vnet.ibm.com>
Date: Sun, 22 Feb 2009 10:24:38 -0800
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: etienne <etienne.basset@...ericable.fr>
Cc: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH][SMACK] convert smack rule list to linux list
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists