[PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Daniel Alvarez]

Due to bug 21812, a lookup operation in map_newlink() turns out
into an insert because of holes in the interface part of the map.
This leads to incorrectly set the name of the interface to NULL when
the interface is not present for the address being processed (most
likely because the interface was added between the RTM_GETLINK and
RTM_GETADDR calls to the kernel). When such changes are detected
by the kernel, it'll mark the dump as "inconsistent" by setting
NLM_F_DUMP_INTR flag on the next netlink message.

This patch checks this condition and retries the whole operation.
Hopes are that next time the interface corresponding to the address
entry is present in the list and correct name is returned.

Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
---
 sysdeps/unix/sysv/linux/ifaddrs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
index 32381f54e4..5659c9605b 100644
--- a/sysdeps/unix/sysv/linux/ifaddrs.c
+++ b/sysdeps/unix/sysv/linux/ifaddrs.c
@@ -370,6 +370,14 @@ getifaddrs_internal (struct ifaddrs **ifap)
 	  if ((pid_t) nlh->nlmsg_pid != nh.pid || nlh->nlmsg_seq != nlp->seq)
 	    continue;
 
+      /* If the dump got interrupted, we can't relay on the results so
+         try again. */
+      if (nlh->nlmsg_flags & NLM_F_DUMP_INTR)
+        {
+          result = -EAGAIN;
+          goto exit_free;
+        }
+
 	  if (nlh->nlmsg_type == NLMSG_DONE)
 	    break;		/* ok */
 
-- 
2.15.1 (Apple Git-101)

Re: [PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Adhemerval Zanella]

On 15/06/2018 10:29, Daniel Alvarez wrote:
> Due to bug 21812, a lookup operation in map_newlink() turns out
> into an insert because of holes in the interface part of the map.
> This leads to incorrectly set the name of the interface to NULL when
> the interface is not present for the address being processed (most
> likely because the interface was added between the RTM_GETLINK and
> RTM_GETADDR calls to the kernel). When such changes are detected
> by the kernel, it'll mark the dump as "inconsistent" by setting
> NLM_F_DUMP_INTR flag on the next netlink message.
> 
> This patch checks this condition and retries the whole operation.
> Hopes are that next time the interface corresponding to the address
> entry is present in the list and correct name is returned.
> 
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>

We don't use DCO, instead we use copyright assignments. However the
change below seems to fit as a trivial change. 

Patch looks good and I can't think in a easy way to add testcase which
is guarantee to be reproducible (maybe by using a network namespace and
force inject an interface so kernel will set NLM_F_DUMP_INTR?).

> ---
>  sysdeps/unix/sysv/linux/ifaddrs.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
> index 32381f54e4..5659c9605b 100644
> --- a/sysdeps/unix/sysv/linux/ifaddrs.c
> +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
> @@ -370,6 +370,14 @@ getifaddrs_internal (struct ifaddrs **ifap)
>  	  if ((pid_t) nlh->nlmsg_pid != nh.pid || nlh->nlmsg_seq != nlp->seq)
>  	    continue;
>  
> +      /* If the dump got interrupted, we can't relay on the results so
> +         try again. */
> +      if (nlh->nlmsg_flags & NLM_F_DUMP_INTR)
> +        {
> +          result = -EAGAIN;
> +          goto exit_free;
> +        }
> +
>  	  if (nlh->nlmsg_type == NLMSG_DONE)
>  	    break;		/* ok */
>  
> 

Re: [PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Daniel Alvarez Sanchez]

Thanks a lot for reviewing. It'd be nice to get this in soon as it can
happen easily on systems with lots of interfaces.

Here it's a reproducer:

$ sudo unshare -n
# cat create-links.sh
#!/bin/bash
for i in {1..512}; do ifname="dum$i"; ip li add $ifname type dummy; ip
li set $ifname up; done
# ./create-links.sh
# cat loop-add-del-link.sh
#!/bin/bash
while :; do ifname="test$RANDOM"; ip li add $ifname type dummy; ip li
set $ifname up; sleep 0.1; ip li del $ifname; done
# ./loop-add-del-link.sh &
[1] 6260
# ./loop-add-del-link.sh &
[2] 6314
# ./loop-add-del-link.sh &
[3] 6374
# ./loop-add-del-link.sh &
[4] 6476
# ./loop-add-del-link.sh &
[5] 6628
# cat dalvarez-tester.c
#include <sys/types.h>
#include <ifaddrs.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

int main(void)
{
        struct ifaddrs *ifaddr, *ifa;

        for (;;)
        {
                if (getifaddrs(&ifaddr) == -1) {
                        perror("getifaddrs");
                        return EXIT_FAILURE;
                }


                /* Walk through linked list, maintaining head pointer so we
                   can free list later */

                for (ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) {
                        if (ifa->ifa_name == NULL) {
                                printf("Gotcha!!!!\n");
                                return EXIT_FAILURE;
                        }
                }

                freeifaddrs(ifaddr);
                usleep(10);
        }

        return EXIT_SUCCESS;
}
On Fri, Jun 15, 2018 at 9:13 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 15/06/2018 10:29, Daniel Alvarez wrote:
> > Due to bug 21812, a lookup operation in map_newlink() turns out
> > into an insert because of holes in the interface part of the map.
> > This leads to incorrectly set the name of the interface to NULL when
> > the interface is not present for the address being processed (most
> > likely because the interface was added between the RTM_GETLINK and
> > RTM_GETADDR calls to the kernel). When such changes are detected
> > by the kernel, it'll mark the dump as "inconsistent" by setting
> > NLM_F_DUMP_INTR flag on the next netlink message.
> >
> > This patch checks this condition and retries the whole operation.
> > Hopes are that next time the interface corresponding to the address
> > entry is present in the list and correct name is returned.
> >
> > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
>
> We don't use DCO, instead we use copyright assignments. However the
> change below seems to fit as a trivial change.
>
> Patch looks good and I can't think in a easy way to add testcase which
> is guarantee to be reproducible (maybe by using a network namespace and
> force inject an interface so kernel will set NLM_F_DUMP_INTR?).
>
> > ---
> >  sysdeps/unix/sysv/linux/ifaddrs.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
> > index 32381f54e4..5659c9605b 100644
> > --- a/sysdeps/unix/sysv/linux/ifaddrs.c
> > +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
> > @@ -370,6 +370,14 @@ getifaddrs_internal (struct ifaddrs **ifap)
> >         if ((pid_t) nlh->nlmsg_pid != nh.pid || nlh->nlmsg_seq != nlp->seq)
> >           continue;
> >
> > +      /* If the dump got interrupted, we can't relay on the results so
> > +         try again. */
> > +      if (nlh->nlmsg_flags & NLM_F_DUMP_INTR)
> > +        {
> > +          result = -EAGAIN;
> > +          goto exit_free;
> > +        }
> > +
> >         if (nlh->nlmsg_type == NLMSG_DONE)
> >           break;              /* ok */
> >
> >

