On Fri, 21 Jan 2011 15:44:29 -0600
shirishpargaonkar@xxxxxxxxx wrote:
> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>
> RB tree search and insertion routines.
>
> A SID which needs to be mapped, is looked up in one of the RB trees
> depending whether SID is either owner or group SID.
> If found in a tree, a (mapped) id from that node is assigned to
> uid or gid as appropriated.
>
> Otherwise, the SID is mapped and looked-up with the help of
> Winbind via upcall mechanism.
>
> To map a SID, which can be either a Owner SID or a Group SID, key
> description starts with the string "os" or "gs" followed by SID converted
> to a string. Without "os" or "gs", cifs.upcall does not know whether
> SID needs to be mapped to either an uid or a gid.
>
> For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
> it would be used to obtain an SID (string) for an id.
>
>
> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
[...]
> +static int
> +sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid *psid,
> + struct cifs_fattr *fattr, uint sidtype)
> +{
> + int rc = 0;
> + unsigned long id;
> + char *sidstr, *strptr;
> + struct key *idkey;
> + const struct cred *saved_cred;
> + struct cifs_sid_id *sididptr;
> +
> + if (sidtype == SIDOWNER) {
> + spin_lock(&cifs_sb->siduidlock);
> + id = id_rb_search(&cifs_sb->uidtree, psid);
> + spin_unlock(&cifs_sb->siduidlock);
> + } else if (sidtype == SIDGROUP) {
> + spin_lock(&cifs_sb->sidgidlock);
> + id = id_rb_search(&cifs_sb->gidtree, psid);
> + spin_unlock(&cifs_sb->sidgidlock);
> + } else
> + return -ENOENT;
> +
> + if (!id) {
> + sidstr = kzalloc(SIDLEN, GFP_KERNEL);
> + if (!sidstr)
> + return -ENOMEM;
> +
> + sididptr = kzalloc(sizeof(struct cifs_sid_id), GFP_KERNEL);
> + if (!sididptr) {
> + kfree(sidstr);
> + return -ENOMEM;
> + }
> +
> + if (sidtype == SIDOWNER)
> + sprintf(sidstr, "%s", "os:");
> + else if (sidtype == SIDGROUP)
> + sprintf(sidstr, "%s", "gs:");
> +
> + strptr = sidstr + strlen(sidstr);
> + sid_to_str(psid, strptr);
> +
> + saved_cred = override_creds(root_cred);
> + idkey = request_key(&cifs_idmap_key_type, sidstr, "");
> + if (IS_ERR(idkey))
> + cFYI(1, "%s: Can't resolve SID to an uid", __func__);
> + else {
> + id = *(unsigned long *)idkey->payload.value;
> + key_put(idkey);
> + memcpy(&sididptr->sid, psid, sizeof(struct cifs_sid));
> + sididptr->id = id;
> + if (sidtype == SIDOWNER) {
> + spin_lock(&cifs_sb->siduidlock);
> + id_rb_insert(&cifs_sb->uidtree, sididptr, id);
> + spin_unlock(&cifs_sb->siduidlock);
> + } else {
> + spin_lock(&cifs_sb->sidgidlock);
> + id_rb_insert(&cifs_sb->gidtree, sididptr, id);
> + spin_unlock(&cifs_sb->sidgidlock);
> + }
> + }
> + revert_creds(saved_cred);
> + kfree(sidstr);
> + }
> +
> + if (sidtype == SIDOWNER)
> + fattr->cf_uid = id;
> + else
> + fattr->cf_gid = id;
> +
> + return rc;
> +}
> +
This looks racy.
You search for an entry in the rbtree and then do the upcall if it
doesn't exist. When you get the response back you stick it in the tree.
What if two different threads are doing this at the same time? It's
easily possible that once the downcall comes in, another thread has
already stuck an entry in that spot.
What you should do is stick a placeholder in the tree prior to doing
the upcall. Then, if another thread comes along and does a search you
have it wait until the upcall is complete. See cifs_sb_tlink for an
example of how to do that.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
