[uclibc-ng-devel] Fix map_newlink abort when interface list changes during getifaddrs
diff mbox series

Message ID 20200124124324.13732-1-vincent.houyi@gmail.com
State Accepted
Headers show
Series
  • [uclibc-ng-devel] Fix map_newlink abort when interface list changes during getifaddrs
Related show

Commit Message

Vincent Hou Jan. 24, 2020, 12:43 p.m. UTC
map_newlink() may abort when interface list changed between netlink
request for getting interfaces and getting addresses. This commit is
ported from the same change from glibc commit.

Signed-off-by: Vincent Hou <vincent.houyi@gmail.com>
---

Hi all,

App code making a getaddrinfo() call may get a occasional abort from
map_newlink(). The reason of the abort is getifaddrs() uses an out-dated
interface list.

In getaddrinfo() call, it will call a function getifaddrs() who retrieves
all interfaces and addresses from kernel via netlink, returning an array 
of interfaces and addresses. It will then call a function map_newlink()
to build a mapping between the interfaces’ index into the returning array
to the interface id in kernel. Then getifaddrs() will bind the 
relationship between interface and the address using the mapping from 
previous step.

The interfaces and addresses are retrieved from two separate netlink 
requests. It retrieves interface lists then followed by address list.
Between two requests, kernel may make change to interfaces and addresses.
If a new interface and address is added between the requests, the 
newly-added interface doesn’t present in the interface list, but the 
new-added address does. So the address will point to an non-existent
interface index.

This issue is fixed by glibc and this change ports the commit from glibc
(with some minor changes).

Thanks,
Vincent Hou
 libc/inet/ifaddrs.c | 53 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 11 deletions(-)

Comments

Vincent Hou Jan. 31, 2020, 4:16 a.m. UTC | #1
Hi all,

I've set this patch under review on Patchwork. This is the first time I
push for open source project. Please correct me if this is not the right
procedure.

I couldn't find a stable way to reproduce the issue, but it does happen
time to time. Not seeing the same issue after applying this patch on my
environment.

Any comments on the patch?

Thanks!
Vincent

On Fri, 24 Jan 2020 at 20:43, Vincent Hou <vincent.houyi@gmail.com> wrote:

