Bug 253959 - service(8) env call with LDAP makes boot take many hours
Summary: service(8) env call with LDAP makes boot take many hours
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Only Me
Assignee: Kyle Evans
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-02 16:36 UTC by Brad Davis
Modified: 2021-03-17 00:12 UTC (History)
2 users (show)

See Also:
brd: mfc-stable13?
kevans: mfc-stable12?
kevans: mfc-stable11-


Attachments
PoC (4.51 KB, patch)
2021-03-02 17:16 UTC, Brad Davis
no flags Details | Diff
Make service(8) environment more consistent with init(8). (2.39 KB, patch)
2021-03-03 03:25 UTC, andrew
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brad Davis freebsd_committer 2021-03-02 16:36:16 UTC
https://cgit.freebsd.org/src/commit/usr.sbin/service?id=736a5a6d1dbbdae68eb102c2ba9c114aafd61821

This change makes env try to query LDAP servers before the network has started and coupled with the devd change to use service(8) to launch devmatch makes it even worse.
Comment 1 Brad Davis freebsd_committer 2021-03-02 16:46:29 UTC
FWIW, I think that this change is a *good* thing, but wondering if there is some way to mitigate env(1) or PAM from being so.. brain dead?
Comment 2 Kyle Evans freebsd_committer 2021-03-02 16:48:06 UTC
(In reply to Brad Davis from comment #1)

I suspect that that would require some more extensive hacking-in, but I've CC'd Andrew to solicit his opinion.
Comment 3 Brad Davis freebsd_committer 2021-03-02 17:03:22 UTC
Maybe a flag to service to prevent adding `-L 0/daemon' ?

That way when service(8) is called via devd it can be disabled?  Granted that doesn't fix all of it, but does take away at least a good portion of the pain.
Comment 4 Brad Davis freebsd_committer 2021-03-02 17:16:48 UTC
Created attachment 222920 [details]
PoC

I created a quick patch to test out..
Comment 5 andrew 2021-03-02 21:18:45 UTC
I think that is completely the wrong approach - let me check something
Comment 6 andrew 2021-03-02 21:25:26 UTC
Brad, can you show all the changes you made to your base system config in order to use pam_ldap?
Comment 7 andrew 2021-03-03 03:25:56 UTC
Created attachment 222931 [details]
Make service(8) environment more consistent with init(8).

Does this patch help at all?

I can't see any way for env to end up invoking pam, but with -L it does do getpw* functions that might go through nsswitch, which could be an issue. So this avoids doing that, and also fixes up an inconsistency between what service(8) was doing and what init(8) does.
Comment 8 Kyle Evans freebsd_committer 2021-03-03 18:23:30 UTC
(In reply to andrew from comment #7)

IMO this is fairly low-impact and an obviously good idea, so I'm going to go ahead and commit it to get some appropriate soak time in given the stage of the release cycle we're at.
Comment 9 commit-hook freebsd_committer 2021-03-03 18:29:28 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=55deb0a5f089c8a27cfc1666655b93881c2b47ae

commit 55deb0a5f089c8a27cfc1666655b93881c2b47ae
Author:     Andrew Gierth <andrew@tao146.riddles.org.uk>
AuthorDate: 2021-03-03 18:25:11 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2021-03-03 18:25:11 +0000

    service(8): use an environment more consistent with init(8)

    init(8) sets the "daemon" login class without specifying a pw
    entry (so no substitutions are done on the variables). service(8)'s
    use of env -L had the effect of specifying root's pw entry, with two
    effects: getpwnam and getpwuid are being called, which may not be
    entirely safe depending on what nsswitch is up to and what stage of
    boot we are at, and substitutions would have been done.

    Fix by teaching env(8) to allow -L -/classname to set the class
    environment with no pw entry at all specified, and use it in
    service(8).

    PR:             253959

 usr.bin/env/env.1           |  7 ++++++-
 usr.bin/env/env.c           | 25 ++++++++++++++++---------
 usr.sbin/service/service.sh |  2 +-
 3 files changed, 23 insertions(+), 11 deletions(-)
Comment 10 Brad Davis freebsd_committer 2021-03-04 16:06:26 UTC
This works, thank you!

Starting a CLI argument with a dash seems kinda ugly..
Comment 11 commit-hook freebsd_committer 2021-03-07 21:40:10 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=872ec7e5b6f35d84745b49c02f58572632de22ed

commit 872ec7e5b6f35d84745b49c02f58572632de22ed
Author:     Andrew Gierth <andrew@tao146.riddles.org.uk>
AuthorDate: 2021-03-03 18:25:11 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2021-03-07 21:39:30 +0000

    service(8): use an environment more consistent with init(8)

    init(8) sets the "daemon" login class without specifying a pw
    entry (so no substitutions are done on the variables). service(8)'s
    use of env -L had the effect of specifying root's pw entry, with two
    effects: getpwnam and getpwuid are being called, which may not be
    entirely safe depending on what nsswitch is up to and what stage of
    boot we are at, and substitutions would have been done.

    Fix by teaching env(8) to allow -L -/classname to set the class
    environment with no pw entry at all specified, and use it in
    service(8).

    PR:             253959

    (cherry picked from commit 55deb0a5f089c8a27cfc1666655b93881c2b47ae)
    (cherry picked from commit 0c1a5eaae83267365330437adb60f44e1a622a2b)

 usr.bin/env/env.1           |  7 ++++++-
 usr.bin/env/env.c           | 25 ++++++++++++++++---------
 usr.sbin/service/service.sh |  2 +-
 3 files changed, 23 insertions(+), 11 deletions(-)
Comment 12 commit-hook freebsd_committer 2021-03-17 00:12:09 UTC
A commit in branch releng/13.0 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=cb370b19715b696cf6db4f7b357cf2e7f2e3adb7

commit cb370b19715b696cf6db4f7b357cf2e7f2e3adb7
Author:     Andrew Gierth <andrew@tao146.riddles.org.uk>
AuthorDate: 2021-03-03 18:25:11 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2021-03-17 00:11:23 +0000

    service(8): use an environment more consistent with init(8)

    init(8) sets the "daemon" login class without specifying a pw
    entry (so no substitutions are done on the variables). service(8)'s
    use of env -L had the effect of specifying root's pw entry, with two
    effects: getpwnam and getpwuid are being called, which may not be
    entirely safe depending on what nsswitch is up to and what stage of
    boot we are at, and substitutions would have been done.

    Fix by teaching env(8) to allow -L -/classname to set the class
    environment with no pw entry at all specified, and use it in
    service(8).

    PR:             253959
    Approved by:    re (gjb)

    (cherry picked from commit 55deb0a5f089c8a27cfc1666655b93881c2b47ae)
    (cherry picked from commit 0c1a5eaae83267365330437adb60f44e1a622a2b)
    (cherry picked from commit 872ec7e5b6f35d84745b49c02f58572632de22ed)

 usr.bin/env/env.1           |  7 ++++++-
 usr.bin/env/env.c           | 25 ++++++++++++++++---------
 usr.sbin/service/service.sh |  2 +-
 3 files changed, 23 insertions(+), 11 deletions(-)
Comment 13 Kyle Evans freebsd_committer 2021-03-17 00:12:43 UTC
In time for 13.0-rc3; thanks, folks.