LinuxLists.cc - [PATCH] nfsidmap: use multiple child keyrings

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

Benjamin Coddington <[email protected]> wrote:

> Yep, I'm aware of that - but unless RH backports this fix (which I was told
> is not going to happen) or uses this multiple keyring nfsidmap approach,
> things are going to stay broken in RHEL6.

Backporting the patches to RHEL-6 is a bit tricky as the contents of the key
struct changed as did some of the internal interface functions. It would be
possible to isolate the changes in the keyring payload struct - but we'd have
to hope no drivers out there access the keyring payload directly.

David

2014-03-25 12:56:13

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On Mar 25, 2014, at 5:34 AM, David Howells <[email protected]> wrote:
> Steve Dickson <[email protected]> wrote:
>
>>>>> The kernel keyring has a max of ~508 entries on 64-bit systems.
>>>>> For installations with more distict users than this limit, create
>>>>> a specified number of child keyrings and fill them evenly.
>
> This got fixed recently.
>
> Commit b2a4df200d570b2c33a57e1ebfa5896e4bc81b69
> Author: David Howells <[email protected]> 2013-09-24 10:35:18
> Subject: KEYS: Expand the capacity of a keyring
>
> With that, the capacity is more or less infinite.

Yep, I'm aware of that - but unless RH backports this fix (which I was told is not going to happen) or uses this multiple keyring nfsidmap approach, things are going to stay broken in RHEL6.

Ben

2014-03-25 09:35:07

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

What kernel are you dealing with?

David

2014-03-25 12:49:26

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

I'm dealing with a bunch of RHEL6 boxes - so the RHEL kernel versions: 2.6.32-431.. I can't just update all these machines to later kernels, unfortunately. This nfsidmap fix is just a stop-gap, since it seemed easier to get Steve D to update nfs-utils than to backport the keyring stuff, and when I suggested this method in the BZ I was instructed to bring it to the list.

I regret all this RHEL-specific noise on this list; apologies.

Ben

On Mar 25, 2014, at 5:35 AM, David Howells <[email protected]> wrote:

>
> What kernel are you dealing with?
>
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-03-24 19:51:53

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On 03/24/2014 02:00 PM, Benjamin Coddington wrote:
>
> On Mar 24, 2014, at 1:00 PM, Steve Dickson <[email protected]> wrote:
>
>> On 03/21/2014 05:08 PM, Benjamin Coddington wrote:
>>> The kernel keyring has a max of ~508 entries on 64-bit systems.
>>> For installations with more distict users than this limit, create
>>> a specified number of child keyrings and fill them evenly.
>> A couple things...
>>
>> 1) no Signed-off-by: line
>>
>> 2) Its seems you can create key rings but can't delete them.
>> Here is what I'm doing:
>> in /etc/request-key.d/id_resolver.conf I have
>> create id_resolver * * /usr/sbin/nfsidmap -n 10 %k %d
>> but when I tried to delete the keys
>> # nfsidmap -vc
>> nfsidmap: clearing '08aa156c I--Q--- 1perm 3f010000 0 0 keyring .id_resolver_child_10: empty'
>> nfsidmap: keyctl_clear(0x8aa156c) failed: Permission denied
>
> This mess works on my fleet of RHEL6 boxes which is where I was trying to fix this. They create the child keyrings with
>
> perm 3b3f0000
>
> Instead of yours which appears to be
>
> perm 3f010000
>
> Are you testing on a later kernel? Likely this behavior has changed.
Yes... Much later...

>
>>> #define PROCKEYS "/proc/keys"
>>> #ifndef DEFAULT_KEYRING
>>> -#define DEFAULT_KEYRING "id_resolver"
>>> +#define DEFAULT_KEYRING ".id_resolver"
>> 3) Why is changing the default needed?
>
> The default is wrong. I think that's the first thing I changed when
> trying to fix this problem, since it looked like id_lookup() should
> gracefully recover in the case that the keyring was full
> (but it still doesn't).
I'm think the "id_resolver" default can from the face
the entry /etc/request-key.d/id_resolver.conf
which tells nfsidmap put the keys on the id_resolver
key ring... so I'm not really sure where the
.id_resolver is coming from... CC-ing David Howells
maybe he knows...

> This actually doesn't have to change, since
> the strcmp()s that use it will work either way -- but it might catch
> someone by surprise later. I'll split this out.
Please do...

>
>> Finally, what -n value do plan on using? Maybe a blurb in the man page
>> on what a good number is and why....
>
> I've got this running now with -n160, since
> we have ~60K distinct uid/gid s. Ideally, I'd like to re-submit
> this to self-scale which wouldn't require any sysadmin tuning,
> but I haven't had the time. Really, this is just a quick fix
> for the brokenness that's in current RHEL and less-new Fedora.
The brokenness in RHEL will be healing very soon... See
https://bugzilla.redhat.com/show_bug.cgi?id=1033708. RHEL is
basically going back to using rpc.idmapd on the client and
nfsidmap is going away... It as just a bad dream... It never
happen! ;-)

> If you want it done right it will take me a week or two to find the time.
In newer I don't think this is needed since they are already larger...
Again, David can address this...

steved.

