[PATCH] Better fix for ifaddrs

[PATCH] Better fix for ifaddrs

[Jakub Jelinek]

Hi!

David Miller said kernel guarantees that netlink requests are split into at
most page sized chunks.
This patch removes again all the MSG_TRUNC handling and instead uses page
sized buffer.  I believe a page should be ok with alloca/VLA always, so I
have removed even the use_malloc etc. stuff, if you think I should use
__libc_use_alloca even for this, please let me know.
Tested on ia64 with 80 interfaces.

2007-03-14  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_request): Revert
	2005-06-13, 2007-02-27 and 2007-03-02 changes.  Use page sized buffer.
	* sysdeps/unix/sysv/linux/check_pf.c (make_request): Use page sized
	buffer.

--- libc/sysdeps/unix/sysv/linux/check_pf.c.jj	2007-01-03 11:04:37.000000000 +0100
+++ libc/sysdeps/unix/sysv/linux/check_pf.c	2007-03-13 18:16:12.000000000 +0100
@@ -1,5 +1,5 @@
 /* Determine protocol families for which interfaces exist.  Linux version.
-   Copyright (C) 2003, 2006 Free Software Foundation, Inc.
+   Copyright (C) 2003, 2006, 2007 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
@@ -80,8 +80,9 @@ make_request (int fd, pid_t pid, bool *s
   *seen_ipv6 = false;
 
   bool done = false;
-  char buf[4096];
-  struct iovec iov = { buf, sizeof (buf) };
+  size_t buf_size = __getpagesize ();
+  char buf[buf_size];
+  struct iovec iov = { buf, buf_size };
   struct in6ailist
   {
     struct in6addrinfo info;
--- libc/sysdeps/unix/sysv/linux/ifaddrs.c.jj	2007-03-13 17:42:17.000000000 +0100
+++ libc/sysdeps/unix/sysv/linux/ifaddrs.c	2007-03-13 18:17:06.000000000 +0100
@@ -122,37 +122,17 @@ int
 __netlink_request (struct netlink_handle *h, int type)
 {
   struct netlink_res *nlm_next;
-  struct netlink_res **new_nlm_list;
-  static volatile size_t buf_size = 4096;
-  char *buf;
+  size_t buf_size = __getpagesize ();
+  char buf[buf_size];
   struct sockaddr_nl nladdr;
   struct nlmsghdr *nlmh;
   ssize_t read_len;
   bool done = false;
-  bool use_malloc = false;
 
   if (__netlink_sendreq (h, type) < 0)
     return -1;
 
-  size_t this_buf_size = buf_size;
-  size_t orig_this_buf_size = this_buf_size;
-  if (__libc_use_alloca (this_buf_size))
-    buf = alloca (this_buf_size);
-  else
-    {
-      buf = malloc (this_buf_size);
-      if (buf != NULL)
-	use_malloc = true;
-      else
-	goto out_fail;
-    }
-
-  struct iovec iov = { buf, this_buf_size };
-
-  if (h->nlm_list != NULL)
-    new_nlm_list = &h->end_ptr->next;
-  else
-    new_nlm_list = &h->nlm_list;
+  struct iovec iov = { buf, buf_size };
 
   while (! done)
     {
@@ -166,54 +146,13 @@ __netlink_request (struct netlink_handle
 
       read_len = TEMP_FAILURE_RETRY (__recvmsg (h->fd, &msg, 0));
       if (read_len < 0)
-	goto out_fail;
+	return -1;
 
       if (nladdr.nl_pid != 0)
 	continue;
 
       if (__builtin_expect (msg.msg_flags & MSG_TRUNC, 0))
-	{
-	  if (this_buf_size >= SIZE_MAX / 2)
-	    goto out_fail;
-
-	  nlm_next = *new_nlm_list;
-	  while (nlm_next != NULL)
-	    {
-	      struct netlink_res *tmpptr;
-
-	      tmpptr = nlm_next->next;
-	      free (nlm_next);
-	      nlm_next = tmpptr;
-	    }
-	  *new_nlm_list = NULL;
-
-	  if (__libc_use_alloca (2 * this_buf_size))
-	    buf = extend_alloca (buf, this_buf_size, 2 * this_buf_size);
-	  else
-	    {
-	      this_buf_size *= 2;
-
-	      char *new_buf = realloc (use_malloc ? buf : NULL, this_buf_size);
-	      if (new_buf == NULL)
-		goto out_fail;
-	      buf = new_buf;
-
-	      use_malloc = true;
-	    }
-	  buf_size = this_buf_size;
-
-	  iov.iov_base = buf;
-	  iov.iov_len = this_buf_size;
-
-	  /* Increase sequence number, so that we can distinguish
-	     between old and new request messages.  */
-	  h->seq++;
-
-	  if (__netlink_sendreq (h, type) < 0)
-	    goto out_fail;
-
-	  continue;
-	}
+	return -1;
 
       size_t count = 0;
       size_t remaining_len = read_len;