Re: [PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Florian Weimer]

On 06/29/2018 08:57 AM, Daniel Alvarez Sanchez wrote:
> Thanks a lot for reviewing. It'd be nice to get this in soon as it can
> happen easily on systems with lots of interfaces.
> 
> Here it's a reproducer:
> 
> $ sudo unshare -n
> # cat create-links.sh
> #!/bin/bash
> for i in {1..512}; do ifname="dum$i"; ip li add $ifname type dummy; ip
> li set $ifname up; done
> # ./create-links.sh
> # cat loop-add-del-link.sh
> #!/bin/bash
> while :; do ifname="test$RANDOM"; ip li add $ifname type dummy; ip li
> set $ifname up; sleep 0.1; ip li del $ifname; done
> # ./loop-add-del-link.sh &
> [1] 6260
> # ./loop-add-del-link.sh &
> [2] 6314
> # ./loop-add-del-link.sh &
> [3] 6374
> # ./loop-add-del-link.sh &
> [4] 6476
> # ./loop-add-del-link.sh &
> [5] 6628
> # cat dalvarez-tester.c
> #include <sys/types.h>
> #include <ifaddrs.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>

Can you come up with a test that uses the loopback interface and 
loopback aliases devices only (lo:NNN)?  Or is this not possible?

I don't want to implement all the Netlink stuff for creating a dummy 
interface, and the kernel module might not be available.

Thanks,
Florian

Re: [PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Florian Weimer]

On 06/15/2018 03:29 PM, Daniel Alvarez wrote:
> Due to bug 21812, a lookup operation in map_newlink() turns out
> into an insert because of holes in the interface part of the map.
> This leads to incorrectly set the name of the interface to NULL when
> the interface is not present for the address being processed (most
> likely because the interface was added between the RTM_GETLINK and
> RTM_GETADDR calls to the kernel). When such changes are detected
> by the kernel, it'll mark the dump as "inconsistent" by setting
> NLM_F_DUMP_INTR flag on the next netlink message.
> 
> This patch checks this condition and retries the whole operation.
> Hopes are that next time the interface corresponding to the address
> entry is present in the list and correct name is returned.

I fixed a few cosmetic issues, added a ChangeLog entry, and committed 
this.  Thanks.

I'm working on a self-contained test case, too.

Thanks,
Florian

[PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Daniel Alvarez]

Due to bug 21812, a lookup operation in map_newlink() turns out
into an insert because of holes in the interface part of the map.
This leads to incorrectly set the name of the interface to NULL when
the interface is not present for the address being processed (most
likely because the interface was added between the RTM_GETLINK and
RTM_GETADDR calls to the kernel).

This patch checks this condition and retries the whole operation.
Hopes are that next time the interface corresponding to the address
entry is present in the list and correct name is returned.

Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
---
 sysdeps/unix/sysv/linux/ifaddrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
index 3bc9902..62421e8 100644
--- a/sysdeps/unix/sysv/linux/ifaddrs.c
+++ b/sysdeps/unix/sysv/linux/ifaddrs.c
@@ -740,7 +740,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
 		{
 		  int idx = map_newlink (ifam->ifa_index - 1, ifas,
 					 map_newlink_data, newlink);
-		  if (__glibc_unlikely (idx == -1))
+		  if (__glibc_unlikely (idx == -1) || ifas[idx].ifa.ifa_name == NULL)
 		    goto try_again;
 		  ifas[ifa_index].ifa.ifa_name = ifas[idx].ifa.ifa_name;
 		}
-- 
2.9.3

Re: [PATCH] [BZ #21812] getifaddrs() Don't return ifa entries with NULL names

[Florian Weimer]

On 07/24/2017 10:56 PM, Daniel Alvarez wrote:
> Due to bug 21812, a lookup operation in map_newlink() turns out
> into an insert because of holes in the interface part of the map.
> This leads to incorrectly set the name of the interface to NULL when
> the interface is not present for the address being processed (most
> likely because the interface was added between the RTM_GETLINK and
> RTM_GETADDR calls to the kernel).
> 
> This patch checks this condition and retries the whole operation.
> Hopes are that next time the interface corresponding to the address
> entry is present in the list and correct name is returned.
> 
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

The downstream bug has a different patch:

  <https://bugzilla.redhat.com/show_bug.cgi?id=1472832>

I think using NLM_F_DUMP_INTR to check the inconsistency is preferable
(if it actually works).  glibc requires kernel 3.2 these days, so we can
assume that kernel support is present.

Thanks,
Florian