smbd failed in host when both lxd container and host have smbd

This is "just one more" case of the common old init scripts didn't consider there could be more (=containers).

Interesting is that the stop already runs with the --pidfile so the stop will not kill it.

But the start will be blocked, as the existance of such a process will make --start be a no-op.

Man page:
Note: unless --pid or --pidfile are specified, start-stop-daemon behaves similar to killall(1). start-stop-daemon will scan the process table looking for any processes which match the process name, parent pid, uid, and/or gid (if specified). Any matching process will prevent --start from starting the daemon. All matching processes will be sent the TERM signal (or the one specified via --signal or --retry) if --stop is specified. For daemons which have long-lived children which need to live through a --stop, you must specify a pidfile.

-S, --start [--] arguments
Check for the existence of a specified process. If such a process exists, start-stop-daemon does nothing, and exits with error status 1 (0 if --oknodo is specified). If such a process does not exist, it starts an instance, using either the executable specified by --exec or, if specified, by --startas. Any arguments given after -- on the command line are passed unmodified to the program being started.

The --oknodo will make it a silent non fatal exit int hat case - as it is fine to run "start" if it is running already.

I'd recommend "--pidfile $SMBDPID" instead of the suggested path, but otherwise would agree to the fix.

It should be safe as that is essentially how later versions (Bionic) do it (via MAINPID tracking in systemd).

This will eventually be interesting for Trusty as well (added a bug task), but lets discuss and fix Xenial first to avoid revising two branches all the time.

To avoid unintended processes to be acted on we should combine that with exec.
Man page:
-p, --pidfile pid-file
Check whether a process has created the file pid-file. Note: using this matching option alone might cause unintended processes to be acted on, if the old process terminated without being able to remove the pid-file.

Text would be like:
Let daemon start/stop actions only using --exec use --pidfile as well to avoid affecting or being affected by processed of the same name in containers. Let calls only using pidfile add --exec to avoid acting on processes reusing the PIDs.

There is some detail that I'm not sure it was intended or not and we have to be careful.
- debian/samba.samba-ad-dc.init
- debian/samba.smbd.init
On start they used --exec so they would block if "the other" was running
On stop they used --pidfile (so they only stop their own)
I wondered if that would be an issue, but those two init also share the PIDfile path, so it would not really behave differently after the change.

I opened an MP for review and discussions at [1].
In addition there is a xenial PPA to test from [2].

[1]: https://code.launchpad.net/~paelzer/ubuntu/+source/samba/+git/samba/+merge/355532
[2]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3435

Dep8 tests (rather basic) complete and ok.
Waiting for MP review and PPA testing now.

The Xenial upload is in the SRU queue now.
Note: I'd start on Trusty after this completed in Xenial.

Hello Markus, or anyone else affected,

Accepted samba into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.3.11+dfsg-0ubuntu0.16.04.17 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Without any containers it (re-)starts fine:

systemctl restart smbd; sleep 2s; systemctl status smbd
● smbd.service - LSB: start Samba SMB/CIFS daemon (smbd)
   Loaded: loaded (/etc/init.d/smbd; bad; vendor preset: enabled)
   Active: active (running) since Tue 2018-10-09 11:58:52 UTC; 2s ago
     Docs: man:systemd-sysv-generator(8)
  Process: 8925 ExecStop=/etc/init.d/smbd stop (code=exited, status=0/SUCCESS)
  Process: 8932 ExecStart=/etc/init.d/smbd start (code=exited, status=0/SUCCESS)
    Tasks: 3
   Memory: 21.7M
      CPU: 200ms
   CGroup: /system.slice/smbd.service
           ├─8943 /usr/sbin/smbd -D
           ├─8944 /usr/sbin/smbd -D
           └─8947 /usr/sbin/smbd -D

Oct 09 11:58:52 x systemd[1]: Starting LSB: start Samba SMB/CIFS daemon (smbd)...
Oct 09 11:58:52 x smbd[8932]: * Starting SMB/CIFS daemon smbd
Oct 09 11:58:52 x smbd[8932]: ...done.
Oct 09 11:58:52 x systemd[1]: Started LSB: start Samba SMB/CIFS daemon (smbd).