@@ -237,39 +176,9 @@ __netlink_request (struct netlink_handle
 	      struct nlmsgerr *nlerr = (struct nlmsgerr *) NLMSG_DATA (nlmh);
 	      if (nlmh->nlmsg_len < NLMSG_LENGTH (sizeof (struct nlmsgerr)))
 		errno = EIO;
-	      else if (nlerr->error == -EBUSY
-		       && orig_this_buf_size != this_buf_size)
-		{
-		  /* If EBUSY and MSG_TRUNC was seen, try again with a new
-		     netlink socket.  */
-		  struct netlink_handle hold = *h;
-		  if (__netlink_open (h) < 0)
-		    {
-		      *h = hold;
-		      goto out_fail;
-		    }
-		  __netlink_close (&hold);
-		  orig_this_buf_size = this_buf_size;
-		  nlm_next = *new_nlm_list;
-		  while (nlm_next != NULL)
-		    {
-		      struct netlink_res *tmpptr;
-
-		      tmpptr = nlm_next->next;
-		      free (nlm_next);
-		      nlm_next = tmpptr;
-		    }
-		  *new_nlm_list = NULL;
-		  count = 0;
-		  h->seq++;
-
-		  if (__netlink_sendreq (h, type) < 0)
-		    goto out_fail;
-		  break;
-		}
 	      else
 		errno = -nlerr->error;
-	      goto out_fail;
+	      return -1;
 	    }
 	}
 
@@ -281,7 +190,7 @@ __netlink_request (struct netlink_handle
       nlm_next = (struct netlink_res *) malloc (sizeof (struct netlink_res)
 						+ read_len);
       if (nlm_next == NULL)
-	goto out_fail;
+	return -1;
       nlm_next->next = NULL;
       nlm_next->nlh = memcpy (nlm_next + 1, buf, read_len);
       nlm_next->size = read_len;
@@ -293,14 +202,7 @@ __netlink_request (struct netlink_handle
       h->end_ptr = nlm_next;
     }
 
-  if (use_malloc)
-    free (buf);
   return 0;
-
-out_fail:
-  if (use_malloc)
-    free (buf);
-  return -1;
 }
 
 

	Jakub

Re: [PATCH] Better fix for ifaddrs

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

Jakub Jelinek wrote:
> if you think I should use
> __libc_use_alloca even for this, please let me know.

I fear for archs like ia64 and ppc64 where the page size can widely vary
we need this.  But I think that this code should be optional, i.e., only
for those archs.  No need to slow down the rest of the world.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

Re: [PATCH] Better fix for ifaddrs

[Jakub Jelinek]

On Wed, Mar 14, 2007 at 05:45:15PM -0700, Ulrich Drepper wrote:
> Jakub Jelinek wrote:
> > if you think I should use
> > __libc_use_alloca even for this, please let me know.
> 
> I fear for archs like ia64 and ppc64 where the page size can widely vary
> we need this.  But I think that this code should be optional, i.e., only
> for those archs.  No need to slow down the rest of the world.

Ok, how about this then?

