On Sat, Jan 22, 2011 at 6:48 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> 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.
Would it work if compare_sid returns 0 (would to add a
check for it) during node insert (function id_rb_insert()), free the
allocated node and return?
Function id_rb_insert() is invoked within a spin lock.
>
> 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
