Same as D57387.

• increment more counters
• add a branch annotation

In D56942#1314723, @zlei wrote:

In D56942#1314704, @markj wrote:

In D56942#1314685, @zlei wrote:

I'm OK with this revision, although I think an explicit memory barrier while increasing / decreasing sc->sc_count will make the code more clear than a NULL check in the reader side (lagg_rr_start).

A memory barrier isn't sufficient. For sc_count and length(sc_ports) to be consistent with each other, you need a mutex.

The writer side is lagg_port_create() and lagg_port_destroy(). They are serialized. The reader side always checks sc_count before accessing sc_ports, so this order should be sufficient for the writer side,

diff --git a/sys/net/if_lagg.c b/sys/net/if_lagg.c
index 0333162da0d4..226565633b3a 100644
--- a/sys/net/if_lagg.c
+++ b/sys/net/if_lagg.c
@@ -879,7 +879,7 @@ lagg_port_create(struct lagg_softc *sc, struct ifnet *ifp)
                CK_SLIST_INSERT_AFTER(tlp, lp, lp_entries);
        else
                CK_SLIST_INSERT_HEAD(&sc->sc_ports, lp, lp_entries);
-       sc->sc_count++;
+       atomic_add_rel_int(&sc->sc_count, 1);
 
        lagg_setmulti(lp);
 
@@ -962,8 +962,8 @@ lagg_port_destroy(struct lagg_port *lp, int rundelport)
        }
 
        /* Finally, remove the port from the lagg */
+       atomic_add_rel_int(&sc->sc_count, -1);
        CK_SLIST_REMOVE(&sc->sc_ports, lp, lagg_port, lp_entries);
-       sc->sc_count--;
 
        /* Update the primary interface */
        if (lp == sc->sc_primary) {

The reader side use atomic_load_acq_int() to get sc_count, then iterate sc_ports.

In D57372#1314709, @novel wrote:

In D57372#1314650, @markj wrote:

in this case

You might further specify which case this is?

In case of running bhyve as a non-root user.

In D56942#1314685, @zlei wrote:

I'm OK with this revision, although I think an explicit memory barrier while increasing / decreasing sc->sc_count will make the code more clear than a NULL check in the reader side (lagg_rr_start).

in this case

Is it possible to add a regression test?

Ping? I've converted the locking here as requested.

Ping? I'd like to commit this if there are no objections. The bug it fixes is real, and this patch or something like it is required no matter how other if_lagg lifecycle issues are handled.

In D57274#1312947, @emaste wrote:

I mean, sure, I guess, but what's the threat model here? I sure hope you trust the tarballs you're unpacking to install a system not to be malicious, otherwise what's the point?

I could come up with a contrived exploit scenario, but yes if someone controls the tarballs being unpacked during install it's likely much easier to just provide a trojaned binary.

This came to secteam and I thought "sure, why not."

In D57247#1312477, @asomers wrote:

Not really a domain expert, either, am I. Can you give me a clue about how to test this? I can use virtio-scsi with bhyve. But how does one tell bhyve to tell the guest that the storage capacity has changed?

In D57244#1311781, @markj wrote:

Shall we update the kill test in tests/sys/capsicum/capmode.cc to verify sigqueue too?

Shall we update the kill test in tests/sys/capsicum/capmode.cc to verify sigqueue too?

Sorry, never mind, I missed that dcb_bound is set implicitly via the union.

My comments are just nits, I think the change is fine.

When all handles referencing a vnode are closed, VOP_INACTIVE (implemented by p9fs_inactive()) should be closed. Isn't that the right place to clunk the file? From what I can see, that's currently happening in p9fs_reclaim() which is too late as you say.

Actually this one doesn't matter too much since sysmouse_bufpoll isn't dynamically allocated.

I cannot apply the patch, it fails in pdfork.2. (I already applied the pdopenpid() patch.) Could you please rebase? Or, if your branch is already public, could you please include a pointer to it in the review description so that it's easier for me to fetch? That makes it easier for me to e.g., write syzkaller descriptions for new syscalls.

In D50614#1311340, @emaste wrote:

In my tree I hid the sysctl to find consumers and confirm that the system works without it. The consumers are:

• Rework validition to avoid multiplication.
• Fall back to the extended signature table if the processor flags don't match.

I don't really understand--if we were to remove TIOCSTI, wouldn't that be done by removing the runtime implementation, rather than hiding the TIOCSTI symbol like this?

In D57209#1310974, @jrm wrote:

Thanks both. Nice catch to fix the inverted condition. If I understand correctly, before this fix, we would never have detected an extended signature, and thus never applied microcode for processors only listed in the extended signature.

I don't really see the point. I'd just keep the option, having it silently do nothing, and remove the documentation.