On x86_64/i386 where PAGE_SIZE is defined, constant and <= PTHREAD_STACK_MIN/4 
the malloc+free are completely optimized out by the compiler, the only bad
thing there is that it will still use char *buf = alloca (4096); rather than
char buf[4096]; which is a few instructions more expensive.  But even that
is fixable in the compiler (if alloca with a constant argument is always
reachable from the entry BB, then the compiler can just create a char array
with that size instead of alloca).  Working around this in glibc would be
ugly, hardcoding what __libc_use_alloca does internally e.g. in preprocessor
directive would be bad.

2007-03-15  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/unix/sysv/linux/ifaddrs.c (__netlink_request): Never reallocate
	the buffer, instead fail for MSG_TRUNC or for EBUSY NLMSG_ERR.  Instead
	use a page sized buffer.
	* sysdeps/unix/sysv/linux/check_pf.c (make_request): Use page sized
	buffer.

--- libc/sysdeps/unix/sysv/linux/check_pf.c.jj	2007-01-03 11:04:37.000000000 +0100
+++ libc/sysdeps/unix/sysv/linux/check_pf.c	2007-03-15 09:20:11.000000000 +0100
@@ -1,5 +1,5 @@
 /* Determine protocol families for which interfaces exist.  Linux version.
-   Copyright (C) 2003, 2006 Free Software Foundation, Inc.
+   Copyright (C) 2003, 2006, 2007 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
@@ -71,17 +71,38 @@ make_request (int fd, pid_t pid, bool *s
   memset (&nladdr, '\0', sizeof (nladdr));
   nladdr.nl_family = AF_NETLINK;
 
+#ifdef PAGE_SIZE
+  /* Help the compiler optimize out the malloc call if PAGE_SIZE
+     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
+  const size_t buf_size = PAGE_SIZE;
+#else
+  const size_t buf_size = __getpagesize ();
+#endif
+  bool use_malloc = false;
+  char *buf;
+
+  if (__libc_use_alloca (buf_size))
+    buf = alloca (buf_size);
+  else
+    {
+      buf = malloc (buf_size);
+      if (buf != NULL)
+	use_malloc = true;
+      else
+	goto out_fail;
+    }
+
+  struct iovec iov = { buf, buf_size };
+
   if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
 				    (struct sockaddr *) &nladdr,
 				    sizeof (nladdr))) < 0)
-    return -1;
+    goto out_fail;
 
   *seen_ipv4 = false;
   *seen_ipv6 = false;
 
   bool done = false;
-  char buf[4096];
-  struct iovec iov = { buf, sizeof (buf) };
   struct in6ailist
   {
     struct in6addrinfo info;
@@ -101,10 +122,10 @@ make_request (int fd, pid_t pid, bool *s
 
       ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
       if (read_len < 0)
-	return -1;
+	goto out_fail;
 
       if (msg.msg_flags & MSG_TRUNC)
-	return -1;
+	goto out_fail;
 
       struct nlmsghdr *nlmh;
       for (nlmh = (struct nlmsghdr *) buf;
@@ -186,7 +207,7 @@ make_request (int fd, pid_t pid, bool *s
     {
       *in6ai = malloc (in6ailistlen * sizeof (**in6ai));
       if (*in6ai == NULL)
-	return -1;
+	goto out_fail;
 
       *in6ailen = in6ailistlen;
 
@@ -198,6 +219,13 @@ make_request (int fd, pid_t pid, bool *s
       while (in6ailist != NULL);
     }
 
+  if (use_malloc)
+    free (buf);
+  return 0;
+
+out_fail:
+  if (use_malloc)
+    free (buf);
   return 0;
 }
 
--- libc/sysdeps/unix/sysv/linux/ifaddrs.c.jj	2007-03-13 17:42:17.000000000 +0100
+++ libc/sysdeps/unix/sysv/linux/ifaddrs.c	2007-03-15 09:18:43.000000000 +0100
@@ -122,37 +122,36 @@ int
 __netlink_request (struct netlink_handle *h, int type)
 {
   struct netlink_res *nlm_next;
-  struct netlink_res **new_nlm_list;
-  static volatile size_t buf_size = 4096;
-  char *buf;
   struct sockaddr_nl nladdr;
   struct nlmsghdr *nlmh;
   ssize_t read_len;
   bool done = false;
-  bool use_malloc = false;
 
-  if (__netlink_sendreq (h, type) < 0)
-    return -1;
+#ifdef PAGE_SIZE
+  /* Help the compiler optimize out the malloc call if PAGE_SIZE
+     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
+  const size_t buf_size = PAGE_SIZE;
+#else
+  const size_t buf_size = __getpagesize ();
+#endif
+  bool use_malloc = false;
+  char *buf;
 
-  size_t this_buf_size = buf_size;
-  size_t orig_this_buf_size = this_buf_size;
-  if (__libc_use_alloca (this_buf_size))
-    buf = alloca (this_buf_size);
+  if (__libc_use_alloca (buf_size))
+    buf = alloca (buf_size);
   else
     {
-      buf = malloc (this_buf_size);
+      buf = malloc (buf_size);
       if (buf != NULL)
 	use_malloc = true;
       else
 	goto out_fail;
     }
 
-  struct iovec iov = { buf, this_buf_size };
+  struct iovec iov = { buf, buf_size };
 
-  if (h->nlm_list != NULL)
-    new_nlm_list = &h->end_ptr->next;
-  else
-    new_nlm_list = &h->nlm_list;
+  if (__netlink_sendreq (h, type) < 0)
+    goto out_fail;
 
   while (! done)
     {
@@ -172,48 +171,7 @@ __netlink_request (struct netlink_handle
 	continue;
 
       if (__builtin_expect (msg.msg_flags & MSG_TRUNC, 0))
-	{
-	  if (this_buf_size >= SIZE_MAX / 2)
-	    goto out_fail;
-
-	  nlm_next = *new_nlm_list;
-	  while (nlm_next != NULL)
-	    {
-	      struct netlink_res *tmpptr;
-
-	      tmpptr = nlm_next->next;
-	      free (nlm_next);
-	      nlm_next = tmpptr;
-	    }
-	  *new_nlm_list = NULL;
-
-	  if (__libc_use_alloca (2 * this_buf_size))
-	    buf = extend_alloca (buf, this_buf_size, 2 * this_buf_size);
-	  else
-	    {
-	      this_buf_size *= 2;
-
-	      char *new_buf = realloc (use_malloc ? buf : NULL, this_buf_size);
-	      if (new_buf == NULL)
-		goto out_fail;
-	      buf = new_buf;
-
-	      use_malloc = true;
-	    }
-	  buf_size = this_buf_size;
-
-	  iov.iov_base = buf;
-	  iov.iov_len = this_buf_size;
-
-	  /* Increase sequence number, so that we can distinguish
-	     between old and new request messages.  */
-	  h->seq++;
-
-	  if (__netlink_sendreq (h, type) < 0)
-	    goto out_fail;
-
-	  continue;
-	}
+	goto out_fail;
 
       size_t count = 0;
       size_t remaining_len = read_len;
