Bug 236564 - periodic.sh: Anticongestion function does not work as is expected.
Summary: periodic.sh: Anticongestion function does not work as is expected.
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-16 01:12 UTC by Yasuhiro Kimura
Modified: 2019-09-22 00:13 UTC (History)
6 users (show)

See Also:
asomers: mfc-stable12+
asomers: mfc-stable11+


Attachments
Patch file (749 bytes, patch)
2019-03-16 01:12 UTC, Yasuhiro Kimura
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yasuhiro Kimura freebsd_committer freebsd_triage 2019-03-16 01:12:31 UTC
Created attachment 202893 [details]
Patch file

I. Summary

Under some conditions anticongestion function in /etc/default/periodic.conf does not work as is expected

II. Backgroup

Anticongestion function is defined in /etc/default/periodic.conf. If I understand correctly the purpose of it is to insert random sleep just one time through all periodic scripts even if it is called more than once.

III. Problem Description

Currently (at base r345186) /etc/periodic/daily/480.leapfile-ntpd is a only periodic script on base system that calls anticongestion function. So let me assume following situation.

* There is no /etc/weekly.local.
* There is no periodic script under /usr/local/etc/periodic/security.
* There is only one periodic script 123-foo under /usr/local/etc/periodic/weekly.
* 123-foo calls anticongestion function in it.

Then what happens when 'periodic weekly' is executed from cron? Periodic(8) executes scripts with following order.

1. /etc/periodic/weekly/310.locate
2. /etc/periodic/weekly/320.whatis
3. /etc/periodic/weekly/340.noid
4. /etc/periodic/weekly/450.status-security
  4.01. /etc/periodic/security/100.chksetuid
  4.02. /etc/periodic/security/110.neggrpperm
  4.03. /etc/periodic/security/200.chkmounts
  4.04. /etc/periodic/security/300.chkuid0
  4.05. /etc/periodic/security/400.passwdless
  4.06. /etc/periodic/security/410.logincheck
  4.07. /etc/periodic/security/500.ipfwdenied
  4.08. /etc/periodic/security/510.ipfdenied
  4.09. /etc/periodic/security/520.pfdenied
  4.10. /etc/periodic/security/550.ipfwlimit
  4.11. /etc/periodic/security/610.ipf6denied
  4.12. /etc/periodic/security/700.kernelmsg
  4.13. /etc/periodic/security/800.loginfail
  4.14. /etc/periodic/security/900.tcpwrap
5. /etc/periodic/weekly/999.local
6. /usr/local/etc/periodic/weekly/123-foo

As is explained above no script under /etc/periodic/weekly and /etc/periodic/security calls anticongestion function. So expected behavior is that anticongestion function is called and random sleep insterted at step 6. But it is different from real one. What really happens is that anticongestion function is called at step 6 but random sleep is not inserted.

IV. Source of Problem

The source of problem is that periodic(8) is executed twice when 'periodic weekly' is executed from cron. That is, first when it is executed from cron, and next when /etc/periodic/weekly/450.status-security is executed.

There is following codes at line 79-81 of /usr/sbin/periodic.

----------------------------------------------------------------------
if [ -z "$PERIODIC_ANTICONGESTION_FILE" ] ; then
	export PERIODIC_ANTICONGESTION_FILE=`mktemp ${TMPDIR:-/tmp}/periodic.anticongestion.XXXXXXXXXX`
fi
----------------------------------------------------------------------

And there is following code at last line of /usr/sbin/periodic.

----------------------------------------------------------------------
rm -f $PERIODIC_ANTICONGESTION_FILE
----------------------------------------------------------------------

Anticongestion function is defined in /etc/default/periodic.conf as following.

----------------------------------------------------------------------
	# Sleep for a random amount of time in order to mitigate the thundering
	# herd problem of multiple hosts running periodic simultaneously.
	# Will not sleep when used interactively.
	# Will sleep at most once per invocation of periodic
	anticongestion() {
		[ -n "$PERIODIC_IS_INTERACTIVE" ] && return
		if [ -f "$PERIODIC_ANTICONGESTION_FILE" ]; then
			rm -f $PERIODIC_ANTICONGESTION_FILE
			sleep `jot -r 1 0 ${anticongestion_sleeptime}`
		fi
	}
----------------------------------------------------------------------

So what really happens is as following.

a. 'periodic weekly' is executed from cron and master process of periodic(8) is created. 
b. Because PERIODIC_ANTICONGESTION_FILE is not set in master process, temporary file is created, its path is set as value of PERIODIC_ANTICONGESTION_FILE and PERIODIC_ANTICONGESTION_FILE is exported.
c. /etc/periodic/weekly/450.status-security is executed and slave process of periodic(8) is created
d. Because PERIODIC_ANTICONGESTION_FILE is already set in slave process, another temporary file is not created and the value of PERIODIC_ANTICONGESTION_FILE is not changed.
e. At the end of slave process temporary file created at step b is removed.
f. Slave process exits and return to master process.
g. /usr/local/etc/periodic/weekly/123-foo is executed and anticongestion function is called.
h. Because temporary file created at step b is already removed at step e, random sleep is not inserted.

In this way anticongestion function does not work as is expected.

V. Solution

By applying attached patch the problem is fixed.
Comment 1 Warner Losh freebsd_committer freebsd_triage 2019-08-13 15:56:29 UTC
 The patch looks good to my eye, but I have no ability to test it.
Comment 2 Rodney W. Grimes freebsd_committer freebsd_triage 2019-08-13 15:57:09 UTC
This bug is on no lists, could it please be re-triaged and put onto some list in the CC: field.  I would do it but I am not sure what list best applies to this specific bug and leave that to the experts to decide.