But if I add a container with a smb binary it will fail to restart:

● smbd.service - LSB: start Samba SMB/CIFS daemon (smbd)
   Loaded: loaded (/etc/init.d/smbd; bad; vendor preset: enabled)
   Active: active (exited) since Tue 2018-10-09 12:02:41 UTC; 2s ago
     Docs: man:systemd-sysv-generator(8)
  Process: 12732 ExecStop=/etc/init.d/smbd stop (code=exited, status=0/SUCCESS)
  Process: 12739 ExecStart=/etc/init.d/smbd start (code=exited, status=0/SUCCESS)

Oct 09 12:02:41 x systemd[1]: Starting LSB: start Samba SMB/CIFS daemon (smbd)...
Oct 09 12:02:41 x smbd[12739]: * Starting SMB/CIFS daemon smbd
Oct 09 12:02:41 x smbd[12739]: ...done.
Oct 09 12:02:41 x systemd[1]: Started LSB: start Samba SMB/CIFS daemon (smbd).

This will block (as initially reported) and be non-obvious until one realizes the process from the container is what blocks this.

Installing from proposed

root@x:~# apt install samba
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
  libwbclient0 python-samba samba-common samba-common-bin samba-libs
Suggested packages:
  bind9 bind9utils ctdb ldb-tools smbldap-tools winbind heimdal-clients
Recommended packages:
  samba-dsdb-modules samba-vfs-modules
The following packages will be upgraded:
  libwbclient0 python-samba samba samba-common samba-common-bin samba-libs
6 upgraded, 0 newly installed, 0 to remove and 23 not upgraded.
Need to get 7759 kB of archives.
After this operation, 0 B of additional disk space will be used.
Do you want to continue? [Y/n] Y
Get:1 http://archive.ubuntu.com/ubuntu xenial-proposed/main amd64 python-samba amd64 2:4.3.11+dfsg-0ubuntu0.16.04.17 [1059 kB]
Get:2 http://archive.ubuntu.com/ubuntu xenial-proposed/main amd64 samba-common-bin amd64 2:4.3.11+dfsg-0ubuntu0.16.04.17 [506 kB]
Get:3 http://archive.ubuntu.com/ubuntu xenial-proposed/main amd64 samba-libs amd64 2:4.3.11+dfsg-0ubuntu0.16.04.17 [5172 kB]
Get:4 http://archive.ubuntu.com/ubuntu xenial-proposed/main amd64 libwbclient0 amd64 2:4.3.11+dfsg-0ubuntu0.16.04.17 [30.3 kB]
Get:5 http://archive.ubuntu.com/ubuntu xenial-proposed/main amd64 samba amd64 2:4.3.11+dfsg-0ubuntu0.16.04.17 [908 kB]
Get:6 http:...

I think the testing and reviews performed on this fix are sufficient to let it in without any additional aging. The changes are easily revertible in the worst case.

The verification of the Stable Release Update for samba has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package samba - 2:4.3.11+dfsg-0ubuntu0.16.04.17

