vmbus_server: preserve proxy pipe offer data - #4208
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes loss of proxy-provided named-pipe offer payload data when vmbus_server relays offers from vmbusproxy. It keeps the semantic ChannelType::Pipe classification (byte vs message mode) while ensuring the full provider-owned UserDefinedData is preserved across the OfferParams -> OfferParamsInternal conversion boundary.
Changes:
- Convert the proxy offer to
OfferParamsInternalonce, then overrideuser_definedforChannelType::Pipewith the original proxy-providedUserDefinedpayload. - Keep all other channel kinds and offer flag behavior unchanged.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Rather than working around this in proxyintegration, I think we should update ChannelType::Pipe to support these flags and handle them in the conversion to OfferParamsInternal so we can cleanly reconstruct user_data from the "native" OpenVMM structures.
Since I think it is possible for a pipe to include custom other offer data (though I'm not sure if that's actually used right now), ChannelType::Pipe also should allow specifying the remaining 112 bytes of custom data.
dc753cd to
951e789
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
vm/devices/vmbus/vmbus_server/src/proxyintegration.rs:576
- The PR description says proxy-originated pipe offers should preserve the complete original
UserDefinedData, but this change only extracts and re-encodesPipeFlags(last 4 bytes).OfferParamsInternal::fromstill starts fromUserDefinedData::new_zeroed()and only writespipe_type+pipe_flags, so any provider-defined bytes in the rest of the 120-byte payload are still discarded.
ChannelType::Pipe {
message_mode,
pipe_flags: *offer.UserDefined.as_pipe_flags(),
}
951e789 to
39bf621
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
vm/devices/vmbus/vmbus_core/src/protocol.rs:596
PipeFlagsis interpreted from the trailing bytes ofUserDefinedData(viaUserDefinedData::as_pipe_flags*), but that encoding detail isn’t documented here. Adding a short doc comment onPipeFlagswill make the implicit layout contract easier to discover and less likely to be broken by future changes.
#[derive(Inspect)]
#[bitfield(u32)]
#[derive(IntoBytes, FromBytes, Immutable, KnownLayout, PartialEq, Eq, Protobuf)]
#[mesh(transparent)]
pub struct PipeFlags {
| pub fn as_pipe_flags(&self) -> &PipeFlags { | ||
| let offset = self.0.len() - size_of::<PipeFlags>(); | ||
| PipeFlags::ref_from_bytes(&self.0[offset..]).expect("from bytes should not fail") | ||
| } | ||
|
|
||
| pub fn as_pipe_flags_mut(&mut self) -> &mut PipeFlags { | ||
| let offset = self.0.len() - size_of::<PipeFlags>(); | ||
| PipeFlags::mut_from_bytes(&mut self.0[offset..]).expect("from bytes should not fail") | ||
| } | ||
|
|
There was a problem hiding this comment.
I think instead of these functions, it would be better to incorporate the flags in PipeUserDefinedParameters (along with the 112-byte padding in between).
39bf621 to
06f40d6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
vm/devices/vmbus/vmbus_channel/src/bus.rs:331
ChannelType::Pipenow requires a full 112-byteuser_definedpayload pluspipe_flags, even though the PR description intends the passthrough to be limited to proxy-originated offers. This forces all native pipe providers to populate placeholder defaults (as seen in the updated call sites) and makes it easier for non-proxy providers to accidentally start relying on non-canonical pipe payloads.
Consider keeping the public ChannelType::Pipe API focused on the semantic classification (message_mode) and moving the proxy-only passthrough into an optional wrapper (e.g. pipe_offer: Option<PipeOfferData>), or a separate enum variant used only by the proxy integration.
message_mode: bool,
/// Provider-defined offer data.
user_defined: PipeUserDefinedData,
/// Pipe capabilities.
pipe_flags: PipeFlags,
vm/devices/vmbus/vmbus_core/src/protocol.rs:624
UserDefinedData::as_pipe_params(_mut)usesref_from_bytes(..).expect("from bytes should not fail"), but the newPipeUserDefinedParameterslayout only has a size assert. Adding an alignment assert would make this invariant explicit and avoid a future runtime panic if the struct’s alignment ever changes.
static_assertions::const_assert_eq!(
size_of::<PipeUserDefinedParameters>(),
size_of::<UserDefinedData>()
);
1868394
into
microsoft:main
Problem
When relaying a named-pipe channel offer from
vmbusproxy,vmbus_serverconverts the offer toChannelType::Pipe. That type retains only whether the pipe uses byte or message mode.Converting the resulting
OfferParamstoOfferParamsInternaltherefore reconstructsUserDefinedDatafrom zeroes and preserves only the pipe mode. Any additional pipe-specific offer data supplied by the channel provider is lost.Fix
For proxy-originated pipe channels, preserve the complete original
UserDefinedDataafter performing the normal typed conversion and pipe-mode validation.This keeps the existing semantic channel classification while ensuring the provider-owned offer payload is relayed unchanged.
The override is intentionally limited to this boundary:
This is implemented in the proxy integration rather than
From<OfferParams>becauseOfferParams::Pipeno longer contains the original payload, and the generic conversion is also used by native channel providers.The change was validated with a downlevel Windows guest scenario that previously failed during boot.