>
> Ben
>
>> steved.
>>
>>> #endif
>>>
>>> #ifndef PATH_IDMAPDCONF
>>> @@ -39,7 +39,7 @@ static int keyring_clear(char *keyring);
>>> /*
>>> * Find either a user or group id based on the name@domain string
>>> */
>>> -int id_lookup(char *name_at_domain, key_serial_t key, int type)
>>> +int id_lookup(char *name_at_domain, key_serial_t key, int type, key_serial_t dest_keyring)
>>> {
>>> char id[MAX_ID_LEN];
>>> uid_t uid = 0;
>>> @@ -58,7 +58,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
>>> (type == USER ? "nfs4_owner_to_uid" : "nfs4_group_owner_to_gid"));
>>>
>>> if (rc == 0) {
>>> - rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
>>> + rc = keyctl_instantiate(key, id, strlen(id) + 1, dest_keyring);
>>> if (rc < 0) {
>>> switch(rc) {
>>> case -EDQUOT:
>>> @@ -67,9 +67,9 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
>>> /*
>>> * The keyring is full. Clear the keyring and try again
>>> */
>>> - rc = keyring_clear(DEFAULT_KEYRING);
>>> + rc = keyctl_clear(dest_keyring);
>>> if (rc == 0)
>>> - rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
>>> + rc = keyctl_instantiate(key, id, strlen(id) + 1, dest_keyring);
>>> break;
>>> default:
>>> break;
>>> @@ -85,7 +85,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
>>> /*
>>> * Find the name@domain string from either a user or group id
>>> */
>>> -int name_lookup(char *id, key_serial_t key, int type)
>>> +int name_lookup(char *id, key_serial_t key, int type, key_serial_t dest_keyring)
>>> {
>>> char name[IDMAP_NAMESZ];
>>> char domain[NFS4_MAX_DOMAIN_LEN];
>>> @@ -113,7 +113,7 @@ int name_lookup(char *id, key_serial_t key, int type)
>>> (type == USER ? "nfs4_uid_to_name" : "nfs4_gid_to_name"));
>>>
>>> if (rc == 0) {
>>> - rc = keyctl_instantiate(key, &name, strlen(name), 0);
>>> + rc = keyctl_instantiate(key, &name, strlen(name), dest_keyring);
>>> if (rc < 0)
>>> xlog_err("name_lookup: keyctl_instantiate failed: %m");
>>> }
>>> @@ -127,7 +127,7 @@ static int keyring_clear(char *keyring)
>>> {
>>> FILE *fp;
>>> char buf[BUFSIZ];
>>> - key_serial_t key;
>>> + key_serial_t key, child_key;
>>>
>>> if (keyring == NULL)
>>> keyring = DEFAULT_KEYRING;
>>> @@ -151,6 +151,7 @@ static int keyring_clear(char *keyring)
>>> */
>>> *(strchr(buf, ' ')) = '\0';
>>> sscanf(buf, "%x", &key);
>>> +
>>> if (keyctl_clear(key) < 0) {
>>> xlog_err("keyctl_clear(0x%x) failed: %m", key);
>>> fclose(fp);
>>> @@ -159,7 +160,8 @@ static int keyring_clear(char *keyring)
>>> fclose(fp);
>>> return 0;
>>> }
>>> - xlog_err("'%s' keyring was not found.", keyring);
>>> + if (strstr(keyring, DEFAULT_KEYRING":"))
>>> + xlog_err("'%s' keyring was not found.", keyring);
>>> fclose(fp);
>>> return 1;
>>> }
>>> @@ -232,8 +234,10 @@ int main(int argc, char **argv)
>>> char *type;
>>> int rc = 1, opt;
>>> int timeout = 600;
>>> - key_serial_t key;
>>> + int childrings = 0;
>>> + key_serial_t key, parent_keyring, dest_keyring;
>>> char *progname, *keystr = NULL;
>>> + char child_name[BUFSIZ];
>>> int clearing = 0, keymask = 0;
>>>
>>> /* Set the basename */
>>> @@ -244,7 +248,7 @@ int main(int argc, char **argv)
>>>
>>> xlog_open(progname);
>>>
>>> - while ((opt = getopt(argc, argv, "u:g:r:ct:v")) != -1) {
>>> + while ((opt = getopt(argc, argv, "u:g:r:ct:vn:d:")) != -1) {
>>> switch (opt) {
>>> case 'u':
>>> keymask = UIDKEYS;
>>> @@ -267,6 +271,9 @@ int main(int argc, char **argv)
>>> case 't':
>>> timeout = atoi(optarg);
>>> break;
>>> + case 'n':
>>> + childrings = atoi(optarg);
>>> + break;
>>> default:
>>> xlog_warn(usage, progname);
>>> break;
>>> @@ -284,9 +291,16 @@ int main(int argc, char **argv)
>>> rc = key_revoke(keystr, keymask);
>>> return rc;
>>> }
>>> +
>>> if (clearing) {
>>> xlog_syslog(0);
>>> - rc = keyring_clear(DEFAULT_KEYRING);
>>> + int i = 1;
>>> + for(rc = 0; rc == 0; i++) {
>>> + snprintf(child_name, sizeof(child_name), DEFAULT_KEYRING "_child_%d", i);
>>> + rc = keyring_clear(child_name);
>>> + }
>>> +
>>> + rc = keyring_clear(DEFAULT_KEYRING ":");
>>> return rc;
>>> }
>>>
>>> @@ -315,14 +329,42 @@ int main(int argc, char **argv)
>>> key, type, value, timeout);
>>> }
>>>
>>> + if (childrings) {
>>> + int i;
>>> + long child_size, smallest_size = 2032;
>>> + parent_keyring = request_key("keyring", DEFAULT_KEYRING, NULL, KEY_SPEC_THREAD_KEYRING);
>>> +
>>> + for (i = 1; i <= childrings; i++) {
>>> + key_serial_t child_keyring;
>>> +
>>> + snprintf(child_name, sizeof(child_name), DEFAULT_KEYRING "_child_%d", i);
>>> +
>>> + child_keyring = keyctl_search(parent_keyring, "keyring", child_name, 0);
>>> + if (child_keyring < 0) {
>>> + child_keyring = add_key("keyring", child_name, NULL, 0, parent_keyring);
>>> + xlog_warn("added new child %s: %m", child_name);
>>> + }
>>> +
>>> + child_size = keyctl_read(child_keyring, NULL, 0);
>>> + if (child_size < smallest_size) {
>>> + dest_keyring = child_keyring;
>>> + smallest_size = child_size;
>>> + }
>>> + }
>>> + }
>>> +
>>> if (strcmp(type, "uid") == 0)
>>> - rc = id_lookup(value, key, USER);
>>> + rc = id_lookup(value, key, USER, dest_keyring);
>>> else if (strcmp(type, "gid") == 0)
>>> - rc = id_lookup(value, key, GROUP);
>>> + rc = id_lookup(value, key, GROUP, dest_keyring);
>>> else if (strcmp(type, "user") == 0)
>>> - rc = name_lookup(value, key, USER);
>>> + rc = name_lookup(value, key, USER, dest_keyring);
>>> else if (strcmp(type, "group") == 0)
>>> - rc = name_lookup(value, key, GROUP);
>>> + rc = name_lookup(value, key, GROUP, dest_keyring);
>>> +
>>> + /* if we hung this off a child, unlink from the parent */
>>> + if (dest_keyring)
>>> + keyctl_unlink(key, parent_keyring);
>>>
>>> /* Set timeout to 10 (600 seconds) minutes */
>>> if (rc == 0)
>>> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
>>> index 3a3a523..04f0014 100644
>>> --- a/utils/nfsidmap/nfsidmap.man
>>> +++ b/utils/nfsidmap/nfsidmap.man
>>> @@ -6,7 +6,7 @@
>>> .SH NAME
>>> nfsidmap \- The NFS idmapper upcall program
>>> .SH SYNOPSIS
>>> -.B "nfsidmap [-v] [-t timeout] key desc"
>>> +.B "nfsidmap [-v] [-t timeout] [-n count] key desc"
>>> .br
>>> .B "nfsidmap [-v] [-c]"
>>> .br
>>> @@ -42,6 +42,9 @@ Revoke both the uid and gid key of the given user.
>>> Set the expiration timer, in seconds, on the key.
>>> The default is 600 seconds (10 mins).
>>> .TP
>>> +.B -n count
>>> +Set the the number of child keyrings to create.
>>> +.TP
>>> .B -u user
>>> Revoke the uid key of the given user.
>>> .TP
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2014-03-24 22:10:34

by Anna Schumaker

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On 03/24/2014 05:22 PM, Steve Dickson wrote:
>
> On 03/24/2014 05:03 PM, Benjamin Coddington wrote:
>> On Mar 24, 2014, at 3:51 PM, Steve Dickson <[email protected]> wrote:
>>
>>>>> Finally, what -n value do plan on using? Maybe a blurb in the man page
>>>>> on what a good number is and why....
>>>> I've got this running now with -n160, since
>>>> we have ~60K distinct uid/gid s. Ideally, I'd like to re-submit
>>>> this to self-scale which wouldn't require any sysadmin tuning,
>>>> but I haven't had the time. Really, this is just a quick fix
>>>> for the brokenness that's in current RHEL and less-new Fedora.
>>> The brokenness in RHEL will be healing very soon... See
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1033708. RHEL is
>>> basically going back to using rpc.idmapd on the client and
>>> nfsidmap is going away... It as just a bad dream... It never
>>> happen! ;-)
>> This BZ says the fix is coming in nfs-utils by removing the nfsidmap command.
> Yeah.. I bet the RHEL police are not going to be happy about that... so I
> might have to put it back...
>
>> But IIRC, unless the kernel side of the idmapper is changed, it will exec request-key
>> for every lookup before falling back to the upcall, and there's no cache in front of that.
>> Isn't that going to have some performance problems?
> I don't thinks so, since things will eventually get cached but I'll definitely look into it...

The fallback upcall version uses the "id_legacy" keyring to cache results. A bigger keyring might be the best solution.

Anna
>
>> I'm much more interested in getting the keyrings to work, since they seem to
>> offer significant performance gains.
> I too, but the back port of the current key is a bit too much for RHEL at this point.
>> If RH is going to change the kernel side of the idmapper to only upcall, then I'd be satisfied to use that.
> It was a bone-head decision on my part to flip this type of switch midstream...
> I too just want to put things back to the way were,,,
>
> steved.
>
>> Ben
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-03-24 21:22:36

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On 03/24/2014 05:03 PM, Benjamin Coddington wrote:
>
> On Mar 24, 2014, at 3:51 PM, Steve Dickson <[email protected]> wrote:
>
>>>> Finally, what -n value do plan on using? Maybe a blurb in the man page
>>>> on what a good number is and why....
>>>
>>> I've got this running now with -n160, since
>>> we have ~60K distinct uid/gid s. Ideally, I'd like to re-submit
>>> this to self-scale which wouldn't require any sysadmin tuning,
>>> but I haven't had the time. Really, this is just a quick fix
>>> for the brokenness that's in current RHEL and less-new Fedora.
>> The brokenness in RHEL will be healing very soon... See
>> https://bugzilla.redhat.com/show_bug.cgi?id=1033708. RHEL is
>> basically going back to using rpc.idmapd on the client and
>> nfsidmap is going away... It as just a bad dream... It never
>> happen! ;-)
>
> This BZ says the fix is coming in nfs-utils by removing the nfsidmap command.
Yeah.. I bet the RHEL police are not going to be happy about that... so I
might have to put it back...

> But IIRC, unless the kernel side of the idmapper is changed, it will exec request-key
> for every lookup before falling back to the upcall, and there's no cache in front of that.
> Isn't that going to have some performance problems?
I don't thinks so, since things will eventually get cached but I'll definitely look into it...

> I'm much more interested in getting the keyrings to work, since they seem to
> offer significant performance gains.
I too, but the back port of the current key is a bit too much for RHEL at this point.
>
> If RH is going to change the kernel side of the idmapper to only upcall, then I'd be satisfied to use that.
It was a bone-head decision on my part to flip this type of switch midstream...
I too just want to put things back to the way were,,,

steved.

>
> Ben
>

2014-03-25 09:34:20

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

Steve Dickson <[email protected]> wrote:

> >>> The kernel keyring has a max of ~508 entries on 64-bit systems.
> >>> For installations with more distict users than this limit, create
> >>> a specified number of child keyrings and fill them evenly.

This got fixed recently.

Commit b2a4df200d570b2c33a57e1ebfa5896e4bc81b69
Author: David Howells <[email protected]> 2013-09-24 10:35:18
Subject: KEYS: Expand the capacity of a keyring

With that, the capacity is more or less infinite.

David

2014-03-24 18:22:35

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On Mar 24, 2014, at 1:00 PM, Steve Dickson <[email protected]> wrote:

> On 03/21/2014 05:08 PM, Benjamin Coddington wrote:
>> The kernel keyring has a max of ~508 entries on 64-bit systems.
>> For installations with more distict users than this limit, create
>> a specified number of child keyrings and fill them evenly.
> A couple things...
>
> 1) no Signed-off-by: line
>
> 2) Its seems you can create key rings but can't delete them.
> Here is what I'm doing:
> in /etc/request-key.d/id_resolver.conf I have
> create id_resolver * * /usr/sbin/nfsidmap -n 10 %k %d
> but when I tried to delete the keys
> # nfsidmap -vc
> nfsidmap: clearing '08aa156c I--Q--- 1perm 3f010000 0 0 keyring .id_resolver_child_10: empty'
> nfsidmap: keyctl_clear(0x8aa156c) failed: Permission denied