---------------
samba (2:4.3.11+dfsg-0ubuntu0.16.04.17) xenial; urgency=medium

  * d/samba.nmbd.init, d/samba.samba-ad-dc.init, d/samba.smbd.init, d/winbind.init
    avoid issues due to init scripts misdetecting services (LP: #1792400)
    - use --pidfile on --start to not block on same binaries running in
      containers
    - use --exec on --stop to not cause unintended processes to be acted on,
      if the old process terminated without being able to remove the pid-file.

 -- Christian Ehrhardt <email address hidden> Mon, 24 Sep 2018 12:08:45 +0200

MP for Trusty added: https://code.launchpad.net/~paelzer/ubuntu/+source/samba/+git/samba/+merge/356793

Autopkgtests for trusty (on bileto ticket) complete, no problems going forward - waiting on MP review.

Actually MP completed since I last refreshed the page, so all complete uploading for SRU Team

Hello Markus, or anyone else affected,

Accepted samba into trusty-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.3.11+dfsg-0ubuntu0.14.04.18 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-trusty to verification-done-trusty. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-trusty. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Tested an upgrade from proposed.
- upgrade worked
- Primary on trusty the upstart jobs are important which are not affected
- If I force execution of the sysV code (hacky) I'm affected and the proposed change from proposed fixes it
- So that isn't an issue for the majority of users, but still an issue since the same code is delivered. Looking back we might have skipped it on Trusty entirely, but it is not "wrong" to fix it
- on the positive side we also have not heard an issue on Xenial which got the same change recently.

Setting verified.

Example showing that the default upstart isn't affected:

ubuntu@T-smb-nested:~$ ps axlf | grep smbd
0 1000 13315 3315 20 0 10504 892 pipe_w S+ pts/0 0:00 | \_ grep --color=auto smbd
0 165536 13311 13204 20 0 7604 576 - S pts/6 0:00 \_ /usr/sbin/smbd 1h
4 0 13291 1 20 0 316760 9108 - Ss ? 0:00 smbd -F
1 0 13300 13291 20 0 308676 2840 - S ? 0:00 \_ smbd -F
1 0 13302 13291 20 0 316760 3080 - S ? 0:00 \_ smbd -F

$ sudo service smbd status; echo ""; sudo service smbd restart; echo ""; sudo service smbd status
smbd start/running, process 13291

smbd stop/waiting
smbd start/running, process 13334

smbd start/running, process 13334

So as I outlined before, not that impacted on Trusty, but for the sake of fixing code that the packages deliver pushing the fix is still correct.

If the SRU Team disagrees we could as well cancel the SRU at this stage and remove it from proposed - sorry for the noise then.

Thank you for noting the upstart interaction here. I agree that the fix is correct, but I'm not sure the SRU is justified then. I'm not keen on recommending users automatically download and install extra stuff when it's almost certain no user needs it. Therefore I think we should cancel this SRU. The Trusty task can be set to Invalid even perhaps, because the issue isn't believed to affect any Trusty users (upstart is the only supported init system on Ubuntu Trusty).

If an update is made to Trusty's samba package for any other reason (security update or a different SRU) then it's fine to bundle this one.

I see no particular reason to remove the package from trusty-proposed. It's tested to be correct and is what we'd want bundled anyway.

Thank you for taking the time to verify this stable release fix. We have noticed that you have used the verification-done tag for marking the bug as verified and would like to point out that due to a recent change in SRU bug verification policy fixes now have to be marked with per-release tags (i.e. verification-done-$RELEASE). Please remove the verification-done tag and add one for the release you have tested the package in. Thank you!

https://wiki.ubuntu.com/StableReleaseUpdates#Verification

This bug was fixed in the package samba - 2:4.3.11+dfsg-0ubuntu0.14.04.19

---------------
samba (2:4.3.11+dfsg-0ubuntu0.14.04.19) trusty-security; urgency=medium

  * SECURITY UPDATE: Unprivileged adding of CNAME record causing loop in AD
    Internal DNS server
    - debian/patches/CVE-2018-14629.patch: add CNAME loop prevention using
      counter in source4/dns_server/dns_query.c.
    - CVE-2018-14629
  * SECURITY UPDATE: Double-free in Samba AD DC KDC with PKINIT
    - debian/patches/CVE-2018-16841.patch: fix segfault on PKINIT with
      mis-matching principal in source4/kdc/db-glue.c.
    - CVE-2018-16841
  * SECURITY UPDATE: NULL pointer de-reference in Samba AD DC LDAP server
    - debian/patches/CVE-2018-16851.patch: check ret before manipulating
      blob in source4/ldap_server/ldap_server.c.
    - CVE-2018-16851

 -- Marc Deslauriers <email address hidden> Fri, 16 Nov 2018 09:50:56 -0500