Bug 254006

Summary: net/samba(all versions): rc.d/samba_server stop/restart patch for restart crash
Product: Ports & Packages Reporter: Peter Eriksson <pen>
Component: Individual Port(s)Assignee: Timur I. Bakeyev <timur>
Status: New ---    
Severity: Affects Some People    
Priority: ---    
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Patch to fix samba_server rc.d script to wait for all processes to die
none
A smalls script to stress-test the samba restart-loop none

Description Peter Eriksson 2021-03-04 08:50:13 UTC 
Created attachment 222968 [details]
Patch to fix samba_server rc.d script to wait for all processes to die

The rc.d/samba_server script when stopping and/or restarting samba doesn't wait for _all_ smbd processes to terminate before continuing - it only wait´s for the master process. This can cause Samba (smbd) processes to core dump if a new master smbd process gets started before all the old processes has had time to terminate.

With a non-busy server this rarely get noticed since it happens during samba shutdown and only on busy servers - and for you to get the core dumps you need to have enabled sugid-coredumps...

/etc/sysctl.conf:
  kern.corefile = /var/cores/%N.%P.core
  kern.sugid_coredump = 1

This is related to this Samba issue:
  https://bugzilla.samba.org/show_bug.cgi?id=14636

The solution is to modify the rc.d/samba_server script so it waits for all smbd processes to terminate, not just the master process, before continuing.

Please find enclosed a patch that implements this wait-loop.
Comment 1 Peter Eriksson 2021-03-04 08:53:11 UTC 
Comment on attachment 222968 [details]
Patch to fix samba_server rc.d script to wait for all processes to die

>--- samba_server.orig	2021-03-04 09:38:24.177530000 +0100
>+++ samba_server	2021-03-04 09:37:48.721550000 +0100
>@@ -126,11 +126,27 @@
> 	rcvar=${name}_enable
> 	command="/usr/local/sbin/${name}"
> 	pidfile="${samba_server_piddir}/${name}.pid"
>+	masterpid=""
>+	if [ "${rc_arg}" = "stop" ] ; then
>+	    masterpid="`cat ${pidfile}`"
>+	fi
>+	
> 	# Daemon should be enabled and running
> 	if ( [ -n "${rcvar}" ] && checkyesno "${rcvar}" ) || [ -n "$force_run" ]; then
> 	    run_rc_command "${_rc_prefix}${rc_arg}" ${rc_extra_args}
> 	    # If any of the commands failed, take it as a global result
> 	    result=$((${result} || $?))
>+
>+	    if [ "${rc_arg}" = "stop" -a "$masterpid" != "" ] ; then
>+		if pgrep -q -s "$masterpid"; then
>+		    echo -n "Waiting for all ${name} subprocesses to terminate..."
>+		    while pgrep -q -s "$masterpid"; do
>+			echo -n "."
>+			sleep 1
>+		    done
>+		    echo ""
>+		fi
>+	    fi
> 	fi
>     done
>     return ${result}
Comment 2 Peter Eriksson 2021-03-04 08:54:18 UTC 
(Skip that last comment, shouldn't be there :-)
Comment 3 Li-Wen Hsu freebsd_committer freebsd_triage 2021-03-06 11:47:02 UTC 
Over to maintainer.
Comment 4 Peter Eriksson 2021-03-06 12:17:26 UTC 
Just a quick comment regarding the samba bugid. The thing that was fixed for the coming Samba releases is not the same problem the patch to the FreeBSD samba_server rc.d script sovles. But the restart-too-quickly-problem was found while traceing down the other bug...

The problem is that there are shared locks that the terminating smbd's are trying to clean up - and then the new master smbd comes along and reinitializes the lock again - under the feet of them. This can happen if you are using dbwrap_tdb_mutexes:* = yes (then you typically get a core dump in pthread_mutex_unlock(), but it can also happen even if not doing that.

The only stable solution found (so far) is to wait for all smbd processes to terminate before starting up a new one again. Probably the same with winbindd (but there you typically doesn't have that many concurrent processes so they terminate more quickly).
Comment 5 Peter Eriksson 2021-03-06 12:48:58 UTC 
Created attachment 223022 [details]
A smalls script to stress-test the samba restart-loop

Added a small stress-testing script for the restart-loop.