> map_newlink() may abort when interface list changed between netlink
> request for getting interfaces and getting addresses. This commit is
> ported from the same change from glibc commit.
>
> Signed-off-by: Vincent Hou <vincent.houyi@gmail.com>
> ---
>
> Hi all,
>
> App code making a getaddrinfo() call may get a occasional abort from
> map_newlink(). The reason of the abort is getifaddrs() uses an out-dated
> interface list.
>
> In getaddrinfo() call, it will call a function getifaddrs() who retrieves
> all interfaces and addresses from kernel via netlink, returning an array
> of interfaces and addresses. It will then call a function map_newlink()
> to build a mapping between the interfaces’ index into the returning array
> to the interface id in kernel. Then getifaddrs() will bind the
> relationship between interface and the address using the mapping from
> previous step.
>
> The interfaces and addresses are retrieved from two separate netlink
> requests. It retrieves interface lists then followed by address list.
> Between two requests, kernel may make change to interfaces and addresses.
> If a new interface and address is added between the requests, the
> newly-added interface doesn’t present in the interface list, but the
> new-added address does. So the address will point to an non-existent
> interface index.
>
> This issue is fixed by glibc and this change ports the commit from glibc
> (with some minor changes).
>
> Thanks,
> Vincent Hou
>  libc/inet/ifaddrs.c | 53 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/libc/inet/ifaddrs.c b/libc/inet/ifaddrs.c
> index 0c9310651..72771d35a 100644
> --- a/libc/inet/ifaddrs.c
> +++ b/libc/inet/ifaddrs.c
> @@ -339,17 +339,19 @@ map_newlink (int idx, struct ifaddrs_storage *ifas,
> int *map, int max)
>        else if (map[i] == idx)
>         return i;
>      }
> -  /* This should never be reached. If this will be reached, we have
> -     a very big problem.  */
> -  abort ();
> +
> +  /* This means interfaces changed inbetween the reading of the
> +     RTM_GETLINK and RTM_GETADDR information.  We have to repeat
> +     everything.  */
> +  return -1;
>  }
>
>
>  /* Create a linked list of `struct ifaddrs' structures, one for each
>     network interface on the host machine.  If successful, store the
>     list in *IFAP and return 0.  On errors, return -1 and set `errno'.  */
> -int
> -getifaddrs (struct ifaddrs **ifap)
> +static int
> +getifaddrs_internal (struct ifaddrs **ifap)
>  {
>    struct netlink_handle nh = { 0, 0, 0, NULL, NULL };
>    struct netlink_res *nlp;
> @@ -496,6 +498,13 @@ getifaddrs (struct ifaddrs **ifap)
>                  kernel.  */
>               ifa_index = map_newlink (ifim->ifi_index - 1, ifas,
>                                        map_newlink_data, newlink);
> +             if (__builtin_expect (ifa_index == -1, 0))
> +               {
> +               try_again:
> +                 result = -EAGAIN;
> +                 free (ifas);
> +                 goto exit_free;
> +               }
>               ifas[ifa_index].ifa.ifa_flags = ifim->ifi_flags;
>
>               while (RTA_OK (rta, rtasize))
> @@ -580,9 +589,11 @@ getifaddrs (struct ifaddrs **ifap)
>                  that we have holes in the interface part of the list,
>                  but we always have already the interface for this
> address.  */
>               ifa_index = newlink + newaddr_idx;
> -             ifas[ifa_index].ifa.ifa_flags
> -               = ifas[map_newlink (ifam->ifa_index - 1, ifas,
> -                                   map_newlink_data,
> newlink)].ifa.ifa_flags;
> +             int idx = map_newlink (ifam->ifa_index - 1, ifas,
> +                                    map_newlink_data, newlink);
> +             if (__builtin_expect (idx == -1, 0))
> +               goto try_again;
> +             ifas[ifa_index].ifa.ifa_flags = ifas[idx].ifa.ifa_flags;
>               if (ifa_index > 0)
>                 ifas[ifa_index - 1].ifa.ifa_next = &ifas[ifa_index].ifa;
>               ++newaddr_idx;
> @@ -768,9 +779,13 @@ getifaddrs (struct ifaddrs **ifap)
>               /* If we didn't get the interface name with the
>                  address, use the name from the interface entry.  */
>               if (ifas[ifa_index].ifa.ifa_name == NULL)
> -               ifas[ifa_index].ifa.ifa_name
> -                 = ifas[map_newlink (ifam->ifa_index - 1, ifas,
> -                                     map_newlink_data,
> newlink)].ifa.ifa_name;
> +               {
> +                 int idx = map_newlink (ifam->ifa_index - 1, ifas,
> +                                        map_newlink_data, newlink);
> +                 if (__builtin_expect (idx == -1, 0))
> +                   goto try_again;
> +                 ifas[ifa_index].ifa.ifa_name = ifas[idx].ifa.ifa_name;
> +               }
>
>               /* Calculate the netmask.  */
>               if (ifas[ifa_index].ifa.ifa_addr
> @@ -850,6 +865,22 @@ getifaddrs (struct ifaddrs **ifap)
>
>    return result;
>  }
> +
> +
> +/* Create a linked list of `struct ifaddrs' structures, one for each
> +   network interface on the host machine.  If successful, store the
> +   list in *IFAP and return 0.  On errors, return -1 and set `errno'.  */
> +int
> +getifaddrs (struct ifaddrs **ifap)
> +{
> +  int res;
> +
> +  do
> +    res = getifaddrs_internal (ifap);
> +  while (res == -EAGAIN);
> +
> +  return res;
> +}
>  libc_hidden_def(getifaddrs)
>
>  void
> --
> 2.18.0
>
>
Waldemar Brodkorb Jan. 31, 2020, 6:52 a.m. UTC | #2
Hi Vincent,

I have applied your patch, thx.

best regards
 Waldemar

Vincent Hou wrote,

> Hi all,
> 
> I've set this patch under review on Patchwork. This is the first time I push
> for open source project. Please correct me if this is not the right procedure.
> 
> I couldn't find a stable way to reproduce the issue, but it does happen time to
> time. Not seeing the same issue after applying this patch on my environment.
> 
> Any comments on the patch?
> 
> Thanks!
> Vincent
> 
> On Fri, 24 Jan 2020 at 20:43, Vincent Hou <vincent.houyi@gmail.com> wrote:
> 
>     map_newlink() may abort when interface list changed between netlink
>     request for getting interfaces and getting addresses. This commit is
>     ported from the same change from glibc commit.
> 
>     Signed-off-by: Vincent Hou <vincent.houyi@gmail.com>
>     ---
> 
>     Hi all,
> 
>     App code making a getaddrinfo() call may get a occasional abort from
>     map_newlink(). The reason of the abort is getifaddrs() uses an out-dated
>     interface list.
> 
>     In getaddrinfo() call, it will call a function getifaddrs() who retrieves
>     all interfaces and addresses from kernel via netlink, returning an array
>     of interfaces and addresses. It will then call a function map_newlink()
>     to build a mapping between the interfaces’ index into the returning array
>     to the interface id in kernel. Then getifaddrs() will bind the
>     relationship between interface and the address using the mapping from
>     previous step.
> 
>     The interfaces and addresses are retrieved from two separate netlink
>     requests. It retrieves interface lists then followed by address list.
>     Between two requests, kernel may make change to interfaces and addresses.
>     If a new interface and address is added between the requests, the
>     newly-added interface doesn’t present in the interface list, but the
>     new-added address does. So the address will point to an non-existent
>     interface index.
> 
>     This issue is fixed by glibc and this change ports the commit from glibc
>     (with some minor changes).
> 
>     Thanks,
>     Vincent Hou
>      libc/inet/ifaddrs.c | 53 +++++++++++++++++++++++++++++++++++----------
>      1 file changed, 42 insertions(+), 11 deletions(-)
> 
>     diff --git a/libc/inet/ifaddrs.c b/libc/inet/ifaddrs.c
>     index 0c9310651..72771d35a 100644
>     --- a/libc/inet/ifaddrs.c
>     +++ b/libc/inet/ifaddrs.c
>     @@ -339,17 +339,19 @@ map_newlink (int idx, struct ifaddrs_storage *ifas,
>     int *map, int max)
>            else if (map[i] == idx)
>             return i;
>          }
>     -  /* This should never be reached. If this will be reached, we have
>     -     a very big problem.  */
>     -  abort ();
>     +
>     +  /* This means interfaces changed inbetween the reading of the
>     +     RTM_GETLINK and RTM_GETADDR information.  We have to repeat
>     +     everything.  */
>     +  return -1;
>      }
> 
> 
>      /* Create a linked list of `struct ifaddrs' structures, one for each
>         network interface on the host machine.  If successful, store the
>         list in *IFAP and return 0.  On errors, return -1 and set `errno'.  */
>     -int
>     -getifaddrs (struct ifaddrs **ifap)
>     +static int
>     +getifaddrs_internal (struct ifaddrs **ifap)
>      {
>        struct netlink_handle nh = { 0, 0, 0, NULL, NULL };
>        struct netlink_res *nlp;
>     @@ -496,6 +498,13 @@ getifaddrs (struct ifaddrs **ifap)
>                      kernel.  */
>                   ifa_index = map_newlink (ifim->ifi_index - 1, ifas,
>                                            map_newlink_data, newlink);
>     +             if (__builtin_expect (ifa_index == -1, 0))
>     +               {
>     +               try_again:
>     +                 result = -EAGAIN;
>     +                 free (ifas);
>     +                 goto exit_free;
>     +               }
>                   ifas[ifa_index].ifa.ifa_flags = ifim->ifi_flags;
> 
>                   while (RTA_OK (rta, rtasize))
>     @@ -580,9 +589,11 @@ getifaddrs (struct ifaddrs **ifap)
>                      that we have holes in the interface part of the list,
>                      but we always have already the interface for this
>     address.  */
>                   ifa_index = newlink + newaddr_idx;
>     -             ifas[ifa_index].ifa.ifa_flags
>     -               = ifas[map_newlink (ifam->ifa_index - 1, ifas,
>     -                                   map_newlink_data,
>     newlink)].ifa.ifa_flags;
>     +             int idx = map_newlink (ifam->ifa_index - 1, ifas,
>     +                                    map_newlink_data, newlink);
>     +             if (__builtin_expect (idx == -1, 0))
>     +               goto try_again;
>     +             ifas[ifa_index].ifa.ifa_flags = ifas[idx].ifa.ifa_flags;
>                   if (ifa_index > 0)
>                     ifas[ifa_index - 1].ifa.ifa_next = &ifas[ifa_index].ifa;
>                   ++newaddr_idx;
>     @@ -768,9 +779,13 @@ getifaddrs (struct ifaddrs **ifap)
>                   /* If we didn't get the interface name with the
>                      address, use the name from the interface entry.  */
>                   if (ifas[ifa_index].ifa.ifa_name == NULL)
>     -               ifas[ifa_index].ifa.ifa_name
>     -                 = ifas[map_newlink (ifam->ifa_index - 1, ifas,
>     -                                     map_newlink_data,
>     newlink)].ifa.ifa_name;
>     +               {
>     +                 int idx = map_newlink (ifam->ifa_index - 1, ifas,
>     +                                        map_newlink_data, newlink);
>     +                 if (__builtin_expect (idx == -1, 0))
>     +                   goto try_again;
>     +                 ifas[ifa_index].ifa.ifa_name = ifas[idx].ifa.ifa_name;
>     +               }
> 
>                   /* Calculate the netmask.  */
>                   if (ifas[ifa_index].ifa.ifa_addr
>     @@ -850,6 +865,22 @@ getifaddrs (struct ifaddrs **ifap)
> 
>        return result;
>      }
>     +
>     +
>     +/* Create a linked list of `struct ifaddrs' structures, one for each
>     +   network interface on the host machine.  If successful, store the
>     +   list in *IFAP and return 0.  On errors, return -1 and set `errno'.  */
>     +int
>     +getifaddrs (struct ifaddrs **ifap)
>     +{
>     +  int res;
>     +
>     +  do
>     +    res = getifaddrs_internal (ifap);
>     +  while (res == -EAGAIN);
>     +
>     +  return res;
>     +}
>      libc_hidden_def(getifaddrs)
> 
>      void
>     --
>     2.18.0
> 
> 

> _______________________________________________
> devel mailing list
> devel@uclibc-ng.org
> https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel

Patch
diff mbox series

diff --git a/libc/inet/ifaddrs.c b/libc/inet/ifaddrs.c
index 0c9310651..72771d35a 100644
--- a/libc/inet/ifaddrs.c
+++ b/libc/inet/ifaddrs.c
@@ -339,17 +339,19 @@  map_newlink (int idx, struct ifaddrs_storage *ifas, int *map, int max)
       else if (map[i] == idx)
 	return i;
     }
