ifnet: Remove a redundant check for flag IFF_DYING from ifunit_ref()

[zlei]

Summary

A dying interface will be detached from the global network interface
list (V_ifnet) and then flagged with IFF_DYING. So it is not possible to
have an interface flagged with IFF_DYING but on the list.

No functional change intended.

MFC after: 1 week

[glebius]

The check wasn't done with sufficient locking anyway. IMHO, when everything gets right in this area of the network stack, we will no longer need this flag.

[kp]

sys/net/if.c

2238

Is it worth sticking an MPASS(!(ifp->if_flags & IF_DYING)); here?

I know there are bugs around interface destruction, and this might help us catch such things.

[ae]

I think adding MPASS is better than removing. There is no locking, and it is still possible, that the code you are modifying will first get ifp pointer and then this ifp will be unlinked and marked as DYING :)

[mjg]

Looking at if_unlink_ifnet:

IFNET_WLOCK();
CK_STAILQ_FOREACH(iter, &V_ifnet, if_link)
        if (iter == ifp) {
                CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
                if (!vmove)
                        ifp->if_flags |= IFF_DYING;
                found = 1;
                break;
        }

it very much *is* possible to race it and spot the flag.

cpu0                                                                                            cpu1
CK_STAILQ_FOREACH(ifp, &V_ifnet, if_link) {
       /* ifp is now loaded */
                                                                                                     CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
                                                                                                     ifp->if_flags |= IFF_DYING;
/* the flag *can* be visible by now */
if (strncmp(name, ifp->if_xname, IFNAMSIZ) == 0 &&
!(ifp->if_flags & IFF_DYING))

If anything the bug is that the flag can show up immediately after it was tested for not being there. It may be this happens to be harmless.

However, the claim that flag is impossible to spot here does not hold.

[zlei]

In D49412#1126775, @mjg wrote:

Looking at if_unlink_ifnet:

IFNET_WLOCK();
CK_STAILQ_FOREACH(iter, &V_ifnet, if_link)
        if (iter == ifp) {
                CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
                if (!vmove)
                        ifp->if_flags |= IFF_DYING;
                found = 1;
                break;
        }

it very much *is* possible to race it and spot the flag.

cpu0                                                                                            cpu1
CK_STAILQ_FOREACH(ifp, &V_ifnet, if_link) {
       /* ifp is now loaded */
                                                                                                     CK_STAILQ_REMOVE(&V_ifnet, ifp, ifnet, if_link);
                                                                                                     ifp->if_flags |= IFF_DYING;
/* the flag *can* be visible by now */
if (strncmp(name, ifp->if_xname, IFNAMSIZ) == 0 &&
!(ifp->if_flags & IFF_DYING))

If anything the bug is that the flag can show up immediately after it was tested for not being there. It may be this happens to be harmless.

However, the claim that flag is impossible to spot here does not hold.

Ahh, yes, I forgot the read/write lock, so it is possible for a thread to see ifp with IFF_DYING flag but not on the list ( V_ifnet ).

Well ifunit_ref() does not get blocked, or it will block if_unlink_ifnet(), so even a thread do not see ifp with IFF_DYING, it will if later another thread calls if_unlink_ifnet(). So we do not need this IFF_DYING here, but need other means to make it right.

I'll revise the commit message.