Bug 257511 - iflib: enabling promisc under netmap causes lockup when interface is down
Summary: iflib: enabling promisc under netmap causes lockup when interface is down
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.2-STABLE
Hardware: amd64 Any
: --- Affects Only Me
Assignee: Vincenzo Maffione
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-30 19:35 UTC by Brian Poole
Modified: 2021-09-15 13:07 UTC (History)
3 users (show)

See Also:


Attachments
Test program demonstrating issue (1.43 KB, text/plain)
2021-07-30 19:35 UTC, Brian Poole
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Poole 2021-07-30 19:35:47 UTC
Created attachment 226806 [details]
Test program demonstrating issue

Hello,

I am using the igb driver to collect packets promiscuously. Since I am using netmap, I placed the ioctl(SIOCSIFFLAGS) call to enable promisc after netmap configuration. That way I avoid sending a flood of packets up the network stack if there are packets flowing in on start up. However, I have noticed if I follow these steps, but have not also marked the interface as up, the driver will lock up when I try to receive packets. In my testing, killing the process does not solve the problem - I end up rebooting to restore functionality. I have observed this with multiple iflib drivers - igb, ixgbe, ixl, and ice - leading me to strongly believe it is a problem in iflib. Other drivers I have tested don't allow netmap configuration for an interface that isn't up.

The test program I attached shows my procedure. By running './promisc netmap:igb0', the program opens the port in netmap, then sets the promisc bit, then tries to read a packet. If the interface is up, everything works and it prints 'success'. If the interface is not up, the program will hang. I often don't assign an address to a port that is being used by netmap for promiscuous mode so this problem gets me.

Looking at the iflib.c code, function iflib_if_ioctl(), the SIOCSIFFLAGS case checks if the interface is up, then if running, and finally checks the bits. If the interface is down as in my example, then it checks for running - which I believe is true since netmap has been initialized. In that conditional, the function calls iflib_stop() which has the following comment in it:

    /*
     * Stop any pending txsync/rxsync and prevent new ones
     * form starting. Processes blocked in poll() will get
     * POLLERR.
     */
    netmap_disable_all_rings(ctx->ifc_ifp);

The ioctl() call returns 0 in this case so there's no sign that netmap has been disabled. However, when the poll() call then tries to rxsync() it hangs.

I can avoid problems by making sure the interface is always up before starting my program. However, I'd prefer iflib not hang should an interface be down.
Comment 1 Brian Poole 2021-08-10 13:58:20 UTC
I tried commenting out the iflib_stop() call and setting err instead. With this change, I can collect packets with netmap using the igb, ixgbe, ixl, and ice drivers whether the interface is up or down. Setting the promiscuous bit after initializing netmap does not cause a hang if the interface is down. I'm not sure of the implications of my change, but for my use case it appears to work well.

--- sys/net/iflib.c	(revision 370286)
+++ sys/net/iflib.c	(working copy)
@@ -4230,7 +4296,8 @@
 			} else
 				reinit = 1;
 		} else if (if_getdrvflags(ifp) & IFF_DRV_RUNNING) {
-			iflib_stop(ctx);
+			err = ENETDOWN; // Can't set flags while interface is down
+//			iflib_stop(ctx); // Calling this locks up netmap
 		}
 		ctx->ifc_if_flags = if_getflags(ifp);
 		CTX_UNLOCK(ctx);
Comment 2 Vincenzo Maffione freebsd_committer 2021-08-13 15:17:20 UTC
Thanks for reporting.

As you actually found out by yourself, netmap should not allow you to RXSYNC/TXSYNC while the interface is down or not running. I'll fix this for iflib, perform some tests, and report back on this thread.
Comment 3 Brian Poole 2021-09-13 18:09:09 UTC
First a warning to all - do not try the hack I suggested for iflib.c. That caused other bugs where ifconfig commands were not taking effect because iflib_stop() was not called.

Vincenzo, thank you for your offer of help. I just reviewed the netmap source and don't understand where the problem lies. netmap_disable_all_rings() will set NM_KR_STOPPED for all rings. Then nm_kr_tryget() is called before any [rt]xsync and checks if the ring is stopped. Yet my polling process hangs in a state of STOP such that kill -9 doesn't remove it.

Please let me know if you need any testing or further details.
Comment 4 Vincenzo Maffione freebsd_committer 2021-09-14 21:59:43 UTC
Hi Brian,
  Indeed, your proposed change is of course not valid.

I already took a look at this issue some weeks ago, and I realized that the lockup happens because the rings have NM_KR_LOCKED set. When you call txsync/rxsync from userspace, nm_kr_tryget() will actually wait forever, sleeping in a loop. Note that netmap_disable_all_rings() sets NM_KR_LOCKED, and not NM_KR_STOPPED. So the situation is different from what you are saying.

If NM_KR_STOPPED were set, nm_kr_tryget() would just return, and your userspace program would receive an error code from poll()/ioctl(). That would be the correct behaviour in your scenario.

Now, it turns out that NM_KR_LOCKED is set for a reason. While netmap applications are running on an interface, it can happen that the interface goes through down-up (reset) cycles, e.g. because you are calling nm_open() to switch netmap mode (on/off) for some rings, or because you run ifconfig to enable/disable offloads, set/unset promisc mode, or any other reasons. While this reset cycle is in progress, we want to prevent the running netmap applications from calling txsync/rxsync, effectively locking the ring (NM_KR_LOCKED), but we do not want to propagate any error to userspace, in such a way that the reset cycle is transparent for applications.

The problem is that if the interface is down because the administrator keeps it down (your use case), or if for any other reason the interface is down for an undefined amount of time (rather than being down temporarily, for the time of a reset cycle), we should have NM_KR_STOPPED set rather than NM_KR_LOCKED. However, the current code does distinguish between the two cases, and always set NM_KR_LOCKED (except for some corner cases which are not relevant here).

Sorry about that, but I still have to find some time to keep thinking about this, since I don't see any easy solution.
Comment 5 Brian Poole 2021-09-15 13:07:48 UTC
Thank you for your added explanation. I was looking at the 12.2 source, where I believe netmap_disable_all_rings() does set NM_KR_STOPPED? But I see main, 13.0, and 12-STABLE now all use NM_KR_LOCKED instead. Best of luck and I really appreciate your support of netmap.