[LEDE-DEV] [PATCH v4] dnsmasq: manage resolv.conf iff when listening on 127.0.0.1#53
Paul Oranje
por at xs4all.nl
Fri Jun 23 04:21:54 PDT 2017
Please see in-line,
regards,
Paul
> Op 22 jun. 2017, om 17:09 heeft Hans Dedecker <dedeckeh at gmail.com> het volgende geschreven:
>
> On Thu, Jun 22, 2017 at 12:52 PM, Paul Oranje <por at xs4all.nl> wrote:
>> Hello Hans,
>> So far I did not see a patchwork message on the latest submission.
>> Just a direct quick question: did something go wrong here ?
>> Paul
> Hi Paul,
>
> I've been discussing the patch with jow on the IIRC channel yesterday;
> the patch has become highly discutable for different reasons.
> As already mentioned by me he mentioned the use case of DNAT (thus not
> listening on port 53); also in his opinion we should first create a
> resolv service which allows to change the resolv file in an atomic way
> and then adapt the services.
I have considered these issues (port rewrites and atomiticity), but thought it wiser to handle the rewrite use-case in a separate patch.
About supporting the port rewrite use-case:
It seems a not very likely use-case, but to be able to designate a resolver for the local processes (thus is to be refered to in resolv.conf) when it does **not** listen directly on port 127.0.0.1#53 and do that without having to parse the firewall for port redirection rules (, probably an extra UCI dnsmasq option will be needed that just assigns that role and overrules any other selection criteria (listens on 127.0.0.1#53). The ratio is that this will support very specific set-ups that aren't easily anticipated and leaving the decision to the administrator is more prudent in those cases.
In the cases that a non-local resolver is to be referred to from resolv.conf, dnsmasq just has no role. The resolvfile is then likely generated somehow outside of its scope anyway.
About atomicity of resolv.conf handling:
As long as resolv.conf is changed only **after** the resolver service has been started (and is reset before the service stops), the likelihood of some race condition that would cause a local proces try to resolve against a non-yet-listening resolver, will be very slim (when resolver daemon fails to start; code does not check that). Creation of a separate resolv.conf handler/service (extension of netifd as it already handles the WAN ifup/down case) may offer a generic and guaranteed atomic handling, would be nice, but it would surprise me if that were needed urgently.
> In the patch I noticed the cat which I don't really like in the
> _resolv_teardown, a grep looks more appropriate.
No problem, that is easily changed.
> But jow also pointed
> out that $cfg cannot be used as identifier as uci can reorder the
> different sections resulting into a different identifier.
Okay, but ...
When one is having a dnsmasq configuration with multiple instances, one is likely to have named each section specifically; doing so offers an easy work-around for these quite specific cases.
Besides, this obstacle equally affects the daemon's PID handling, because the run PID file is named /var/run/dnsmasq/dnsmasq.$cfg.pid.
> The handling of resolv.conf has been ugly since it has been created;
> but in jows opinion the patch as it's now won't improve the current
> situation
Pity to read that.
The current implementation definitely has some errors that are fixed with the patch (see LEDE FS#785) and with respect to the problems named "highly discutable", the submitted patch does not make those worse. To the contrary, the point of handling of resolv.conf has explicitly been moved in the program flow such (at start last, at stop first) that it avoids possible races in the current code.
> Hans
>>
>> p.s.
>> When and when not to raise the patch version number is not really clear to me.
>>
>>
>>> Op 21 jun. 2017, om 14:42 heeft Paul Oranje <por at xs4all.nl> het volgende geschreven:
>>>
>>> With this patch the dnsmasq init script manages resolv.conf if and only if
>>> when dnsmasq will listen on 127.0.0.1#53 (is main resolver instance).
>>> Also adds ::1 to the resolver file.
>>>
>>> For unbound a likewise patch exists (PR#4454).
>>> Fixes (combined with the unbound PR) FS#785
>>>
>>> Signed-off-by: Paul Oranje <por at xs4all.nl>
>>> ---
>>> The intended invariant is that resolv.conf is managed whenever a resolver
>>> listens on 127.0.0.1#53. Besides dnsmasq, unbound can take that role as well
>>> (but only when dnsmasq is not already listens on 127.0.0.1#53).
>>> When no instance of dnsmasq has been configured to listen on 127.0.0.1#53 then
>>> resolv.conf is not touched by dnsmasq.
>>>
>>> Currently unbound handles resolv.conf also, but leaves it to dnsmasq whenever
>>> that will run, even when no dnsmasq instance will listen on localhost:53. So
>>> for unbound PR#4454 has been submitted to make sure it always manages
>>> resov.conf when it owns localhost:domain.
>>>
>>>
>>> Tests performed:
>>>
>>> - with/without unbound, dhcp linkages none and dnsmasq
>>> - dnsmasq listens on #53, not #53 (dnsmasq takes precedence when also on #53)
>>> - listen on localhost, not localhost
>>> - noresolv false and true
>>> - one/multiple dnsmasq instances (useless combinations are omitted in testing)
>>>
>>> single dnsmasq instance
>>> standard setup
>>> ==> dnsmasq manages resolv.conf
>>>
>>> two dnsmasq instances, each serving another LAN
>>> both dnsmasq on #53
>>> dnsmasq-2 notinterface loopback
>>> ==> dnsmasq-1 manages resolv.conf
>>>
>>> two dnsmasq unstances and unbound (dhcp_link: none, one dnsmasq behind ubound)
>>> both dnsmasq on #53
>>> dnsmasq-2 on #53, notinterface loopback
>>> noresolv true and server 127.0.0.1#1053
>>> unbound on #1053
>>> ==> dnsmasq-1 manages resolv.conf
>>>
>>> two dnsmasq instances and unbound (dhcp_link: dnsmasq)
>>> dnsmasq-1 on #1053, noresolv true
>>> dnsmasq-2 on #2053, noresolv true
>>> unbound on #53
>>> forward LAN1 to 127.0.0.1#1053, forward LAN2 to 127.0.0.1#2053
>>> ==> unbound manages resolv.conf
>>>
>>> on stops resolv.conf is reset to the auto resolvfile.
>>>
>>>
>>> History:
>>> v1 -> v2 corrected synxtax error
>>> increased PKG_RELEASE
>>> v2 reverted with commit 8180bbac7c237a31bd6e06c63e342c72342b7303
>>> v2 -> v3 rewritten and thoroughly tested
>>> v3 -> v4 corrected test on existence of resolvfile (thanks e9hack)
>>>
>>> Paul
>>> ---
>>> package/network/services/dnsmasq/Makefile | 2 +-
>>> .../network/services/dnsmasq/files/dnsmasq.init | 79 +++++++++++++++-------
>>> 2 files changed, 55 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/package/network/services/dnsmasq/Makefile b/package/network/services/dnsmasq/Makefile
>>> index f9ab13aef0..35ac6b2891 100644
>>> --- a/package/network/services/dnsmasq/Makefile
>>> +++ b/package/network/services/dnsmasq/Makefile
>>> @@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk
>>>
>>> PKG_NAME:=dnsmasq
>>> PKG_VERSION:=2.77
>>> -PKG_RELEASE:=3
>>> +PKG_RELEASE:=4
>>>
>>> PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz
>>> PKG_SOURCE_URL:=http://thekelleys.org.uk/dnsmasq/
>>> diff --git a/package/network/services/dnsmasq/files/dnsmasq.init b/package/network/services/dnsmasq/files/dnsmasq.init
>>> index d5177ecb0c..f22d5c3257 100644
>>> --- a/package/network/services/dnsmasq/files/dnsmasq.init
>>> +++ b/package/network/services/dnsmasq/files/dnsmasq.init
>>> @@ -707,9 +707,51 @@ dhcp_relay_add() {
>>> fi
>>> }
>>>
>>> +_resolv_setup()
>>> +{
>>> + local cfg="$1"
>>> + local port notinterfaces
>>> +
>>> + config_get port "$cfg" port "53"
>>> + [ $port = "53" ] || return
>>> +
>>> + config_get notinterfaces "$cfg" notinterface ""
>>> + [ -n "$notinterfaces" ] && list_contains notinterfaces "loopback" && return
>>> +
>>> + # dnsmasq instance is designated to listen on 127.0.0.1#53.
>>> + # rewrite /tmp/resolv.conf
>>> + rm -f /tmp/resolv.conf
>>> + {
>>> + echo "# /tmp/resolv.conf generated by dnsmasq $cfg $( date )"
>>> + [ $ADD_LOCAL_DOMAIN -eq 1 ] && [ -n "$DOMAIN" ] && {
>>> + echo "search $DOMAIN"
>>> + }
>>> + DNS_SERVERS="$DNS_SERVERS 127.0.0.1 ::1"
>>> + for DNS_SERVER in $DNS_SERVERS ; do
>>> + echo "nameserver $DNS_SERVER"
>>> + done
>>> + } > /tmp/resolv.conf
>>> +
>>> + return
>>> +}
>>> +
>>> +_resolv_teardown()
>>> +{
>>> + cfg="$1"
>>> +
>>> + case $( cat /tmp/resolv.conf ) in
>>> + *"generated by dnsmasq $cfg"*)
>>> + # resolv.conf was written by this instance,
>>> + # reset /tmp/resolv.conf to default.
>>> + [ -f /tmp/resolv.conf ] && rm -f /tmp/resolv.conf
>>> + ln -s /tmp/resolv.conf.auto /tmp/resolv.conf
>>> + ;;
>>> + esac
>>> +}
>>> +
>>> dnsmasq_start()
>>> {
>>> - local cfg="$1" disabled resolvfile user_dhcpscript
>>> + local cfg="$1" disabled noresolv resolvfile user_dhcpscript
>>>
>>> config_get_bool disabled "$cfg" disabled 0
>>> [ "$disabled" -gt 0 ] && return 0
>>> @@ -785,7 +827,6 @@ dnsmasq_start()
>>> append_bool "$cfg" nonegcache "--no-negcache"
>>> append_bool "$cfg" strictorder "--strict-order"
>>> append_bool "$cfg" logqueries "--log-queries=extra"
>>> - append_bool "$cfg" noresolv "--no-resolv"
>>> append_bool "$cfg" localise_queries "--localise-queries"
>>> append_bool "$cfg" readethers "--read-ethers"
>>> append_bool "$cfg" dbus "--enable-dbus"
>>> @@ -854,14 +895,15 @@ dnsmasq_start()
>>> config_get_bool cachelocal "$cfg" cachelocal 1
>>>
>>> config_get_bool noresolv "$cfg" noresolv 0
>>> - if [ "$noresolv" != "1" ]; then
>>> + if [ "$noresolv" = "1" ]; then
>>> + xappend "--no-resolv"
>>> + else
>>> config_get resolvfile "$cfg" resolvfile "/tmp/resolv.conf.auto"
>>> + xappend "--resolv-file=$resolvfile"
>>> # So jail doesn't complain if file missing
>>> - [ -n "$resolvfile" -a \! -e "$resolvfile" ] && touch "$resolvfile"
>>> + [ ! -e "$resolvfile" ] && touch "$resolvfile"
>>> fi
>>>
>>> - [ -n "$resolvfile" ] && xappend "--resolv-file=$resolvfile"
>>> -
>>> config_get hostsfile "$cfg" dhcphostsfile
>>> [ -e "$hostsfile" ] && xappend "--dhcp-hostsfile=$hostsfile"
>>>
>>> @@ -973,16 +1015,6 @@ dnsmasq_start()
>>> echo >> $CONFIGFILE_TMP
>>> mv -f $CONFIGFILE_TMP $CONFIGFILE
>>>
>>> - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && {
>>> - rm -f /tmp/resolv.conf
>>> - [ $ADD_LOCAL_DOMAIN -eq 1 ] && [ -n "$DOMAIN" ] && {
>>> - echo "search $DOMAIN" >> /tmp/resolv.conf
>>> - }
>>> - DNS_SERVERS="$DNS_SERVERS 127.0.0.1"
>>> - for DNS_SERVER in $DNS_SERVERS ; do
>>> - echo "nameserver $DNS_SERVER" >> /tmp/resolv.conf
>>> - done
>>> - }
>>>
>>> procd_open_instance $cfg
>>> procd_set_param command $PROG -C $CONFIGFILE -k -x /var/run/dnsmasq/dnsmasq."${cfg}".pid
>>> @@ -1000,21 +1032,18 @@ dnsmasq_start()
>>> procd_add_jail_mount_rw /var/run/dnsmasq/ $leasefile
>>>
>>> procd_close_instance
>>> +
>>> +
>>> + # rewrite /tmp/resolv.conf only for main instance
>>> + _resolv_setup $cfg
>>> }
>>>
>>> dnsmasq_stop()
>>> {
>>> local cfg="$1"
>>>
>>> - config_get resolvfile "$cfg" "resolvfile"
>>> -
>>> - #relink /tmp/resolve.conf only for main instance
>>> - [ "$resolvfile" = "/tmp/resolv.conf.auto" ] && {
>>> - [ -f /tmp/resolv.conf ] && {
>>> - rm -f /tmp/resolv.conf
>>> - ln -s "$resolvfile" /tmp/resolv.conf
>>> - }
>>> - }
>>> + #relink /tmp/resolv.conf only for main instance
>>> + _resolv_teardown $cfg
>>>
>>> rm -f ${BASEDHCPSTAMPFILE}.${cfg}.*.dhcp
>>> }
>>> --
>>> 2.13.1
>>>
>>>
>>> _______________________________________________
>>> Lede-dev mailing list
>>> Lede-dev at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/lede-dev
>>
More information about the Lede-dev
mailing list