This mess works on my fleet of RHEL6 boxes which is where I was trying to fix this. They create the child keyrings with

perm 3b3f0000

Instead of yours which appears to be

perm 3f010000

Are you testing on a later kernel? Likely this behavior has changed.

>> #define PROCKEYS "/proc/keys"
>> #ifndef DEFAULT_KEYRING
>> -#define DEFAULT_KEYRING "id_resolver"
>> +#define DEFAULT_KEYRING ".id_resolver"
> 3) Why is changing the default needed?

The default is wrong. I think that's the first thing I changed when trying to fix this problem, since it looked like id_lookup() should gracefully recover in the case that the keyring was full (but it still doesn't). This actually doesn't have to change, since the strcmp()s that use it will work either way -- but it might catch someone by surprise later. I'll split this out.

> Finally, what -n value do plan on using? Maybe a blurb in the man page
> on what a good number is and why....

I've got this running now with -n160, since we have ~60K distinct uid/gid s. Ideally, I'd like to re-submit this to self-scale which wouldn't require any sysadmin tuning, but I haven't had the time. Really, this is just a quick fix for the brokenness that's in current RHEL and less-new Fedora. If you want it done right it will take me a week or two to find the time.

Ben

> steved.
>
>> #endif
>>
>> #ifndef PATH_IDMAPDCONF
>> @@ -39,7 +39,7 @@ static int keyring_clear(char *keyring);
>> /*
>> * Find either a user or group id based on the name@domain string
>> */
>> -int id_lookup(char *name_at_domain, key_serial_t key, int type)
>> +int id_lookup(char *name_at_domain, key_serial_t key, int type, key_serial_t dest_keyring)
>> {
>> char id[MAX_ID_LEN];
>> uid_t uid = 0;
>> @@ -58,7 +58,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
>> (type == USER ? "nfs4_owner_to_uid" : "nfs4_group_owner_to_gid"));
>>
>> if (rc == 0) {
>> - rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
>> + rc = keyctl_instantiate(key, id, strlen(id) + 1, dest_keyring);
>> if (rc < 0) {
>> switch(rc) {
>> case -EDQUOT:
>> @@ -67,9 +67,9 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
>> /*
>> * The keyring is full. Clear the keyring and try again
>> */
>> - rc = keyring_clear(DEFAULT_KEYRING);
>> + rc = keyctl_clear(dest_keyring);
>> if (rc == 0)
>> - rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
>> + rc = keyctl_instantiate(key, id, strlen(id) + 1, dest_keyring);
>> break;
>> default:
>> break;
>> @@ -85,7 +85,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
>> /*
>> * Find the name@domain string from either a user or group id
>> */
>> -int name_lookup(char *id, key_serial_t key, int type)
>> +int name_lookup(char *id, key_serial_t key, int type, key_serial_t dest_keyring)
>> {
>> char name[IDMAP_NAMESZ];
>> char domain[NFS4_MAX_DOMAIN_LEN];
>> @@ -113,7 +113,7 @@ int name_lookup(char *id, key_serial_t key, int type)
>> (type == USER ? "nfs4_uid_to_name" : "nfs4_gid_to_name"));
>>
>> if (rc == 0) {
>> - rc = keyctl_instantiate(key, &name, strlen(name), 0);
>> + rc = keyctl_instantiate(key, &name, strlen(name), dest_keyring);
>> if (rc < 0)
>> xlog_err("name_lookup: keyctl_instantiate failed: %m");
>> }
>> @@ -127,7 +127,7 @@ static int keyring_clear(char *keyring)
>> {
>> FILE *fp;
>> char buf[BUFSIZ];
>> - key_serial_t key;
>> + key_serial_t key, child_key;
>>
>> if (keyring == NULL)
>> keyring = DEFAULT_KEYRING;
>> @@ -151,6 +151,7 @@ static int keyring_clear(char *keyring)
>> */
>> *(strchr(buf, ' ')) = '\0';
>> sscanf(buf, "%x", &key);
>> +
>> if (keyctl_clear(key) < 0) {
>> xlog_err("keyctl_clear(0x%x) failed: %m", key);
>> fclose(fp);
>> @@ -159,7 +160,8 @@ static int keyring_clear(char *keyring)
>> fclose(fp);
>> return 0;
>> }
>> - xlog_err("'%s' keyring was not found.", keyring);
>> + if (strstr(keyring, DEFAULT_KEYRING":"))
>> + xlog_err("'%s' keyring was not found.", keyring);
>> fclose(fp);
>> return 1;
>> }
>> @@ -232,8 +234,10 @@ int main(int argc, char **argv)
>> char *type;
>> int rc = 1, opt;
>> int timeout = 600;
>> - key_serial_t key;
>> + int childrings = 0;
>> + key_serial_t key, parent_keyring, dest_keyring;
>> char *progname, *keystr = NULL;
>> + char child_name[BUFSIZ];
>> int clearing = 0, keymask = 0;
>>
>> /* Set the basename */
>> @@ -244,7 +248,7 @@ int main(int argc, char **argv)
>>
>> xlog_open(progname);
>>
>> - while ((opt = getopt(argc, argv, "u:g:r:ct:v")) != -1) {
>> + while ((opt = getopt(argc, argv, "u:g:r:ct:vn:d:")) != -1) {
>> switch (opt) {
>> case 'u':
>> keymask = UIDKEYS;
>> @@ -267,6 +271,9 @@ int main(int argc, char **argv)
>> case 't':
>> timeout = atoi(optarg);
>> break;
>> + case 'n':
>> + childrings = atoi(optarg);
>> + break;
>> default:
>> xlog_warn(usage, progname);
>> break;
>> @@ -284,9 +291,16 @@ int main(int argc, char **argv)
>> rc = key_revoke(keystr, keymask);
>> return rc;
>> }
>> +
>> if (clearing) {
>> xlog_syslog(0);
>> - rc = keyring_clear(DEFAULT_KEYRING);
>> + int i = 1;
>> + for(rc = 0; rc == 0; i++) {
>> + snprintf(child_name, sizeof(child_name), DEFAULT_KEYRING "_child_%d", i);
>> + rc = keyring_clear(child_name);
>> + }
>> +
>> + rc = keyring_clear(DEFAULT_KEYRING ":");
>> return rc;
>> }
>>
>> @@ -315,14 +329,42 @@ int main(int argc, char **argv)
>> key, type, value, timeout);
>> }
>>
>> + if (childrings) {
>> + int i;
>> + long child_size, smallest_size = 2032;
>> + parent_keyring = request_key("keyring", DEFAULT_KEYRING, NULL, KEY_SPEC_THREAD_KEYRING);
>> +
>> + for (i = 1; i <= childrings; i++) {
>> + key_serial_t child_keyring;
>> +
>> + snprintf(child_name, sizeof(child_name), DEFAULT_KEYRING "_child_%d", i);
>> +
>> + child_keyring = keyctl_search(parent_keyring, "keyring", child_name, 0);
>> + if (child_keyring < 0) {
>> + child_keyring = add_key("keyring", child_name, NULL, 0, parent_keyring);
>> + xlog_warn("added new child %s: %m", child_name);
>> + }
>> +
>> + child_size = keyctl_read(child_keyring, NULL, 0);
>> + if (child_size < smallest_size) {
>> + dest_keyring = child_keyring;
>> + smallest_size = child_size;
>> + }
>> + }
>> + }
>> +
>> if (strcmp(type, "uid") == 0)
>> - rc = id_lookup(value, key, USER);
>> + rc = id_lookup(value, key, USER, dest_keyring);
>> else if (strcmp(type, "gid") == 0)
>> - rc = id_lookup(value, key, GROUP);
>> + rc = id_lookup(value, key, GROUP, dest_keyring);
>> else if (strcmp(type, "user") == 0)
>> - rc = name_lookup(value, key, USER);
>> + rc = name_lookup(value, key, USER, dest_keyring);
>> else if (strcmp(type, "group") == 0)
>> - rc = name_lookup(value, key, GROUP);
>> + rc = name_lookup(value, key, GROUP, dest_keyring);
>> +
>> + /* if we hung this off a child, unlink from the parent */
>> + if (dest_keyring)
>> + keyctl_unlink(key, parent_keyring);
>>
>> /* Set timeout to 10 (600 seconds) minutes */
>> if (rc == 0)
>> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
>> index 3a3a523..04f0014 100644
>> --- a/utils/nfsidmap/nfsidmap.man
>> +++ b/utils/nfsidmap/nfsidmap.man
>> @@ -6,7 +6,7 @@
>> .SH NAME
>> nfsidmap \- The NFS idmapper upcall program
>> .SH SYNOPSIS
>> -.B "nfsidmap [-v] [-t timeout] key desc"
>> +.B "nfsidmap [-v] [-t timeout] [-n count] key desc"
>> .br
>> .B "nfsidmap [-v] [-c]"
>> .br
>> @@ -42,6 +42,9 @@ Revoke both the uid and gid key of the given user.
>> Set the expiration timer, in seconds, on the key.
>> The default is 600 seconds (10 mins).
>> .TP
>> +.B -n count
>> +Set the the number of child keyrings to create.
>> +.TP
>> .B -u user
>> Revoke the uid key of the given user.
>> .TP
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-03-24 23:57:29

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On 03/24/2014 03:51 PM, Steve Dickson wrote:
>
>
> On 03/24/2014 02:00 PM, Benjamin Coddington wrote:
>>
>> On Mar 24, 2014, at 1:00 PM, Steve Dickson <[email protected]> wrote:
>>
>>> On 03/21/2014 05:08 PM, Benjamin Coddington wrote:
>>>> The kernel keyring has a max of ~508 entries on 64-bit systems.
>>>> For installations with more distict users than this limit, create
>>>> a specified number of child keyrings and fill them evenly.
>>> A couple things...
>>>
>>> 1) no Signed-off-by: line
>>>
>>> 2) Its seems you can create key rings but can't delete them.
>>> Here is what I'm doing:
>>> in /etc/request-key.d/id_resolver.conf I have
>>> create id_resolver * * /usr/sbin/nfsidmap -n 10 %k %d
>>> but when I tried to delete the keys
>>> # nfsidmap -vc
>>> nfsidmap: clearing '08aa156c I--Q--- 1perm 3f010000 0 0 keyring .id_resolver_child_10: empty'
>>> nfsidmap: keyctl_clear(0x8aa156c) failed: Permission denied
>>
>> This mess works on my fleet of RHEL6 boxes which is where I was trying to fix this. They create the child keyrings with
>>
>> perm 3b3f0000
>>
>> Instead of yours which appears to be
>>
>> perm 3f010000
>>
>> Are you testing on a later kernel? Likely this behavior has changed.
> Yes... Much later...
>
>>
>>>> #define PROCKEYS "/proc/keys"
>>>> #ifndef DEFAULT_KEYRING
>>>> -#define DEFAULT_KEYRING "id_resolver"
>>>> +#define DEFAULT_KEYRING ".id_resolver"
>>> 3) Why is changing the default needed?
>>
>> The default is wrong. I think that's the first thing I changed when
>> trying to fix this problem, since it looked like id_lookup() should
>> gracefully recover in the case that the keyring was full
>> (but it still doesn't).
> I'm think the "id_resolver" default can from the face
> the entry /etc/request-key.d/id_resolver.conf
> which tells nfsidmap put the keys on the id_resolver
> key ring... so I'm not really sure where the
> .id_resolver is coming from... CC-ing David Howells
> maybe he knows...
To translate in king's English...

The reason the default is "id_resolver" is because the
is the name of the key ring defined in id_resolver.conf
is id_resolver. Now how that is translated into ".id_resolver"
in /proc/keys is not clear....

steved.

2014-03-25 09:29:16

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

Steve Dickson <[email protected]> wrote:

> The reason the default is "id_resolver" is because the
> is the name of the key ring defined in id_resolver.conf
> is id_resolver. Now how that is translated into ".id_resolver"
> in /proc/keys is not clear....

Where in /etc/request-key.d/id_resolver.conf does it mention the name of a
keyring?

create id_resolver * * /usr/sbin/nfsidmap %k %d

Match it against this line from request-key.conf:

#OP TYPE DESCRIPTION CALLOUT INFO PROGRAM ARG1 ARG2 ARG3 ...

The "id_resolver" here is the name of the *key type* to be matched for that
line. There is nothing here to do with keyrings.

David

2014-03-25 10:41:23

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On 03/25/2014 05:29 AM, David Howells wrote:
> Steve Dickson <[email protected]> wrote:
>
>> The reason the default is "id_resolver" is because the
>> is the name of the key ring defined in id_resolver.conf
>> is id_resolver. Now how that is translated into ".id_resolver"
>> in /proc/keys is not clear....
>
> Where in /etc/request-key.d/id_resolver.conf does it mention the name of a
> keyring?
>
> create id_resolver * * /usr/sbin/nfsidmap %k %d
I though "id_resolver" was the name.

>
> Match it against this line from request-key.conf:
>
> #OP TYPE DESCRIPTION CALLOUT INFO PROGRAM ARG1 ARG2 ARG3 ...
I should probably document this in request-key.conf.

>
> The "id_resolver" here is the name of the *key type* to be matched for that
> line. There is nothing here to do with keyrings.
Thanks for the clarification...

steved.

>
> David
>

2014-03-24 17:04:41

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On 03/21/2014 05:08 PM, Benjamin Coddington wrote:
> The kernel keyring has a max of ~508 entries on 64-bit systems.
> For installations with more distict users than this limit, create
> a specified number of child keyrings and fill them evenly.
A couple things...

1) no Signed-off-by: line

