getaddrinfo() dest. addr. selection Rule 7 bugfix

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

getaddrinfo() dest. addr. selection Rule 7 bugfix

Sridhar Samudrala
The following patch fixes Rule 7(Prefer native transport) in the destination
address selection process as implemented by getaddrinfo().

It uses netlink RTM_GETROUTE and SIOCGIFHWADDR interfaces to get the outgoing
interface type in order to determine if a particular destination address can be
reached via native transport or not. Using RTM_GETROUTE, we can also get the
source address and hence we can avoid the sequence of socket, connect and
getsockname to get this info.

I tried putting these netlink specific helper functions in linux specific
directory(sysdeps/unix/sysv/linux), but i ran into some problems while building
glibc. So i added them to getaddinfo.c. As such, the patch needs some code
re-arrangement, but i think the idea behind the implementation is fine.

Please review and apply with appropriate changes if it looks OK.

Thanks
Sridhar

-------------------------------------------------------------------------------


diff --git a/nscd/gai.c b/nscd/gai.c
index 2e706bd..da2a5c9 100644
--- a/nscd/gai.c
+++ b/nscd/gai.c
@@ -26,6 +26,8 @@ #define __bind bind
 #define __sendto sendto
 #define __strchrnul strchrnul
 #define __getline getline
+#define __recv recv
+#define __ioctl ioctl
 
 #include <getaddrinfo.c>
 
diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 9cebf77..c3aafae 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -60,6 +60,10 @@ #include <bits/libc-lock.h>
 #include <not-cancel.h>
 #include <nscd/nscd-client.h>
 #include <nscd/nscd_proto.h>
+#include <sys/ioctl.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <net/if_arp.h>
 
 #ifdef HAVE_LIBIDN
 extern int __idna_to_ascii_lz (const char *input, char **output, int flags);
@@ -1124,9 +1128,9 @@ struct sort_result
   uint8_t source_addr_len;
   bool got_source_addr;
   uint8_t source_addr_flags;
+  uint8_t native_transport;
 };
 
