[uclibc-ng-devel] [PATCH] Fix map_newlink abort when interface list changes during getifaddrs

Waldemar Brodkorb wbx at uclibc-ng.org
Fri Jan 31 07:52:40 CET 2020


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 at 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 at 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 at uclibc-ng.org
> https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel



More information about the devel mailing list