2) Its seems you can create key rings but can't delete them.
Here is what I'm doing:
in /etc/request-key.d/id_resolver.conf I have
create id_resolver * * /usr/sbin/nfsidmap -n 10 %k %d
but when I tried to delete the keys
# nfsidmap -vc
nfsidmap: clearing '08aa156c I--Q--- 1 perm 3f010000 0 0 keyring .id_resolver_child_10: empty'
nfsidmap: keyctl_clear(0x8aa156c) failed: Permission denied

> ---
> utils/nfsidmap/nfsidmap.c | 76 +++++++++++++++++++++++++++++++++---------
> utils/nfsidmap/nfsidmap.man | 5 ++-
> 2 files changed, 63 insertions(+), 18 deletions(-)
>
> diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
> index 2518ed6..41abede 100644
> --- a/utils/nfsidmap/nfsidmap.c
> +++ b/utils/nfsidmap/nfsidmap.c
> @@ -15,7 +15,7 @@
> #include "conffile.h"
>
> int verbose = 0;
> -char *usage="Usage: %s [-v] [-c || [-u|-g|-r key] || [-t timeout] key desc]";
> +char *usage="Usage: %s [-v] [-c || [-u|-g|-r key] || [-t timeout] [-n count] key desc]";
>
> #define MAX_ID_LEN 11
> #define IDMAP_NAMESZ 128
> @@ -24,7 +24,7 @@ char *usage="Usage: %s [-v] [-c || [-u|-g|-r key] || [-t timeout] key desc]";
>
> #define PROCKEYS "/proc/keys"
> #ifndef DEFAULT_KEYRING
> -#define DEFAULT_KEYRING "id_resolver"
> +#define DEFAULT_KEYRING ".id_resolver"
3) Why is changing the default needed?

