On Tue, 29 Mar 2011 10:23:37 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Tue, Mar 29, 2011 at 8:30 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Mon, 28 Mar 2011 15:25:01 -0500
> > 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 the tree, a (mapped) id from that node is assigned to
> >> uid or gid as appropriate. ÂIf unmapped, an upcall is attempted to
> >> map the SID to an id. ÂIf upcall is successful, node is marked as
> >> mapped. ÂIf upcall fails, node stays marked as unmapped and a mapping
> >> is attempted again during next resolution.
> >>
> >> 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.
> >>
> >> Nodes in rb tree as fields to prevent multiple upcalls for
> >> a SID. ÂAdding and removing nodes is done within global locks.
> >> shrinker routine does not prune a node if mapping for that node
> >> is either pending or or node was recently created (before timeout
> >> period expires and node could be purged). The timeout period should
> >> be long enough at least for an upcall to return back to cifs.
> >>
> >> 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>
> >> ---
> >> Âfs/cifs/cifsacl.c | Â298 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >> Âfs/cifs/cifsacl.h | Â 22 ++++
> >> Â2 files changed, 296 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> >> index c60bb90..5b2ef73 100644
> >> --- a/fs/cifs/cifsacl.c
> >> +++ b/fs/cifs/cifsacl.c
> >> @@ -54,7 +54,30 @@ static const struct cifs_sid sid_authusers = {
> >> Â/* group users */
> >> Âstatic const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
> >>
> >> -static const struct cred *root_cred;
> >> +const struct cred *root_cred;
> >> +
> >> +static void
> >> +shrink_idmap_tree(struct rb_root *root, int nr_to_scan, int *nr_rem,
> >> + Â Â Â Â Â Â Â Â Â Â int *nr_del)
> >> +{
> >> + Â Â struct rb_node *node;
> >> + Â Â struct rb_node *tmp;
> >> + Â Â struct cifs_sid_id *psidid;
> >> +
> >> + Â Â node = rb_first(root);
> >> + Â Â while (node) {
> >> + Â Â Â Â Â Â tmp = node;
> >> + Â Â Â Â Â Â node = rb_next(tmp);
> >> + Â Â Â Â Â Â psidid = rb_entry(tmp, struct cifs_sid_id, rbnode);
> >> + Â Â Â Â Â Â if (time_after(psidid->time + SID_MAP_EXPIRE, jiffies) ||
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â (nr_to_scan == 0) || (*nr_del == nr_to_scan))
> >> + Â Â Â Â Â Â Â Â Â Â ++(*nr_rem);
> >> + Â Â Â Â Â Â else {
> >> + Â Â Â Â Â Â Â Â Â Â rb_erase(tmp, root);
> >> + Â Â Â Â Â Â Â Â Â Â ++(*nr_del);
> >> + Â Â Â Â Â Â }
> >> + Â Â }
> >> +}
> >>
> >> Â/*
> >> Â * Run idmap cache shrinker.
> >> @@ -62,9 +85,21 @@ static const struct cred *root_cred;
> >> Âstatic int
> >> Âcifs_idmap_shrinker(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask)
> >> Â{
> >> - Â Â /* Use a pruning scheme in a subsequent patch instead */
> >> - Â Â cifs_destroy_idmaptrees();
> >> - Â Â return 0;
> >> + Â Â int nr_del = 0;
> >> + Â Â int nr_rem = 0;
> >> + Â Â struct rb_root *root;
> >> +
> >> + Â Â root = &uidtree;
> >> + Â Â spin_lock(&siduidlock);
> >> + Â Â shrink_idmap_tree(root, nr_to_scan, &nr_rem, &nr_del);
> >> + Â Â spin_unlock(&siduidlock);
> >> +
> >> + Â Â root = &gidtree;
> >> + Â Â spin_lock(&sidgidlock);
> >> + Â Â shrink_idmap_tree(root, nr_to_scan, &nr_rem, &nr_del);
> >> + Â Â spin_unlock(&sidgidlock);
> >> +
> >> + Â Â return nr_rem;
> >> Â}
> >>
> >> Âstatic struct shrinker cifs_shrinker = {
> >> @@ -92,7 +127,6 @@ cifs_idmap_key_destroy(struct key *key)
> >> Â Â Â kfree(key->payload.data);
> >> Â}
> >>
> >> -static
> >> Âstruct key_type cifs_idmap_key_type = {
> >>    .name    Â= "cifs.cifs_idmap",
> >> Â Â Â .instantiate = cifs_idmap_key_instantiate,
> >> @@ -101,6 +135,196 @@ struct key_type cifs_idmap_key_type = {
> >>    .match    = user_match,
> >> Â};
> >>
> >> +static void
> >> +id_rb_insert(struct rb_root *root, struct cifs_sid *sidptr,
> >> + Â Â Â Â Â Â struct cifs_sid_id **psidid)
> >> +{
> >> + Â Â int rc;
> >> + Â Â struct rb_node *node = root->rb_node;
> >> + Â Â struct rb_node *parent = NULL;
> >> + Â Â struct rb_node **linkto = &(root->rb_node);
> >> + Â Â struct cifs_sid_id *lsidid;
> >> +
> >> + Â Â while (node) {
> >> + Â Â Â Â Â Â lsidid = rb_entry(node, struct cifs_sid_id, rbnode);
> >> + Â Â Â Â Â Â parent = node;
> >> + Â Â Â Â Â Â rc = compare_sids(sidptr, &((lsidid)->sid));
> >> + Â Â Â Â Â Â if (rc > 0) {
> >> + Â Â Â Â Â Â Â Â Â Â linkto = &(node->rb_left);
> >> + Â Â Â Â Â Â Â Â Â Â node = node->rb_left;
> >> + Â Â Â Â Â Â } else if (rc < 0) {
> >> + Â Â Â Â Â Â Â Â Â Â linkto = &(node->rb_right);
> >> + Â Â Â Â Â Â Â Â Â Â node = node->rb_right;
> >> + Â Â Â Â Â Â }
> >> + Â Â }
> >> +
> >> + Â Â memcpy(&(*psidid)->sid, sidptr, sizeof(struct cifs_sid));
> >> + Â Â clear_bit(SID_ID_PENDING, &(*psidid)->state);
> >> + Â Â clear_bit(SID_ID_MAPPED, &(*psidid)->state);
> >> +
> >> + Â Â rb_link_node(&(*psidid)->rbnode, parent, linkto);
> >> + Â Â rb_insert_color(&(*psidid)->rbnode, root);
> >> +}
> >> +
> >> +static struct cifs_sid_id *
> >> +id_rb_search(struct rb_root *root, struct cifs_sid *sidptr)
> >> +{
> >> + Â Â int rc;
> >> + Â Â struct rb_node *node = root->rb_node;
> >> + Â Â struct rb_node *parent = NULL;
> >> + Â Â struct rb_node **linkto = &(root->rb_node);
> >> + Â Â struct cifs_sid_id *lsidid;
> >> +
> >> + Â Â while (node) {
> >> + Â Â Â Â Â Â lsidid = rb_entry(node, struct cifs_sid_id, rbnode);
> >> + Â Â Â Â Â Â parent = node;
> >> + Â Â Â Â Â Â rc = compare_sids(sidptr, &((lsidid)->sid));
> >> + Â Â Â Â Â Â if (rc > 0) {
> >> + Â Â Â Â Â Â Â Â Â Â linkto = &(node->rb_left);
> >> + Â Â Â Â Â Â Â Â Â Â node = node->rb_left;
> >> + Â Â Â Â Â Â } else if (rc < 0) {
> >> + Â Â Â Â Â Â Â Â Â Â linkto = &(node->rb_right);
> >> + Â Â Â Â Â Â Â Â Â Â node = node->rb_right;
> >> + Â Â Â Â Â Â } else /* node found */
> >> + Â Â Â Â Â Â Â Â Â Â return lsidid;
> >> + Â Â }
> >> +
> >> + Â Â return NULL;
> >> +}
> >> +
> >> +static void
> >> +sid_to_str(struct cifs_sid *sidptr, char *sidstr)
> >> +{
> >> + Â Â int i;
> >> + Â Â unsigned long saval;
> >> + Â Â char *strptr;
> >> +
> >> + Â Â strptr = sidstr;
> >> +
> >> + Â Â sprintf(strptr, "%s", "S");
> >> + Â Â strptr = sidstr + strlen(sidstr);
> >> +
> >> + Â Â sprintf(strptr, "-%d", sidptr->revision);
> >> + Â Â strptr = sidstr + strlen(sidstr);
> >> +
> >> + Â Â for (i = 0; i < 6; ++i) {
> >> + Â Â Â Â Â Â if (sidptr->authority[i]) {
> >> + Â Â Â Â Â Â Â Â Â Â sprintf(strptr, "-%d", sidptr->authority[i]);
> >> + Â Â Â Â Â Â Â Â Â Â strptr = sidstr + strlen(sidstr);
> >> + Â Â Â Â Â Â }
> >> + Â Â }
> >> +
> >> + Â Â for (i = 0; i < sidptr->num_subauth; ++i) {
> >> + Â Â Â Â Â Â saval = le32_to_cpu(sidptr->sub_auth[i]);
> >> + Â Â Â Â Â Â sprintf(strptr, "-%ld", saval);
> >> + Â Â Â Â Â Â strptr = sidstr + strlen(sidstr);
> >> + Â Â }
> >> +}
> >> +
> >> +static int
> >> +sidid_pending_wait(void *unused)
> >> +{
> >> + Â Â schedule();
> >> + Â Â return signal_pending(current) ? -ERESTARTSYS : 0;
> >> +}
> >> +
> >> +static int
> >> +sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid *psid,
> >> + Â Â Â Â Â Â struct cifs_fattr *fattr, uint sidtype)
> >> +{
> >> + Â Â int rc;
> >> + Â Â unsigned long cid;
> >> + Â Â char *strptr;
> >> + Â Â struct key *idkey;
> >> + Â Â const struct cred *saved_cred;
> >> + Â Â struct cifs_sid_id *psidid, *npsidid;
> >> + Â Â struct rb_root *cidtree;
> >> + Â Â spinlock_t *cidlock;
> >> +
> >> + Â Â if (sidtype == SIDOWNER) {
> >> + Â Â Â Â Â Â cid = cifs_sb->mnt_uid; /* default uid, in case upcall fails */
> >> + Â Â Â Â Â Â cidlock = &siduidlock;
> >> + Â Â Â Â Â Â cidtree = &uidtree;
> >> + Â Â } else if (sidtype == SIDGROUP) {
> >> + Â Â Â Â Â Â cid = cifs_sb->mnt_gid; /* default gid, in case upcall fails */
> >> + Â Â Â Â Â Â cidlock = &sidgidlock;
> >> + Â Â Â Â Â Â cidtree = &gidtree;
> >> + Â Â } else
> >> + Â Â Â Â Â Â return -ENOENT;
> >> +
> >> + Â Â spin_lock(cidlock);
> >> + Â Â psidid = id_rb_search(cidtree, psid);
> >> + Â Â spin_unlock(cidlock);
> >> +
> >> + Â Â if (!psidid) { /* node does not exist, allocate one & attempt adding */
> >> + Â Â Â Â Â Â npsidid = kzalloc(sizeof(struct cifs_sid_id), GFP_KERNEL);
> >> + Â Â Â Â Â Â if (!npsidid)
> >> + Â Â Â Â Â Â Â Â Â Â return -ENOMEM;
> >> +
> >> + Â Â Â Â Â Â spin_lock(cidlock);
> >> + Â Â Â Â Â Â psidid = id_rb_search(cidtree, psid);
> >> + Â Â Â Â Â Â if (psidid) { /* node happened to get inserted meanwhile */
> >> + Â Â Â Â Â Â Â Â Â Â spin_unlock(cidlock);
> >> + Â Â Â Â Â Â Â Â Â Â kfree(npsidid);
> >> + Â Â Â Â Â Â } else {
> >> + Â Â Â Â Â Â Â Â Â Â psidid = npsidid;
> >> + Â Â Â Â Â Â Â Â Â Â id_rb_insert(cidtree, psid, &psidid);
> >> + Â Â Â Â Â Â Â Â Â Â spin_unlock(cidlock);
> >> + Â Â Â Â Â Â }
> >> + Â Â }
> >> +
> >> + Â Â if (!test_bit(SID_ID_MAPPED, &psidid->state)) { /* SID not mapped */
> >
> > Â Â Â Â^^^ nit: maybe just do:
> >
> > Â Â Â Âif (test_bit(SID_ID_MAPPED...
> > Â Â Â Â Â Â Â Âgoto out;
> >
> > Â Â Â Â...or something. That would allow you to reduce the indentation
> > Â Â Â Âof the code below and make it easier to read.
> >
>
> Will make this change.
>
> >> + Â Â Â Â Â Â if (!test_and_set_bit(SID_ID_PENDING, &psidid->state)) {
> >> + Â Â Â Â Â Â Â Â Â Â memset(psidid->sidstr, '\0', SIDLEN);
> >> + Â Â Â Â Â Â Â Â Â Â if (sidtype == SIDOWNER)
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â sprintf(psidid->sidstr, "%s", "os:");
> >> + Â Â Â Â Â Â Â Â Â Â else
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â sprintf(psidid->sidstr, "%s", "gs:");
> >> +
> >> + Â Â Â Â Â Â Â Â Â Â strptr = psidid->sidstr + strlen(psidid->sidstr);
> >> + Â Â Â Â Â Â Â Â Â Â sid_to_str(psid, strptr);
> >> +
> >
> > Would it be better to do the string handling above before you insert
> > this struct into the tree? In the case where this thing can't be mapped,
> > then you're going to have to re-convert the sid on every pass through
> > this function. Just doing that once would make more sense, IMO...
> >
>
> Will address it.
>
> > I wonder too...once this thing is mapped, then the above string will
> > never be used again, will it? If so, would it be best to release that
> > memory instead of keeping it around in the cache?
>
> I think kmalloc the memory for string when node is allocated and free it
> once SID is mapped? The string is really not needed at all once
> a SID is mapped.
>
I think so too. This cache can possibly grow to be rather large, so you
ought to be as efficient as possible with memory (that's a good goal
anyway). It might even be worthwhile to make your own slabcache for
these entries, but that can be done in a later patch once the need is
demonstrated.
> >
> >> + Â Â Â Â Â Â Â Â Â Â saved_cred = override_creds(root_cred);
> >> + Â Â Â Â Â Â Â Â Â Â idkey = request_key(&cifs_idmap_key_type,
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â psidid->sidstr, "");
> >> + Â Â Â Â Â Â Â Â Â Â if (IS_ERR(idkey)) {
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â psidid->id = cid;
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â cFYI(1, "%s: Can't map SID to an id", __func__);
> >> + Â Â Â Â Â Â Â Â Â Â } else {
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â psidid->id =
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â *(unsigned long *)idkey->payload.value;
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â psidid->time = jiffies;
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â set_bit(SID_ID_MAPPED, &psidid->state);
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â key_put(idkey);
> >> + Â Â Â Â Â Â Â Â Â Â }
> >> + Â Â Â Â Â Â Â Â Â Â revert_creds(saved_cred);
> >> + Â Â Â Â Â Â Â Â Â Â clear_bit(SID_ID_PENDING, &psidid->state);
> >> + Â Â Â Â Â Â Â Â Â Â wake_up_bit(&psidid->state, SID_ID_PENDING);
> >> + Â Â Â Â Â Â } else {
> >> + Â Â Â Â Â Â Â Â Â Â /*
> >> + Â Â Â Â Â Â Â Â Â Â Â* either an existing node with SID mapped (in which
> >> + Â Â Â Â Â Â Â Â Â Â Â* case we would not wait) or a node whose SID is
> >> + Â Â Â Â Â Â Â Â Â Â Â* being mapped by somebody else (in which case, we
> >> + Â Â Â Â Â Â Â Â Â Â Â* would end up waiting).
> >> + Â Â Â Â Â Â Â Â Â Â Â*/
> >> + Â Â Â Â Â Â Â Â Â Â rc = wait_on_bit(&psidid->state, SID_ID_PENDING,
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â sidid_pending_wait, TASK_INTERRUPTIBLE);
> >> + Â Â Â Â Â Â Â Â Â Â if (rc) {
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â cFYI(1, "%s: sidid_pending_wait interrupted %d",
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __func__, rc);
> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â return rc;
> >> + Â Â Â Â Â Â Â Â Â Â }
> >> + Â Â Â Â Â Â }
> >> + Â Â }
> >> +
> >> + Â Â if (sidtype == SIDOWNER)
> >> + Â Â Â Â Â Â fattr->cf_uid = psidid->id;
> >> + Â Â else
> >> + Â Â Â Â Â Â fattr->cf_gid = psidid->id;
> >> +
> >> + Â Â return 0;
> >> +}
> >> +
> >
> > So let's consider the case where we have a directory with a lot of
> > files in it that are owned by a user that can't be mapped. Someone does
> > a "ls -l" in that dir.
> >
> > Presumably, on every stat() call we'll call into this routine to map
> > the SID to a unix uid. The MAPPED bit will never be set though, so
> > we'll always call request_key().
> >
> > cifs.upcall will generally negatively instantiate the key with a 1s
> > timeout. I think if request-key doesn't find a match (indicating that
> > no one set up request-key.conf) then it negatively instantiates the key
> > with a 60s timeout.
> >
> > Is that reasonable behavior here?
> >
>
> Not sure I understand the negative instantiation of key with a timeout.
> I had a timeout field in some of the earlier patch versions whrere re-mapping
> for an unmapped SID is attempted only after that timeout expires
>
I believe it works something like this...
When the upcall returns an error without instantiating the key, the
kernel negatively instantiates the key with a 60s timeout. The
cifs.upcall program however will instead negatively instantiate the key
with a 1s timeout.
When there's a negative key in cache, request_key will return an error
(-ENOKEY, I think) without doing the upcall. That's generally a good
thing. We don't want to bog down the box by forcing it to keep forking
new upcall processes that aren't going to work.
The question here is -- what behavior do you want on subsequent
request_key invocations when one fails? How long do you want to wait
before allowing a new upcall in this situation?
--
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
