diff mbox series

resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]

Message ID 87mumi6075.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047] | expand

Commit Message

Florian Weimer Feb. 26, 2019, 3:25 p.m. UTC 
The Linux kernel suppresses some ICMP error messages by default for
UDP sockets.  This commit enables full ICMP error reporting,
hopefully resulting in faster failover to working name servers.

(This change has seen some real-world testing in Fedora 30 and Fedora
rawhide, with no issues reported.)

2019-02-26  Florian Weimer  <fweimer@redhat.com>

	[BZ #24047]
	resolv: Enable full ICMP errors for UDP DNS sockets
	* resolv/res_enable_icmp.c: New file.
	* resolv/Makefile (libresolv-routines): Add res_enable_icmp.
	* resolv/resolv-internal.h (__res_enable_icmp): Declare.
	* resolv/res_send.c (reopen): Call __res_enable_icmp on new
	socket.

Comments

Florian Weimer March 7, 2019, 11:21 a.m. UTC | #1 
* Florian Weimer:

> The Linux kernel suppresses some ICMP error messages by default for
> UDP sockets.  This commit enables full ICMP error reporting,
> hopefully resulting in faster failover to working name servers.
>
> (This change has seen some real-world testing in Fedora 30 and Fedora
> rawhide, with no issues reported.)
>
> 2019-02-26  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #24047]
> 	resolv: Enable full ICMP errors for UDP DNS sockets
> 	* resolv/res_enable_icmp.c: New file.
> 	* resolv/Makefile (libresolv-routines): Add res_enable_icmp.
> 	* resolv/resolv-internal.h (__res_enable_icmp): Declare.
> 	* resolv/res_send.c (reopen): Call __res_enable_icmp on new
> 	socket.

I plan to commit this soon.

Thanks,
Florian
Zack Weinberg March 7, 2019, 12:08 p.m. UTC | #2 
On Thu, Mar 7, 2019 at 6:21 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Florian Weimer:
>
> > The Linux kernel suppresses some ICMP error messages by default for
> > UDP sockets.  This commit enables full ICMP error reporting,
> > hopefully resulting in faster failover to working name servers.
...

The documentation makes it sound like IP_RECVERR doesn't do anything
by itself, you need to call recvmsg(MSG_ERRQUEUE) to actually see the
errors.  As far as I can tell, res_send.c doesn't do that, either
before or after this patch.  Is the documentation wrong?

zw
Zack Weinberg March 7, 2019, 12:09 p.m. UTC | #3 
On Thu, Mar 7, 2019 at 7:08 AM Zack Weinberg <zackw@panix.com> wrote:
>
> On Thu, Mar 7, 2019 at 6:21 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Florian Weimer:
> >
> > > The Linux kernel suppresses some ICMP error messages by default for
> > > UDP sockets.  This commit enables full ICMP error reporting,
> > > hopefully resulting in faster failover to working name servers.
> ...
>
> The documentation makes it sound like IP_RECVERR doesn't do anything
> by itself, you need to call recvmsg(MSG_ERRQUEUE) to actually see the
> errors.  As far as I can tell, res_send.c doesn't do that, either
> before or after this patch.  Is the documentation wrong?

er, by "the documentation" I am referring to the ip(7) manpage.

zw
Florian Weimer March 7, 2019, 12:18 p.m. UTC | #4 
* Zack Weinberg:

> On Thu, Mar 7, 2019 at 7:08 AM Zack Weinberg <zackw@panix.com> wrote:
>>
>> On Thu, Mar 7, 2019 at 6:21 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >
>> > * Florian Weimer:
>> >
>> > > The Linux kernel suppresses some ICMP error messages by default for
>> > > UDP sockets.  This commit enables full ICMP error reporting,
>> > > hopefully resulting in faster failover to working name servers.
>> ...
>>
>> The documentation makes it sound like IP_RECVERR doesn't do anything
>> by itself, you need to call recvmsg(MSG_ERRQUEUE) to actually see the
>> errors.  As far as I can tell, res_send.c doesn't do that, either
>> before or after this patch.  Is the documentation wrong?
>
> er, by "the documentation" I am referring to the ip(7) manpage.

Yes, the documentation is misleading.

I believe the kernel code is in net/ipv4/raw.c in the raw_err function,
which handles ICMP errors for UDP as well.  The harderr handling is
bypassed if the socket has IP_RECVERR set.

Thanks,
Florian
Zack Weinberg March 11, 2019, 6:34 p.m. UTC | #5 
On Thu, Mar 7, 2019 at 7:18 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Zack Weinberg:
>
> > On Thu, Mar 7, 2019 at 7:08 AM Zack Weinberg <zackw@panix.com> wrote:
> >>
> >> On Thu, Mar 7, 2019 at 6:21 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> >
> >> > * Florian Weimer:
> >> >
> >> > > The Linux kernel suppresses some ICMP error messages by default for
> >> > > UDP sockets.  This commit enables full ICMP error reporting,
> >> > > hopefully resulting in faster failover to working name servers.
> >> ...
> >>
> >> The documentation makes it sound like IP_RECVERR doesn't do anything
> >> by itself, you need to call recvmsg(MSG_ERRQUEUE) to actually see the
> >> errors.  As far as I can tell, res_send.c doesn't do that, either
> >> before or after this patch.  Is the documentation wrong?
> >
> > er, by "the documentation" I am referring to the ip(7) manpage.
>
> Yes, the documentation is misleading.
>
> I believe the kernel code is in net/ipv4/raw.c in the raw_err function,
> which handles ICMP errors for UDP as well.  The harderr handling is
> bypassed if the socket has IP_RECVERR set.

I've now looked at this code for myself and I think you're right.  No
objection to the patch from me, then.

zw
Joseph Myers March 12, 2019, 5:33 p.m. UTC | #6 
This has broken the build for Hurd:

res_enable_icmp.c: In function '__res_enable_icmp':
res_enable_icmp.c:30:38: error: 'IP_RECVERR' undeclared (first use in this function); did you mean 'IPV6_RECVERR'?

No idea why the generic bits/in.h as used for Hurd defines IPV6_RECVERR 
but not IP_RECVERR, or what Hurd actually implements in this regard.
Florian Weimer March 12, 2019, 6:04 p.m. UTC | #7 
* Joseph Myers:

> This has broken the build for Hurd:
>
> res_enable_icmp.c: In function '__res_enable_icmp':
> res_enable_icmp.c:30:38: error: 'IP_RECVERR' undeclared (first use in this function); did you mean 'IPV6_RECVERR'?
>
> No idea why the generic bits/in.h as used for Hurd defines IPV6_RECVERR 
> but not IP_RECVERR, or what Hurd actually implements in this regard.

Sorry about that.

Thanks for pointing out the IPV6_RECVERR definition, I would have missed
that.

XNU does not support IP_RECVERR either, so I think the safest way to
deal with this is the attached patch.

Alternatively, I could add the stub to the generic code, but I think
doing it this way is clearer for those who read just the resolv code.

Thanks,
Florian

hurd: Add no-op version of __res_enable_icmp [BZ #24047]

Mach does not support IP_RECVERR, so replace this function with a
stub in a sysdeps override for Hurd.

This fixes commit 08504de71813ddbd447bfbca4a325cbe8ce8bcda
("resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]").

2019-03-12  Florian Weimer  <fweimer@redhat.com>

	[BZ #24047]
	* sysdeps/mach/hurd/res_enable_icmp.c: New file.

diff --git a/sysdeps/mach/hurd/res_enable_icmp.c b/sysdeps/mach/hurd/res_enable_icmp.c
new file mode 100644
index 0000000000..4b0456340c
--- /dev/null
+++ b/sysdeps/mach/hurd/res_enable_icmp.c
@@ -0,0 +1,27 @@
+/* Enable full ICMP errors on a socket.  No-op version for Hurd.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Mach does not support the IP_RECVERR extension.  */
+
+#include <resolv.h>
+
+int
+__res_enable_icmp (int family, int fd)
+{
+  return 0;
+}
Carlos O'Donell March 13, 2019, 1:52 p.m. UTC | #8 
On 3/12/19 2:04 PM, Florian Weimer wrote:
> * Joseph Myers:
> 
>> This has broken the build for Hurd:
>>
>> res_enable_icmp.c: In function '__res_enable_icmp':
>> res_enable_icmp.c:30:38: error: 'IP_RECVERR' undeclared (first use in this function); did you mean 'IPV6_RECVERR'?
>>
>> No idea why the generic bits/in.h as used for Hurd defines IPV6_RECVERR 
>> but not IP_RECVERR, or what Hurd actually implements in this regard.
> 
> Sorry about that.
> 
> Thanks for pointing out the IPV6_RECVERR definition, I would have missed
> that.
> 
> XNU does not support IP_RECVERR either, so I think the safest way to
> deal with this is the attached patch.

Agreed.

Integrating the Linux defines and moving them down into the generic
header would be quite a bit of work and validation, probably for not
much gain at this point i.e. defines from sysdeps/unix/sysv/linux/bits/in.h
need to move to bits/in.h to provide IPv4/IPv6 support in one header
(as Joseph points out I also don't know what it was this way today).

> Alternatively, I could add the stub to the generic code, but I think
> doing it this way is clearer for those who read just the resolv code.

Yes, that would be messier than the hurd implementation you're proposing
now.

> hurd: Add no-op version of __res_enable_icmp [BZ #24047]
> 
> Mach does not support IP_RECVERR, so replace this function with a
> stub in a sysdeps override for Hurd.
> 
> This fixes commit 08504de71813ddbd447bfbca4a325cbe8ce8bcda
> ("resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]").
> 
> 2019-03-12  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24047]
> 	* sysdeps/mach/hurd/res_enable_icmp.c: New file.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> diff --git a/sysdeps/mach/hurd/res_enable_icmp.c b/sysdeps/mach/hurd/res_enable_icmp.c
> new file mode 100644
> index 0000000000..4b0456340c
> --- /dev/null
> +++ b/sysdeps/mach/hurd/res_enable_icmp.c
> @@ -0,0 +1,27 @@
> +/* Enable full ICMP errors on a socket.  No-op version for Hurd.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Mach does not support the IP_RECVERR extension.  */
> +
> +#include <resolv.h>
> +
> +int
> +__res_enable_icmp (int family, int fd)
> +{
> +  return 0;

OK.

> +}
>
Carlos O'Donell March 13, 2019, 3:26 p.m. UTC | #9 
On 3/13/19 9:52 AM, Carlos O'Donell wrote:
> On 3/12/19 2:04 PM, Florian Weimer wrote:
>> This fixes commit 08504de71813ddbd447bfbca4a325cbe8ce8bcda
>> ("resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]").
>>
>> 2019-03-12  Florian Weimer  <fweimer@redhat.com>
>>
>> 	[BZ #24047]
>> 	* sysdeps/mach/hurd/res_enable_icmp.c: New file.
> 
> OK for master.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

My bmg run for i686-gnu is fixed by this commit. Thanks.
diff mbox series

Patch

diff --git a/resolv/Makefile b/resolv/Makefile
index 8f22e6a154..ebe1b733f2 100644
--- a/resolv/Makefile
+++ b/resolv/Makefile
@@ -105,7 +105,7 @@  libresolv-routines := res_comp res_debug \
 		      res_data res_mkquery res_query res_send		\
 		      inet_net_ntop inet_net_pton inet_neta base64	\
 		      ns_parse ns_name ns_netint ns_ttl ns_print	\
-		      ns_samedomain ns_date \
+		      ns_samedomain ns_date res_enable_icmp \
 		      compat-hooks compat-gethnamaddr
 
 libanl-routines := gai_cancel gai_error gai_misc gai_notify gai_suspend \
diff --git a/resolv/res_enable_icmp.c b/resolv/res_enable_icmp.c
new file mode 100644
index 0000000000..bdc9220f08
--- /dev/null
+++ b/resolv/res_enable_icmp.c
@@ -0,0 +1,37 @@ 
+/* Enable full ICMP errors on a socket.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+
+int
+__res_enable_icmp (int family, int fd)
+{
+  int one = 1;
+  switch (family)
+    {
+    case AF_INET:
+      return setsockopt (fd, SOL_IP, IP_RECVERR, &one, sizeof (one));
+    case AF_INET6:
+      return setsockopt (fd, SOL_IPV6, IPV6_RECVERR, &one, sizeof (one));
+    default:
+      __set_errno (EAFNOSUPPORT);
+      return -1;
+    }
+}
diff --git a/resolv/res_send.c b/resolv/res_send.c
index fa040c1198..0f6ec83a7b 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -943,6 +943,18 @@  reopen (res_state statp, int *terrno, int ns)
 			return (-1);
 		}
 
+		/* Enable full ICMP error reporting for this
+		   socket.  */
+		if (__res_enable_icmp (nsap->sa_family,
+				       EXT (statp).nssocks[ns]) < 0)
+		  {
+		    int saved_errno = errno;
+		    __res_iclose (statp, false);
+		    __set_errno (saved_errno);
+		    *terrno = saved_errno;
+		    return -1;
+		  }
+
 		/*
 		 * On a 4.3BSD+ machine (client and server,
 		 * actually), sending to a nameserver datagram
diff --git a/resolv/resolv-internal.h b/resolv/resolv-internal.h
index 6ab8f2af09..1500adc607 100644
--- a/resolv/resolv-internal.h
+++ b/resolv/resolv-internal.h
@@ -100,4 +100,10 @@  libc_hidden_proto (__inet_pton_length)
 /* Called as part of the thread shutdown sequence.  */
 void __res_thread_freeres (void) attribute_hidden;
 
+/* The Linux kernel does not enable all ICMP messages on a UDP socket
+   by default.  A call this function enables full error reporting for
+   the socket FD.  FAMILY must be AF_INET or AF_INET6.  Returns 0 on
+   success, -1 on failure.  */
+int __res_enable_icmp (int family, int fd) attribute_hidden;
+
 #endif  /* _RESOLV_INTERNAL_H */