Page MenuHomeFreeBSD

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

Authored by zlei on Wed, Mar 19, 3:01 AM.

Details

Reviewers
glebius
ae
Group Reviewers
network
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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

zlei requested review of this revision.Wed, Mar 19, 3:01 AM

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.

This revision is now accepted and ready to land.Wed, Mar 19, 3:03 AM
kp added inline comments.
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.

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 :)

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.