Thanks,
Rod
Comment 3 Warner Losh freebsd_committer freebsd_triage 2019-08-13 16:05:28 UTC
rod: you're free to add it to the right list if you know it, but Alan has grabbed the bug, and I have to presume he's clueful enough to involve the proper people.
Comment 4 commit-hook freebsd_committer freebsd_triage 2019-08-18 17:12:31 UTC
A commit references this bug:

Author: asomers
Date: Sun Aug 18 17:12:06 UTC 2019
New revision: 351192
URL: https://svnweb.freebsd.org/changeset/base/351192

Log:
  periodic: fix anticongestion for scripts run after security

  Revision 316342, which introduced the anticongestion feature, failed to
  consider that the periodic scripts are executed by a recursive invocation of
  periodic.  The recursive invocation wrongly cleaned up a temporary file that
  should've been cleaned up only by the original invocation.  The result is
  that if the first script that requests an anticongestion sleep runs after
  the security scripts, the sleep won't happen.

  Fix this bug by delaying cleanup until the end of the original invocation.

  PR:		236564
  Submitted by:	Yasuhiro KIMURA <yasu@utahime.org>
  Reviewed by:	imp
  MFC after:	1 month

Changes:
  head/usr.sbin/periodic/periodic.sh
Comment 5 Alan Somers freebsd_committer freebsd_triage 2019-08-18 17:22:05 UTC
Thanks for spotting this, Yasuhiro.  I'll MFC in a month and then we'll be all done.
Comment 6 Yasuhiro Kimura freebsd_committer freebsd_triage 2019-09-18 17:01:23 UTC
(In reply to Alan Somers from comment #5)

Hello Alan.

Would you please MFC to stable/12 before branching releng/12.1?

Best Regards.
Comment 7 commit-hook freebsd_committer freebsd_triage 2019-09-18 17:19:00 UTC
A commit references this bug:

Author: asomers
Date: Wed Sep 18 17:18:09 UTC 2019
New revision: 352489
URL: https://svnweb.freebsd.org/changeset/base/352489

Log:
  MFC r351192, r351203

  r351192:
  periodic: fix anticongestion for scripts run after security

  Revision 316342, which introduced the anticongestion feature, failed to
  consider that the periodic scripts are executed by a recursive invocation of
  periodic.  The recursive invocation wrongly cleaned up a temporary file that
  should've been cleaned up only by the original invocation.  The result is
  that if the first script that requests an anticongestion sleep runs after
  the security scripts, the sleep won't happen.

  Fix this bug by delaying cleanup until the end of the original invocation.

  PR:		236564
  Submitted by:	Yasuhiro KIMURA <yasu@utahime.org>
  Reviewed by:	imp

  r351203:
  periodic: replace "tty" with "test -t 0"

  Apparently using tty for this purpose has been deprecated since 4.4 Lite.

  Reviewed by:	cy
  Differential Revision:	https://reviews.freebsd.org/D21318

Changes:
_U  stable/12/
  stable/12/usr.sbin/periodic/periodic.sh
Comment 8 commit-hook freebsd_committer freebsd_triage 2019-09-18 17:22:03 UTC
A commit references this bug:

Author: asomers
Date: Wed Sep 18 17:21:34 UTC 2019
New revision: 352490
URL: https://svnweb.freebsd.org/changeset/base/352490

Log:
  MFC r351192, r351203

  r351192:
  periodic: fix anticongestion for scripts run after security

  Revision 316342, which introduced the anticongestion feature, failed to
  consider that the periodic scripts are executed by a recursive invocation of
  periodic.  The recursive invocation wrongly cleaned up a temporary file that
  should've been cleaned up only by the original invocation.  The result is
  that if the first script that requests an anticongestion sleep runs after
  the security scripts, the sleep won't happen.

  Fix this bug by delaying cleanup until the end of the original invocation.

  PR:		236564
  Submitted by:	Yasuhiro KIMURA <yasu@utahime.org>
  Reviewed by:	imp

  r351203:
  periodic: replace "tty" with "test -t 0"

  Apparently using tty for this purpose has been deprecated since 4.4 Lite.

  Reviewed by:	cy
  Differential Revision:	https://reviews.freebsd.org/D21318

Changes:
_U  stable/11/
  stable/11/usr.sbin/periodic/periodic.sh
Comment 9 Alan Somers freebsd_committer freebsd_triage 2019-09-18 17:23:18 UTC
Done!  Thanks for your contribution.
Comment 10 commit-hook freebsd_committer freebsd_triage 2019-09-22 00:13:08 UTC
A commit references this bug:

Author: asomers
Date: Sun Sep 22 00:12:44 UTC 2019
New revision: 352588
URL: https://svnweb.freebsd.org/changeset/base/352588

Log:
  MF stable/12 r352489

  Approved by:	re (kib)

  r351192:
  periodic: fix anticongestion for scripts run after security

  Revision 316342, which introduced the anticongestion feature, failed to
  consider that the periodic scripts are executed by a recursive invocation of
  periodic.  The recursive invocation wrongly cleaned up a temporary file that
  should've been cleaned up only by the original invocation.  The result is
  that if the first script that requests an anticongestion sleep runs after
  the security scripts, the sleep won't happen.

  Fix this bug by delaying cleanup until the end of the original invocation.

  PR:             236564
  Submitted by:   Yasuhiro KIMURA <yasu@utahime.org>
  Reviewed by:    imp

  r351203:
  periodic: replace "tty" with "test -t 0"

  Apparently using tty for this purpose has been deprecated since 4.4 Lite.

  Reviewed by:    cy
  Differential Revision:  https://reviews.freebsd.org/D21318

Changes:
_U  releng/12.1/
  releng/12.1/usr.sbin/periodic/periodic.sh