@@ -237,36 +195,6 @@ __netlink_request (struct netlink_handle
 	      struct nlmsgerr *nlerr = (struct nlmsgerr *) NLMSG_DATA (nlmh);
 	      if (nlmh->nlmsg_len < NLMSG_LENGTH (sizeof (struct nlmsgerr)))
 		errno = EIO;
-	      else if (nlerr->error == -EBUSY
-		       && orig_this_buf_size != this_buf_size)
-		{
-		  /* If EBUSY and MSG_TRUNC was seen, try again with a new
-		     netlink socket.  */
-		  struct netlink_handle hold = *h;
-		  if (__netlink_open (h) < 0)
-		    {
-		      *h = hold;
-		      goto out_fail;
-		    }
-		  __netlink_close (&hold);
-		  orig_this_buf_size = this_buf_size;
-		  nlm_next = *new_nlm_list;
-		  while (nlm_next != NULL)
-		    {
-		      struct netlink_res *tmpptr;
-
-		      tmpptr = nlm_next->next;
-		      free (nlm_next);
-		      nlm_next = tmpptr;
-		    }
-		  *new_nlm_list = NULL;
-		  count = 0;
-		  h->seq++;
-
-		  if (__netlink_sendreq (h, type) < 0)
-		    goto out_fail;
-		  break;
-		}
 	      else
 		errno = -nlerr->error;
 	      goto out_fail;


	Jakub