[pci] guard set_features+handle mask drift in legacy & common interfaces

[zeeshanlakhani]

This work fixes a known issue where legacy interface writes could arrive after feature negotiation ended or a DRIVER_OK flag was set, causing side-effects and unwanted reconfiguration.

We fix this with a guard before set_features that checks that negotiation has closed first, setting NEEDS_RESET (discarding the write) if a different mask is requested. A write that produces the same mask is just a no-op here.

For modern, common interfaces, writes after FEATURES_OK could mutate the state's negotiated_features without calling set_features itself, causing the mask to diverge from what the device had already applied. This came up in iximeow's comment, which proposed NEEDS_RESET for feature writes between FEATURES_OK and DRIVER_OK.

To handle this, we apply a similar methodology, but for CommonConfigReg::DriverFeature once negotiation is closed. We also reject retried FEATURES_OK flag sets while NEEDS_RESET status is already set.

We need these changes for upcoming viona work involving setting MAC filters.

[iximeow]

I closed #1053 because I proposed it not knowing the mechanism by which it fixed the issue I was observing, and I later determined that the issue was different and motivated a different fix (the fourth paragraph in #1053) - I did not close #1053 leaving a known issue in the tree, beyond that a guest with a driver that is explicitly misbehaving against modern registers can cause a bit worse misbehavior than we'd like.

the issue here is more subtle, in that I'd overlooked that writes to the legacy features register went directly to set_features too, and that the lack of a FEATURES_OK in legacy negotiation meant that worked kind of by accident since we even started supporting modern devices. legacy drivers would probably write a feature set once, get the correct number of set_features() calls as a result, and then continue with negotiation. it was really not brought along with the FSM I'd hoped to set up in #1064.

so there are kind of two bugs here: the device retains writes to state.negotiated_features after FEATURES_OK which, yes, is pretty wonky. for example writing F_CTRL_RX into the features after FEATURES_OK will mean that migrating such a device will have the destination's device operate with a different feature set than was actually initially set up. weird, not ideal, and all not in line with the VirtIO spec. additionally, writes to LegacyConfigReg::DriverFeature after the first can cause set_features() to run multiple times on an initialized device, which is also, I'd say, "pretty wonky".

what I'm confused about is: how does this relate to #1210? I don't see a way that that PR should have issues or not have issues based on this being in the Propolis, and I don't see how this could lead to reapplying set_features() on a device that was already configured. this is all under a pretty clear MUST NOT from the spec (what I was gesturing at in the comment you linked to), is there a guest that is setting additional features after negotiation, which we need to tolerate?

I have looked at the diff itself and I think it's probably not how we'd want to go about fixing these issues, but I think it's more important to understand what issues we have or will run into than getting into the change itself.

[zeeshanlakhani]

I closed #1053 because I proposed it not knowing the mechanism by which it fixed the issue I was observing, and I later determined that the issue was different and motivated a different fix (the fourth paragraph in #1053) - I did not close #1053 leaving a known issue in the tree, beyond that a guest with a driver that is explicitly misbehaving against modern registers can cause a bit worse misbehavior than we'd like.

the issue here is more subtle, in that I'd overlooked that writes to the legacy features register went directly to set_features too, and that the lack of a FEATURES_OK in legacy negotiation meant that worked kind of by accident since we even started supporting modern devices. legacy drivers would probably write a feature set once, get the correct number of set_features() calls as a result, and then continue with negotiation. it was really not brought along with the FSM I'd hoped to set up in #1064.

so there are kind of two bugs here: the device retains writes to state.negotiated_features after FEATURES_OK which, yes, is pretty wonky. for example writing F_CTRL_RX into the features after FEATURES_OK will mean that migrating such a device will have the destination's device operate with a different feature set than was actually initially set up. weird, not ideal, and all not in line with the VirtIO spec. additionally, writes to LegacyConfigReg::DriverFeature after the first can cause set_features() to run multiple times on an initialized device, which is also, I'd say, "pretty wonky".

what I'm confused about is: how does this relate to #1210? I don't see a way that that PR should have issues or not have issues based on this being in the Propolis, and I don't see how this could lead to reapplying set_features() on a device that was already configured. this is all under a pretty clear MUST NOT from the spec (what I was gesturing at in the comment you linked to), is there a guest that is setting additional features after negotiation, which we need to tolerate?

I have looked at the diff itself and I think it's probably not how we'd want to go about fixing these issues, but I think it's more important to understand what issues we have or will run into than getting into the change itself.

Thanks for the walkthrough on #1053. This came about due to its seemingly possible connection with the legacy feature-write path and clearing guest Rx state that includes the multicast MAC filter table in #1210. In #1210, set_features() clears guest RX state like so:

fn set_features(&self, feat: u64) -> Result<(), ()> {
    self.hdl.set_features(feat).map_err(|_| ())?;

    // Clear guest Rx state after applying the new feature set.
    //
    // Legacy devices can also update features without passing through the
    // modern (device) FEATURES_OK transition.
    {
        let mut state = self.inner.lock().unwrap();
        self.clear_guest_rx_state(&mut state).map_err(|_| ())?;
    }

...

fn clear_guest_rx_state(
    &self,
    state: &mut Inner,
) -> Result<(), RxConfigError> {
    state.filter = FilterState::empty();
    state.unicast_mac_filters = Box::new([]);
    state.multicast_mac_filters = Box::new([]);
    state.mac_table_set = false;
    self.apply_rx_config(state, RxReconcileCause::Reinitialize)

which includes the multicast table (now). My worry was that a late legacy feature write could re-enter the new RX cleanup path (less so worried about the modern/common mask drifting). But, this is more me being defensive vs finding the reproducer, but I had been exploring it late last week, so I took a chance to see what may be possible.