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

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

Details

Reviewers

glebius
ae
pouria

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 created this revision.Mar 19 2025, 3:01 AM

Herald added subscribers: melifaro, imp. · View Herald TranscriptMar 19 2025, 3:01 AM

zlei requested review of this revision.Mar 19 2025, 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.Mar 19 2025, 3:03 AM

kp added a subscriber: kp.Mar 19 2025, 4:45 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.

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.

zlei mentioned this in D55875: ifnet: Add some sanity checks.Mon, Mar 16, 11:15 AM

pouria accepted this revision as: pouria.Mon, Mar 16, 11:17 AM

bms added a subscriber: bms.Mon, Mar 16, 11:17 AM