Message ID | 20180615132915.83940-1-dalvarez@redhat.com |
---|---|
State | New |
Headers | show |
Series | [BZ,#21812] getifaddrs() Don't return ifa entries with NULL names | expand |
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 */ > >
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 */ > > > >
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
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
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 */