Page MenuHomeFreeBSD

fix mountd race where a sighup during get_exportlist() does not cause another exports reload
ClosedPublic

Authored by rmacklem on Jun 4 2020, 1:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 19, 10:18 PM
Unknown Object (File)
Mon, Feb 17, 12:16 PM
Unknown Object (File)
Sun, Feb 16, 12:13 PM
Unknown Object (File)
Sat, Jan 25, 7:25 PM
Unknown Object (File)
Sat, Jan 25, 7:21 PM
Unknown Object (File)
Sat, Jan 25, 7:13 PM
Unknown Object (File)
Fri, Jan 24, 6:57 PM
Unknown Object (File)
Wed, Jan 22, 1:27 PM
Subscribers

Details

Summary

Without this patch, if a SIGHUP is handled while the process is executing
get_exportlist(), that SIGHUP is essentially ignored because got_sighup
is reset to 0 after get_exportlist().
This results in the exports file(s) not being reloaded until another SIGHUP
signal is sent to mountd.
Reported by PR#246597.

This patch fixes this by resetting got_sighup to zero before the get_exportlist()
call while SIGHUP is blocked.
It also defines a delay time of 250msec before doing another exports reload
if there are RPC request(s) to process. This prevents repeated exports reloads
from delaying handling of RPC requests significantly.

Test Plan

Tested using an assortment of exports files being reloaded.
Also, the reporter of PR#246597 tested the patch and confirmed
that it fixed the problem for him.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

The calculation of curtime and nexttime were changed slightly
to ensure they are done in 64bits, since time_t is 32bits on
some arches.

usr.sbin/mountd/mountd.c
684 ↗(On Diff #72663)

The results of multiple gettimeofday() calls are not guaranteed to be monotonic. If ntpd steps the clock after the second gettimeofday() call, nexttime could end up being multiple seconds in the future. clock_gettime(CLOCK_MONOTONIC) should be used instead.

711 ↗(On Diff #72663)

I think the problem could also be solved by using kevent(). EVFILT_SIGNAL allows you to watch for delivery of blocked signals, so you can block SIGHUP and handle pending reloads entirely within the event loop. This would eliminate the need to wake up periodically even when the service is idle.

The wrinkle is that the RPC code really wants you to use select(); to use kevent() to monitor the descriptors, you'd need to convert svc_fdset to an array of events, and then use the triggered events to build an fdset to pass to svc_getreqset(). I'm not sure if the awkwardness of doing this outweighs the benefit of not having idle wakeups.

Switch to clock_gettime(CLOCK_MONOTONIC,..) as suggested by markj.

rmacklem added inline comments.
usr.sbin/mountd/mountd.c
684 ↗(On Diff #72663)

I had thought of the clock being set and was going to add
a hack to handle that, but I didn't know about CLOCK_MONOTONIC
and that is a better solution.

711 ↗(On Diff #72663)

I'd rather not mess with how the RPC library requests are
handled, I think I'll stick with the idle wakeups.
(The chunk of code is based on what is in the RPC library.)

markj added inline comments.
usr.sbin/mountd/mountd.c
711 ↗(On Diff #72663)

Fair enough.

This revision is now accepted and ready to land.Jun 5 2020, 1:54 AM