Finally, what -n value do plan on using? Maybe a blurb in the man page
on what a good number is and why....

steved.

> #endif
>
> #ifndef PATH_IDMAPDCONF
> @@ -39,7 +39,7 @@ static int keyring_clear(char *keyring);
> /*
> * Find either a user or group id based on the name@domain string
> */
> -int id_lookup(char *name_at_domain, key_serial_t key, int type)
> +int id_lookup(char *name_at_domain, key_serial_t key, int type, key_serial_t dest_keyring)
> {
> char id[MAX_ID_LEN];
> uid_t uid = 0;
> @@ -58,7 +58,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
> (type == USER ? "nfs4_owner_to_uid" : "nfs4_group_owner_to_gid"));
>
> if (rc == 0) {
> - rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
> + rc = keyctl_instantiate(key, id, strlen(id) + 1, dest_keyring);
> if (rc < 0) {
> switch(rc) {
> case -EDQUOT:
> @@ -67,9 +67,9 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
> /*
> * The keyring is full. Clear the keyring and try again
> */
> - rc = keyring_clear(DEFAULT_KEYRING);
> + rc = keyctl_clear(dest_keyring);
> if (rc == 0)
> - rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
> + rc = keyctl_instantiate(key, id, strlen(id) + 1, dest_keyring);
> break;
> default:
> break;
> @@ -85,7 +85,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
> /*
> * Find the name@domain string from either a user or group id
> */
> -int name_lookup(char *id, key_serial_t key, int type)
> +int name_lookup(char *id, key_serial_t key, int type, key_serial_t dest_keyring)
> {
> char name[IDMAP_NAMESZ];
> char domain[NFS4_MAX_DOMAIN_LEN];
> @@ -113,7 +113,7 @@ int name_lookup(char *id, key_serial_t key, int type)
> (type == USER ? "nfs4_uid_to_name" : "nfs4_gid_to_name"));
>
> if (rc == 0) {
> - rc = keyctl_instantiate(key, &name, strlen(name), 0);
> + rc = keyctl_instantiate(key, &name, strlen(name), dest_keyring);
> if (rc < 0)
> xlog_err("name_lookup: keyctl_instantiate failed: %m");
> }
> @@ -127,7 +127,7 @@ static int keyring_clear(char *keyring)
> {
> FILE *fp;
> char buf[BUFSIZ];
> - key_serial_t key;
> + key_serial_t key, child_key;
>
> if (keyring == NULL)
> keyring = DEFAULT_KEYRING;
> @@ -151,6 +151,7 @@ static int keyring_clear(char *keyring)
> */
> *(strchr(buf, ' ')) = '\0';
> sscanf(buf, "%x", &key);
> +
> if (keyctl_clear(key) < 0) {
> xlog_err("keyctl_clear(0x%x) failed: %m", key);
> fclose(fp);
> @@ -159,7 +160,8 @@ static int keyring_clear(char *keyring)
> fclose(fp);
> return 0;
> }
> - xlog_err("'%s' keyring was not found.", keyring);
> + if (strstr(keyring, DEFAULT_KEYRING":"))
> + xlog_err("'%s' keyring was not found.", keyring);
> fclose(fp);
> return 1;
> }
> @@ -232,8 +234,10 @@ int main(int argc, char **argv)
> char *type;
> int rc = 1, opt;
> int timeout = 600;
> - key_serial_t key;
> + int childrings = 0;
> + key_serial_t key, parent_keyring, dest_keyring;
> char *progname, *keystr = NULL;
> + char child_name[BUFSIZ];
> int clearing = 0, keymask = 0;
>
> /* Set the basename */
> @@ -244,7 +248,7 @@ int main(int argc, char **argv)
>
> xlog_open(progname);
>
> - while ((opt = getopt(argc, argv, "u:g:r:ct:v")) != -1) {
> + while ((opt = getopt(argc, argv, "u:g:r:ct:vn:d:")) != -1) {
> switch (opt) {
> case 'u':
> keymask = UIDKEYS;
> @@ -267,6 +271,9 @@ int main(int argc, char **argv)
> case 't':
> timeout = atoi(optarg);
> break;
> + case 'n':
> + childrings = atoi(optarg);
> + break;
> default:
> xlog_warn(usage, progname);
> break;
> @@ -284,9 +291,16 @@ int main(int argc, char **argv)
> rc = key_revoke(keystr, keymask);
> return rc;
> }
> +
> if (clearing) {
> xlog_syslog(0);
> - rc = keyring_clear(DEFAULT_KEYRING);
> + int i = 1;
> + for(rc = 0; rc == 0; i++) {
> + snprintf(child_name, sizeof(child_name), DEFAULT_KEYRING "_child_%d", i);
> + rc = keyring_clear(child_name);
> + }
> +
> + rc = keyring_clear(DEFAULT_KEYRING ":");
> return rc;
> }
>
> @@ -315,14 +329,42 @@ int main(int argc, char **argv)
> key, type, value, timeout);
> }
>
> + if (childrings) {
> + int i;
> + long child_size, smallest_size = 2032;
> + parent_keyring = request_key("keyring", DEFAULT_KEYRING, NULL, KEY_SPEC_THREAD_KEYRING);
> +
> + for (i = 1; i <= childrings; i++) {
> + key_serial_t child_keyring;
> +
> + snprintf(child_name, sizeof(child_name), DEFAULT_KEYRING "_child_%d", i);
> +
> + child_keyring = keyctl_search(parent_keyring, "keyring", child_name, 0);
> + if (child_keyring < 0) {
> + child_keyring = add_key("keyring", child_name, NULL, 0, parent_keyring);
> + xlog_warn("added new child %s: %m", child_name);
> + }
> +
> + child_size = keyctl_read(child_keyring, NULL, 0);
> + if (child_size < smallest_size) {
> + dest_keyring = child_keyring;
> + smallest_size = child_size;
> + }
> + }
> + }
> +
> if (strcmp(type, "uid") == 0)
> - rc = id_lookup(value, key, USER);
> + rc = id_lookup(value, key, USER, dest_keyring);
> else if (strcmp(type, "gid") == 0)
> - rc = id_lookup(value, key, GROUP);
> + rc = id_lookup(value, key, GROUP, dest_keyring);
> else if (strcmp(type, "user") == 0)
> - rc = name_lookup(value, key, USER);
> + rc = name_lookup(value, key, USER, dest_keyring);
> else if (strcmp(type, "group") == 0)
> - rc = name_lookup(value, key, GROUP);
> + rc = name_lookup(value, key, GROUP, dest_keyring);
> +
> + /* if we hung this off a child, unlink from the parent */
> + if (dest_keyring)
> + keyctl_unlink(key, parent_keyring);
>
> /* Set timeout to 10 (600 seconds) minutes */
> if (rc == 0)
> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
> index 3a3a523..04f0014 100644
> --- a/utils/nfsidmap/nfsidmap.man
> +++ b/utils/nfsidmap/nfsidmap.man
> @@ -6,7 +6,7 @@
> .SH NAME
> nfsidmap \- The NFS idmapper upcall program
> .SH SYNOPSIS
> -.B "nfsidmap [-v] [-t timeout] key desc"
> +.B "nfsidmap [-v] [-t timeout] [-n count] key desc"
> .br
> .B "nfsidmap [-v] [-c]"
> .br
> @@ -42,6 +42,9 @@ Revoke both the uid and gid key of the given user.
> Set the expiration timer, in seconds, on the key.
> The default is 600 seconds (10 mins).
> .TP
> +.B -n count
> +Set the the number of child keyrings to create.
> +.TP
> .B -u user
> Revoke the uid key of the given user.
> .TP
>

2014-03-25 00:15:47

[permalink] [raw]

Subject: Re: [PATCH] nfsidmap: use multiple child keyrings

On Mar 24, 2014, at 7:57 PM, Steve Dickson <[email protected]> wrote:

>
> On 03/24/2014 03:51 PM, Steve Dickson wrote:
>>
>>
>> On 03/24/2014 02:00 PM, Benjamin Coddington wrote:
>>>
>>> On Mar 24, 2014, at 1:00 PM, Steve Dickson <[email protected]> wrote:
>>>
>>>> On 03/21/2014 05:08 PM, Benjamin Coddington wrote:
>>>>> The kernel keyring has a max of ~508 entries on 64-bit systems.
>>>>> For installations with more distict users than this limit, create
>>>>> a specified number of child keyrings and fill them evenly.
>>>> A couple things...
>>>>
>>>> 1) no Signed-off-by: line
>>>>
>>>> 2) Its seems you can create key rings but can't delete them.
>>>> Here is what I'm doing:
>>>> in /etc/request-key.d/id_resolver.conf I have
>>>> create id_resolver * * /usr/sbin/nfsidmap -n 10 %k %d
>>>> but when I tried to delete the keys
>>>> # nfsidmap -vc
>>>> nfsidmap: clearing '08aa156c I--Q--- 1perm 3f010000 0 0 keyring .id_resolver_child_10: empty'
>>>> nfsidmap: keyctl_clear(0x8aa156c) failed: Permission denied
>>>
>>> This mess works on my fleet of RHEL6 boxes which is where I was trying to fix this. They create the child keyrings with
>>>
>>> perm 3b3f0000
>>>
>>> Instead of yours which appears to be
>>>
>>> perm 3f010000
>>>
>>> Are you testing on a later kernel? Likely this behavior has changed.
>> Yes... Much later...
>>
>>>
>>>>> #define PROCKEYS "/proc/keys"
>>>>> #ifndef DEFAULT_KEYRING
>>>>> -#define DEFAULT_KEYRING "id_resolver"
>>>>> +#define DEFAULT_KEYRING ".id_resolver"
>>>> 3) Why is changing the default needed?
>>>
>>> The default is wrong. I think that's the first thing I changed when
>>> trying to fix this problem, since it looked like id_lookup() should
>>> gracefully recover in the case that the keyring was full
>>> (but it still doesn't).
>> I'm think the "id_resolver" default can from the face
>> the entry /etc/request-key.d/id_resolver.conf
>> which tells nfsidmap put the keys on the id_resolver
>> key ring... so I'm not really sure where the
>> .id_resolver is coming from... CC-ing David Howells
>> maybe he knows...
> To translate in king's English...
>
> The reason the default is "id_resolver" is because the
> is the name of the key ring defined in id_resolver.conf
> is id_resolver. Now how that is translated into ".id_resolver"
> in /proc/keys is not clear....

The idmapper in the kernel defines a key type as 'id_resolver', which is what request-key uses to map to a helper - but it also creates a keyring called ".id_resolver". This is the keyring of inadequate capacity. It is this keyring name that nfsidmap looks for in /proc/keys as DEFAULT_KEYRING, and so DEFAULT_KEYRING should match the kernel's keyring name exactly if nfsidmap wants to use "request_key" during the instantiation to become a possessor (which is needed to have the right permissions to hang further keys off this keyring).

Ben

2014-03-24 21:03:32