unofficial mirror of libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
@ 2019-02-26 15:25 Florian Weimer
  2019-03-07 11:21 ` Florian Weimer
  2019-03-12 17:33 ` Joseph Myers
  0 siblings, 2 replies; 10+ messages in thread
From: Florian Weimer @ 2019-02-26 15:25 UTC (permalink / raw
  To: libc-alpha

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.

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 */

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-02-26 15:25 [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047] Florian Weimer
@ 2019-03-07 11:21 ` Florian Weimer
  2019-03-07 12:08   ` Zack Weinberg
  2019-03-12 17:33 ` Joseph Myers
  1 sibling, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2019-03-07 11:21 UTC (permalink / raw
  To: libc-alpha

* 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-07 11:21 ` Florian Weimer
@ 2019-03-07 12:08   ` Zack Weinberg
  2019-03-07 12:09     ` Zack Weinberg
  0 siblings, 1 reply; 10+ messages in thread
From: Zack Weinberg @ 2019-03-07 12:08 UTC (permalink / raw
  To: Florian Weimer; +Cc: GNU C Library

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-07 12:08   ` Zack Weinberg
@ 2019-03-07 12:09     ` Zack Weinberg
  2019-03-07 12:18       ` Florian Weimer
  0 siblings, 1 reply; 10+ messages in thread
From: Zack Weinberg @ 2019-03-07 12:09 UTC (permalink / raw
  To: Florian Weimer; +Cc: GNU C Library

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-07 12:09     ` Zack Weinberg
@ 2019-03-07 12:18       ` Florian Weimer
  2019-03-11 18:34         ` Zack Weinberg
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2019-03-07 12:18 UTC (permalink / raw
  To: Zack Weinberg; +Cc: GNU C Library

* 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-07 12:18       ` Florian Weimer
@ 2019-03-11 18:34         ` Zack Weinberg
  0 siblings, 0 replies; 10+ messages in thread
From: Zack Weinberg @ 2019-03-11 18:34 UTC (permalink / raw
  To: Florian Weimer; +Cc: GNU C Library

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-02-26 15:25 [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047] Florian Weimer
  2019-03-07 11:21 ` Florian Weimer
@ 2019-03-12 17:33 ` Joseph Myers
  2019-03-12 18:04   ` Florian Weimer
  1 sibling, 1 reply; 10+ messages in thread
From: Joseph Myers @ 2019-03-12 17:33 UTC (permalink / raw
  To: Florian Weimer; +Cc: libc-alpha, samuel.thibault

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-12 17:33 ` Joseph Myers
@ 2019-03-12 18:04   ` Florian Weimer
  2019-03-13 13:52     ` Carlos O'Donell
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Weimer @ 2019-03-12 18:04 UTC (permalink / raw
  To: Joseph Myers; +Cc: libc-alpha, samuel.thibault

* 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;
+}

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-12 18:04   ` Florian Weimer
@ 2019-03-13 13:52     ` Carlos O'Donell
  2019-03-13 15:26       ` Carlos O'Donell
  0 siblings, 1 reply; 10+ messages in thread
From: Carlos O'Donell @ 2019-03-13 13:52 UTC (permalink / raw
  To: Florian Weimer, Joseph Myers; +Cc: libc-alpha, samuel.thibault

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.

> +}
> 


-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047]
  2019-03-13 13:52     ` Carlos O'Donell
@ 2019-03-13 15:26       ` Carlos O'Donell
  0 siblings, 0 replies; 10+ messages in thread
From: Carlos O'Donell @ 2019-03-13 15:26 UTC (permalink / raw
  To: Florian Weimer, Joseph Myers; +Cc: libc-alpha, samuel.thibault

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.

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-03-13 15:27 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-26 15:25 [PATCH] resolv: Enable full ICMP errors for UDP DNS sockets [BZ #24047] Florian Weimer
2019-03-07 11:21 ` Florian Weimer
2019-03-07 12:08   ` Zack Weinberg
2019-03-07 12:09     ` Zack Weinberg
2019-03-07 12:18       ` Florian Weimer
2019-03-11 18:34         ` Zack Weinberg
2019-03-12 17:33 ` Joseph Myers
2019-03-12 18:04   ` Florian Weimer
2019-03-13 13:52     ` Carlos O'Donell
2019-03-13 15:26       ` Carlos O'Donell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).