-  /* This should never be reached. If this will be reached, we have
-     a very big problem.  */
-  abort ();
+
+  /* This means interfaces changed inbetween the reading of the
+     RTM_GETLINK and RTM_GETADDR information.  We have to repeat
+     everything.  */
+  return -1;
 }
 
 
 /* Create a linked list of `struct ifaddrs' structures, one for each
    network interface on the host machine.  If successful, store the
    list in *IFAP and return 0.  On errors, return -1 and set `errno'.  */
-int
-getifaddrs (struct ifaddrs **ifap)
+static int
+getifaddrs_internal (struct ifaddrs **ifap)
 {
   struct netlink_handle nh = { 0, 0, 0, NULL, NULL };
   struct netlink_res *nlp;
@@ -496,6 +498,13 @@  getifaddrs (struct ifaddrs **ifap)
 		 kernel.  */
 	      ifa_index = map_newlink (ifim->ifi_index - 1, ifas,
 				       map_newlink_data, newlink);
+	      if (__builtin_expect (ifa_index == -1, 0))
+		{
+		try_again:
+		  result = -EAGAIN;
+		  free (ifas);
+		  goto exit_free;
+		}
 	      ifas[ifa_index].ifa.ifa_flags = ifim->ifi_flags;
 
 	      while (RTA_OK (rta, rtasize))
@@ -580,9 +589,11 @@  getifaddrs (struct ifaddrs **ifap)
 		 that we have holes in the interface part of the list,
 		 but we always have already the interface for this address.  */
 	      ifa_index = newlink + newaddr_idx;
-	      ifas[ifa_index].ifa.ifa_flags
-		= ifas[map_newlink (ifam->ifa_index - 1, ifas,
-				    map_newlink_data, newlink)].ifa.ifa_flags;
+	      int idx = map_newlink (ifam->ifa_index - 1, ifas,
+				     map_newlink_data, newlink);
+	      if (__builtin_expect (idx == -1, 0))
+		goto try_again;
+	      ifas[ifa_index].ifa.ifa_flags = ifas[idx].ifa.ifa_flags;
 	      if (ifa_index > 0)
 		ifas[ifa_index - 1].ifa.ifa_next = &ifas[ifa_index].ifa;
 	      ++newaddr_idx;
@@ -768,9 +779,13 @@  getifaddrs (struct ifaddrs **ifap)
 	      /* If we didn't get the interface name with the
 		 address, use the name from the interface entry.  */
 	      if (ifas[ifa_index].ifa.ifa_name == NULL)
-		ifas[ifa_index].ifa.ifa_name
-		  = ifas[map_newlink (ifam->ifa_index - 1, ifas,
-				      map_newlink_data, newlink)].ifa.ifa_name;
+		{
+		  int idx = map_newlink (ifam->ifa_index - 1, ifas,
+					 map_newlink_data, newlink);
+		  if (__builtin_expect (idx == -1, 0))
+		    goto try_again;
+		  ifas[ifa_index].ifa.ifa_name = ifas[idx].ifa.ifa_name;
+		}
 
 	      /* Calculate the netmask.  */
 	      if (ifas[ifa_index].ifa.ifa_addr
@@ -850,6 +865,22 @@  getifaddrs (struct ifaddrs **ifap)
 
   return result;
 }
+
+
+/* Create a linked list of `struct ifaddrs' structures, one for each
+   network interface on the host machine.  If successful, store the
+   list in *IFAP and return 0.  On errors, return -1 and set `errno'.  */
+int
+getifaddrs (struct ifaddrs **ifap)
+{
+  int res;
+
+  do
+    res = getifaddrs_internal (ifap);
+  while (res == -EAGAIN);
+
+  return res;
+}
 libc_hidden_def(getifaddrs)
 
 void