Race condition in IPv6 DAD detection code?

[prabhakar lakhera]

Hi,

I had sent a mail sometime back regarding some queries/concern regarding v6
DAD detection code.
Here's what my concern is, drawing tables for the whole sequence in hopes
of getting some replies:

  *CPU1*

 *CPU2*

In nd6_dad_na_input...



  struct dadq *dp;



        if (ifa == NULL)

                panic("ifa == NULL in nd6_dad_na_input");



        dp = nd6_dad_find(ifa);

        if (dp)







nd6_dad_timer.. last attempt and NA counter not yet incremented by the
thread on CPU1, therefore address considered to be not duplicated.

if (dp->dad_ns_ocount < dp->dad_count) {

…..

        } else {

                /*

                 * We have transmitted sufficient number of DAD packets.

                 * See what we've got.

                 */

                int duplicate;



                duplicate = 0;



                if (dp->dad_na_icount) {

                        /*

                         * the check is in nd6_dad_na_input(),

                         * but just in case

                         */

                        duplicate++;

                }



                if (dp->dad_ns_icount) {

                        /* We've seen NS, means DAD has failed. */

                        duplicate++;

                }



                if (duplicate) {   ß----- Duplicate 0 here

                        /* (*dp) will be freed in nd6_dad_duplicated() */

                        dp = NULL;

                        nd6_dad_duplicated(ifa);

                } else {



In nd6_dad_na_input...



        dp->dad_na_icount++;



        /* remove the address. */

        nd6_dad_duplicated(ifa);



Inside nd6_dad_duplicated…



nd6_dad_duplicated(struct ifaddr *ifa)

{

        struct in6_ifaddr *ia = (struct in6_ifaddr *)ifa;

        struct ifnet *ifp;

        struct dadq *dp;

        char ip6buf[INET6_ADDRSTRLEN];



        dp = nd6_dad_find(ifa);

        if (dp == NULL) {

                log(LOG_ERR, "nd6_dad_duplicated: DAD structure not
found\n");

                return;

        }



       …. --à dp is not NULL as has not yet been removed from queue.









Nd6_dad_timer executing else….



/*

                         * We are done with DAD.  No NA came, no NS came.

                         * No duplicate address found.

                         */

                        ia->ia6_flags &= ~IN6_IFF_TENTATIVE;



                        nd6log((LOG_DEBUG,

                            "%s: DAD complete for %s - no duplicates
found\n",

                            if_name(ifa->ifa_ifp),

                            ip6_sprintf(ip6buf, &ia->ia_addr.sin6_addr)));



                        TAILQ_REMOVE(&V_dadq, (struct dadq *)dp, dad_list);

                        free(dp, M_IP6NDP);

                        dp = NULL;

                        ifa_free(ifa);

                }

In nd6_dad_duplicated(struct ifaddr *ifa) …

*Working on freed pointer???

*

*        log(LOG_ERR, "%s: DAD detected duplicate IPv6 address %s: "*

*            "NS in/out=%d/%d, NA in=%d\n",*

*            if_name(ifa->ifa_ifp), ip6_sprintf(ip6buf,
&ia->ia_addr.sin6_addr),*

*            dp->dad_ns_icount, dp->dad_ns_ocount, dp->dad_na_icount);*





On Wed, Mar 7, 2012 at 5:51 PM, prabhakar lakhera <
prabhakar.lakhera at gmail.com> wrote:

> Hi,
>
> I was puzzled to look at DAD detection code in FreeBSD. We check for
> counters for any received NA/NS for DAD in nd6_dad_timer:
>
>   if (dp->dad_na_icount) {
>  1326                         /*
>  1327                          * the check is in nd6_dad_na_input(),
>  1328                          * but just in case
>  1329                          */
>  1330                         duplicate++;
>  1331                 }
>  1332
>  1333                 if (dp->dad_ns_icount) {
>  1334                         /* We've seen NS, means DAD has failed. */
>  1335                         duplicate++;
>  1336                 }
>  1337
>  1338                 if (duplicate) {
>  1339                         /* (*dp) will be freed in
> nd6_dad_duplicated() */
>  1340                         dp = NULL;
>  1341                         nd6_dad_duplicated(ifa);
>
> the function later calls nd6_dad_duplicated to perform the remaining work
> if the address is detected duplicate.
>
> nd6_dad_duplicated also gets called from nd6_dad_na_input
> and  nd6_dad_ns_input, both the functions are the only places which
> increment the input NA/NS counters respectively.
>
>  1505 static void
>  1506 nd6_dad_na_input(struct ifaddr *ifa)
>  1507 {
>  1508         struct dadq *dp;
>  1509
>  1510         if (ifa == NULL)
>  1511                 panic("ifa == NULL in nd6_dad_na_input");
>  1512
>  1513         dp = nd6_dad_find(ifa);
>  1514         if (dp)
>  1515                 dp->dad_na_icount++;
>  1516
>  1517         /* remove the address. */
>  1518         nd6_dad_duplicated(ifa);
>  1519 }
>
> nd6_dad_duplicated stops the timer among other things.
>
> Why nd6_dad_timer need check on these counters if we stop the timer on DAD
> failure anyways?
> Ok.. may be just an optimization which just "hopes" that the counters have
> been updated but the nd6_dad_*_input has not yet called nd6_dad_duplicated.
>
> Can the this timer and na packet processing ever run in parallel, I don;t
> see dp being protected by any locks, nor does it seem that it's been
> reference counted.
> Any explanation will be highly appreciated.
>
> Best,
>
> Prabhakar
>
>
>
>
>
>