-
 static int
 get_scope (const struct sockaddr_storage *ss)
 {
@@ -1434,14 +1438,10 @@ rfc3484_sort (const void *p1, const void
   /* Rule 7: Prefer native transport.  */
   if (a1->got_source_addr)
     {
-      if (!(a1->source_addr_flags & in6ai_temporary)
-  && (a2->source_addr_flags & in6ai_temporary))
- return -1;
-      if ((a1->source_addr_flags & in6ai_temporary)
-  && !(a2->source_addr_flags & in6ai_temporary))
- return 1;
-
-      /* XXX Do we need to check anything beside temporary addresses?  */
+ if (a1->native_transport && !a2->native_transport)
+ return -1;
+ if (!a1->native_transport && a2->native_transport)
+ return 1;
     }
 
 
@@ -1850,6 +1850,156 @@ gaiconf_reload (void)
     gaiconf_init ();
 }
 
+int
+if_name2type(char *ifname)
+{
+  struct ifreq ifr;
+  int fd;
+
+  strncpy (ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
+
+  fd = __socket (AF_INET, SOCK_DGRAM, 0);
+  if (fd < 0)
+    return 0;
+
+  if (__ioctl (fd, SIOCGIFHWADDR, &ifr) < 0)
+    {
+      __close (fd);
+      return 0;
+    }
+
+  __close (fd);
+  return ifr.ifr_hwaddr.sa_family;
+}
+
+/* Get the source address and outgoing interface type for a given destination
+ * address.
+ * If netlink support is present, RTM_GET_ROUTE interface is used to get the
+ * source address and outgoing interface type of the matching route.
+ * If not, UDP connect followed by getsockname is used to get the source
+ * address. We cannot get oif_type when using this mechanism.
+ */
+int
+get_src_and_oif_type(int family, struct sockaddr *dst, socklen_t dst_len,
+                     struct sockaddr_storage *src, int *oif_type)
+{
+  struct {
+    struct nlmsghdr  n;
+    struct rtmsg     r;
+    char             buf[1024];
+  } req;
+  struct nlmsghdr *nlmp;
+  struct rtmsg *rtmp;
+  struct rtattr *rtatp;
+  int rtattrlen;
+  struct sockaddr_in6 *sin6p;
+  struct sockaddr_in *sinp;
+  struct in6_addr *in6p;
+  struct in_addr *inp;
+  int rval = -1;
+  char buf[4096];
+  char oif_name[IFNAMSIZ];
+
+  int fd = __socket (PF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
+  if (fd == -1)
+    {
+      fd = __socket (family, SOCK_DGRAM, IPPROTO_IP);
+      socklen_t sl = sizeof (struct sockaddr_storage);
+      if (fd != -1
+          && __connect (fd, dst, dst_len) == 0
+          && __getsockname (fd, (struct sockaddr *)src, &sl) == 0)
+        {
+          __close(fd);
+          *oif_type = ARPHRD_VOID;
+          return 0;
+        }
+      else
+        return -1;
+    }
+
+  memset(&req, 0, sizeof (req));
+  req.n.nlmsg_len = NLMSG_LENGTH (sizeof (struct rtmsg));
+  req.n.nlmsg_flags = NLM_F_REQUEST;
+  req.n.nlmsg_type = RTM_GETROUTE;
+
+  req.r.rtm_family = family;
+  rtatp = (struct rtattr *)(((char *)&req) + NLMSG_ALIGN (req.n.nlmsg_len));
+  rtatp->rta_type = RTA_DST;
+  switch (family)
+    {
+    case AF_INET6:
+      sin6p = (struct sockaddr_in6 *)dst;
+      rtatp->rta_len = RTA_LENGTH (16);
+      memcpy (RTA_DATA (rtatp), &sin6p->sin6_addr, 16);
+      break;
+    case AF_INET:
+      sinp = (struct sockaddr_in *)dst;
+      rtatp->rta_len = RTA_LENGTH (4);
+      memcpy (RTA_DATA (rtatp), &sinp->sin_addr, 4);
+      break;
+    default:
+      goto fail;
+    }
+  req.n.nlmsg_len = NLMSG_ALIGN (req.n.nlmsg_len) + RTA_ALIGN (rtatp->rta_len);
+
+  rval = TEMP_FAILURE_RETRY (__send (fd, &req, req.n.nlmsg_len, 0));
+  if (rval < 0)
+    goto fail;
+
+  rval = TEMP_FAILURE_RETRY (__recv (fd, buf, sizeof(buf), 0));
+  if (rval < 0)
+    goto fail;
+
+  nlmp = (struct nlmsghdr *)buf;
+  if (!NLMSG_OK (nlmp, rval))
+    {
+      rval = -1;
+      goto fail;
+    }
+
+  rtmp = (struct rtmsg *)NLMSG_DATA (nlmp);
+  rtatp = (struct rtattr *)RTM_RTA (rtmp);
+  rtattrlen = RTM_PAYLOAD (nlmp);
+
+  for (; RTA_OK (rtatp, rtattrlen); rtatp = RTA_NEXT (rtatp, rtattrlen))
+    {
+      switch (rtatp->rta_type)
+        {
+        case RTA_OIF:
+          {
+            int oif_index = *(int *)RTA_DATA (rtatp);
+
+            if_indextoname (oif_index, oif_name);
+            *oif_type = if_name2type (oif_name);
+            break;
+          }
+        case RTA_PREFSRC:
+          switch (rtmp->rtm_family)
+            {
+              case AF_INET6:
+                sin6p = (struct sockaddr_in6 *)src;
+                in6p = (struct in6_addr *)RTA_DATA (rtatp);
+                sin6p->sin6_family = AF_INET6;
+                memcpy (&sin6p->sin6_addr, in6p, 16);
+                break;
+              case AF_INET:
+                sinp = (struct sockaddr_in *)src;
+                inp = (struct in_addr *)RTA_DATA (rtatp);
+                sinp->sin_family = AF_INET;
+                sinp->sin_addr.s_addr = inp->s_addr;
+                break;
+            }
+            break;
+        default:
+  break;
+        }
+    }
+  rval = 0;
+
+fail:
+  __close(fd);
+  return rval;
+}
 
 int
 getaddrinfo (const char *name, const char *service,
@@ -2067,19 +2217,13 @@ #endif
   else
     {
       results[i].source_addr_flags = 0;
+      results[i].native_transport = 1;
 
-      /* We overwrite the type with SOCK_DGRAM since we do not
- want connect() to connect to the other side.  If we
- cannot determine the source address remember this
- fact.  */
-      int fd = __socket (q->ai_family, SOCK_DGRAM, IPPROTO_IP);
       socklen_t sl = sizeof (results[i].source_addr);
-      if (fd != -1
-  && __connect (fd, q->ai_addr, q->ai_addrlen) == 0
-  && __getsockname (fd,
-    (struct sockaddr *) &results[i].source_addr,
-    &sl) == 0)
- {
+      int oif_type;
+      if (get_src_and_oif_type(q->ai_family, q->ai_addr, q->ai_addrlen,
+       &results[i].source_addr, &oif_type) == 0)
+        {
   results[i].source_addr_len = sl;
   results[i].got_source_addr = true;
 
@@ -2098,14 +2242,16 @@ #endif
       if (found != NULL)
  results[i].source_addr_flags = found->flags;
     }
+
+   if  (oif_type == ARPHRD_SIT ||
+        oif_type == ARPHRD_TUNNEL ||
+        oif_type == ARPHRD_TUNNEL6)
+   results[i].native_transport = 0;
  }
       else
  /* Just make sure that if we have to process the same
    address again we do not copy any memory.  */
  results[i].source_addr_len = 0;
-
-      if (fd != -1)
- close_not_cancel_no_status (fd);
     }
 
   /* Remember the canonical name.  */


Reply | Threaded
Open this post in threaded view
|

Re: getaddrinfo() dest. addr. selection Rule 7 bugfix

Ulrich Drepper
Sridhar Samudrala wrote:
> The following patch fixes Rule 7(Prefer native transport) in the destination
> address selection process as implemented by getaddrinfo().

I don't like the patch at all.  Yes, your test for native transport is
correct but the cost is too high.

First, we already open a NETLINK_ROUTE socket once, in __check_pf.  The
code should piggy back on that.  This means reworking how __check_pf is
used or even works.  The socket has to be created unconditionally while
the __check_pf body as it exists today should run as before conditional.
 You'll need to separate opening the socket from the __check_pf code.

Second, the if_name2type code uses yet more syscalls.  These are only
needed if we ever reach rule 7.  So, delay this work.  And in that
situation you can use the same socket for both interfaces.

Third, don't introduce new fields in struct sort_result for yes/no
information.  You replace in6ai_temporary, which should have given you
the hint that you should use a flag.  Maybe this is obsoleted by the
second point above.  In any case, if you remove in6ai_temporary support
in getaddrinfo.c you also have to remove it where the flag is generated
(in check_pf.c).

Fourth, your changes violate all kinds of rules of the coding standard.
 Respect how the rest of the code looks like.  Learn the rules.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


signature.asc (259 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: getaddrinfo() dest. addr. selection Rule 7 bugfix

Sridhar Samudrala
On Mon, 2007-03-05 at 17:54 -0800, Ulrich Drepper wrote:

> Sridhar Samudrala wrote:
> > The following patch fixes Rule 7(Prefer native transport) in the destination
> > address selection process as implemented by getaddrinfo().
>
> I don't like the patch at all.  Yes, your test for native transport is
> correct but the cost is too high.
>
> First, we already open a NETLINK_ROUTE socket once, in __check_pf.  The
> code should piggy back on that.  This means reworking how __check_pf is
> used or even works.  The socket has to be created unconditionally while
> the __check_pf body as it exists today should run as before conditional.
>  You'll need to separate opening the socket from the __check_pf code.

Ulrich,

I thought of doing this. Currently __check_pf() is called once for each
getaddrinfo() call before we get the list of destination addresses.
The new RTM_GETROUTE call needs to be done after we get the destination
address list and once for each dest. address.

>
> Second, the if_name2type code uses yet more syscalls.  These are only
> needed if we ever reach rule 7.  So, delay this work.  And in that
> situation you can use the same socket for both interfaces.
Yes. I agree that if_name2type() should be called only if we reach Rule 7.
Not sure what you meant by 'use the same socket for both interfaces'.
Are you suggesting using the same socket for RTM_GETROUTE and
SIOCGHWADDR calls. Can we do a SIOCGHWADDR ioctl on a netlink socket?

>
> Third, don't introduce new fields in struct sort_result for yes/no
> information.  You replace in6ai_temporary, which should have given you
> the hint that you should use a flag.  Maybe this is obsoleted by the
> second point above.  In any case, if you remove in6ai_temporary support
> in getaddrinfo.c you also have to remove it where the flag is generated
> (in check_pf.c).
We should remove in6ai_temporary flag, but we cannot replace it with
a flag to store 'native_transport'. The flags in in6addrinfo are for
the local addresses. The native_transport flag is per destination
address. So i added a field to sort_result. Anyway, We can avoid this
flag if we do if_name2type() only when we hit Rule 7.

>
> Fourth, your changes violate all kinds of rules of the coding standard.
>  Respect how the rest of the code looks like.  Learn the rules.

I am not fully familiar with the glibc coding standards, but i tried to
follow the syntax of the existing code as much as possible.

Also, i ran into some compile issues when i tried to modify multiple
files and each build takes 15 minutes. Is it possible to make the builds
faster when we modify a single file?

I think it will be much easier for you to make these changes to fix this
issue and treat this as a bug report with a suggested solution.

Thanks
Sridhar



Reply | Threaded
Open this post in threaded view
|

Re: getaddrinfo() dest. addr. selection Rule 7 bugfix

Ulrich Drepper
Sridhar Samudrala wrote:
> I thought of doing this. Currently __check_pf() is called once for each
> getaddrinfo() call before we get the list of destination addresses.
> The new RTM_GETROUTE call needs to be done after we get the destination
> address list and once for each dest. address.

And?  Why should this be a problem with reusing the socket?  It's a lame
excuse for not wanting to do the work.


> Are you suggesting using the same socket for RTM_GETROUTE and
> SIOCGHWADDR calls.

No, I did not.  I suggest, though, to use the same socket for all
necessary SIOCGHWADDR calls.


> Can we do a SIOCGHWADDR ioctl on a netlink socket?

I don't know.  I didn't suggest using the ioctl.


> I am not fully familiar with the glibc coding standards, but i tried to
> follow the syntax of the existing code as much as possible.

Then you did an extremely poor job.  Most lines you didn't copy from
somewhere are wrong.


> I think it will be much easier for you to make these changes to fix this
> issue and treat this as a bug report with a suggested solution.

Then say from the beginning that you're not interested in fixing the
problem and have me waste time by explaining them.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


signature.asc (259 bytes) Download Attachment