[Pkg-shadow-devel] Newuidmap works with usernames instead of uids

Sat Sep 6 08:10:54 UTC 2014

Hi,

The performance hit with getpw* is getting much worse when you have a
remote database (e.g ldap).

You may consider checking the range first as this does not require syscall,
and then checking the user.

Kind regards,
-- 
Nekral

Le samedi 6 septembre 2014, <bostjan at a2o.si> a écrit :
> Heh, I just retested this with 1 000 000 entries, but this time
> entries were specified with UID instead of username. Result: 0.310s!
>
> It seems that getpwnam() returns very quickly if you pass it UID
> (string) instread of actual username.
>
> Anyway, I have added a note to subuid/subgid man pages, pushed to -v3
> branch this commit too.
>
> b.
>
>
>
>
> On 6 September 2014 00:14, Bostjan Skufca <bostjan at a2o.si> wrote:
>> Also, I  ran some benchmarks of the old and new code, under the
>> following conditions:
>> - modest i3 3.3GHz machine, fairly unloaded
>> - 1.000.000 entries in /etc/subuid
>> - command: time ./src/newuidmap 8260 0 100000 100
>> - command fails as there is no appropriate entry in /etc/subuid
>> - results repeated several times, average taken
>>
>> Here are the results(n=1 000 000):
>> - 0.290s: Single loop, no syscalls (old behaviour)
>> - 0.320s: Double loop, no syscalls (new behaviour, but disabled all
>> syscalls in the second loop, temporarily)
>> - 7.500s: Double loop, with getpwnam() syscalls in second loop
>>
>> This last entry should concern us, but I think that people who intend
>> to put 1M records into /etc/subuid, will gladly write their own tools
>> to optimize this.
>>
>> I rerun the test, with single loop without syscalls, and varied the
>> number of entries in /etc/subuid. Results:
>> - 0.001s: with      1000 entries
>> - 0.004s: with    10 000 entries
>> - 0.032s: with   100 000 entries
>> - 0.290s: with 1 000 000 entries
>>
>> Double loop without syscalls, and varied the number of entries in
>> /etc/subuid. Results:
>> - 0.001s: with      1000 entries
>> - 0.004s: with    10 000 entries
>> - 0.033s: with   100 000 entries
>> - 0.300s: with 1 000 000 entries
>>
>> Obviously, number of loops does not matter all that much if no
>> syscalls are used. Also, second loop is WAY faster than the first one.
>>
>> Last test I did was with the final patch applied as-is. Double loop,
>> with syscalls in the second loop. Results:
>> - 0.009s: with      1000 entries
>> - 0.075s: with    10 000 entries
>> - 0.740s: with   100 000 entries
>> - 7.500s: with 1 000 000 entries
>>
>> Realistically I believe that people will max this implementation at
>> around 100-500 entries in the /etc/sub[ug]id. I think that this
>> implementation performs adequately in that area.
>> (I do hope I do NOT get famous for "500 entries in subuid ought to be
>> enough for everybody":)
>>
>>
>> b.
>>
>> On 5 September 2014 23:53, Bostjan Skufca <bostjan at a2o.si> wrote:
>>>>> I created new branch for this one, for you to merge one single commig:
>>>>> https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2
>>>>
>>>> My 2c:
>>>> Do you need to include <grp.h>?
>>>
>>> Leftovers. When I started looking into this I was under impression
>>> that uidmap is determined by user's UID and gidmap is determined by
>>> user's GID. It turned out getgwnam() was not needed at all, as
>>> everything is UID-based.
>>>
>>>
>>>> You could avoid the getpwnam_r call (and malloc, sysconf) by calling
>>>> getpwnam. You just need to store owner_pwd.pw_uid before you enter the
>>>> loop.
>>>> This would also avoid the call to exit(), which IMO should be avoided
in
>>>> commonio.c (better return an error code and let the applications deal
with
>>>> the error - this can let them release a lock for example).
>>>
>>> So obvious :)
>>> (I copy-pasted the code strainght from getpwnam man page, that is why
>>> exit()s were there instead of proper error handling.)
>>>
>>> Unfortunately there is not a single instance of error handling in
>>> lib/subordinateio.c to take as example, and I am not sure that calling
>>> code knows how to handle errors other than "requested uidmap not
>>> allowed", which is what "return NULL;" stands for.
>>>
>>> Therefore ideas about how to do proper error handling are very
>>> welcome, but I think they belong to a separate thread.
>>>
>>>
>>>> I'm not sure regarding the guarantees you have on the size of pw_uid.
>>>> I would recommend:
>>>>         sprintf(owner_uid_string, "%lu", (unsigned long
int)owner_pwd.pw_uid);
>>>
>>> Sensible, fixed.
>>>
>>>
>>>> This is rather cosmetic, but
>>>>                         if (!range_owner_pwd) {
>>>> could be changed to:
>>>>                         if (NULL == range_owner_pwd) {
>>>
>>> Future-proofing of course, fixed that too.
>>>
>>>
>>> Commit with fixes to review:
>>>
https://github.com/bostjan/shadow/commit/d21214c9ba823edd5ac82ae2b1b03f4c219c3841
>>>
>>> Branch to actually merge (contains single commit with everything
>>> squashed into it):
>>> https://github.com/bostjan/shadow/tree/feature/subuidgid-numeric-v3
>>>
>>>
>>> @Serge:
>>> Maybe it would be for the best if you coordinated with Christian
>>> inclusion of this patch into upstream, as you are already "in the
>>> upstream-patch-pushing zone" :)
>>>
>>>
>>> b.
>

-- 
Nekral
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pkg-shadow-devel/attachments/20140906/24eccf4e/attachment.html>