Hi Peter, On Tue, Jul 21, 2026 at 12:22 AM Peter Maydell <[email protected]> wrote: > > On Mon, 29 Jun 2026 at 17:31, Peter Maydell <[email protected]> wrote: > > I ran into this from a different angle today, and I think > > qemu_receive_packet()'s check is wrong currently, not just > > with this change. Although net_peer_needs_padding() will > > almost always return "true" today, if the network device > > has no peer (e.g. run with -net none or -nodefaults and no > > other network backend connected to it) then we won't pad > > packets we feed in for loopback, and so the bugs which > > commit a01344d9d78 is trying to fix still occur. I'll send > > a patch to make that change once I've tested it a bit. > > > > I'm also looking at a variant of this involving vlan headers: > > https://gitlab.com/qemu-project/qemu/-/work_items/3518 > > > > In commit 63b901bfd30 you removed the code from rtl8139.c > > which handled padding out short frames in the device's > > receive path, because we now do the padding at the other end. > > However, in this case the padding code in the device was > > also handling a case where there's an 802.1Q vlan tag: it makes > > the buffer at least MIN_BUF_SIZE + VLAN_HLEN == 64 bytes, with > > the comment "Include some tailroom in case a vlan tag is > > later removed". Then later we use this to avoid the copy > > from the buffer reading off the end, but now the copy can > > read 4 bytes of data off the end of the packet into guest > > memory. > > > > How should we handle this? We could: > > - reinstate the rtl8139 padding code > > - make the generic padding code allow enough extra space > > for the 802.1Q vlan tag (so devices can assume that the > > frame is at least its minimum size even if there's a > > vlan tag) > > - have rtl8139 special case this > > Ping? How do you think we should handle the "short packet > with a vlan header" case? I think I lean towards making > the generic code deal with it. > > I'm also still very unsure whether our network layer is > intended to distinguish "this is a short packet that we > padded" from "this is a packet that the guest padded". > Is a network driver/card model supposed to be able to > opt in to getting unpadded packets and then identify and > diagnose-as-error incoming short packets if it wants to? > That would mean we need to distinguish "we padded this > because the guest sent a short packet and we don't want > to deal with that in loopback or otherwise" from "we > padded this because it comes from a source that hands > us legitimate short packets that would have been padded > if they'd ever gone through a real network device". >
Sorry for the delayed reply. I went back through the original change and the current rtl8139 path. I think the source/destination distinction you pointed out is real, but the current net API does not encode it explicitly. The intent of 969e50b61a285 was that a short packet supplied by a normal backend is a valid Ethernet packet whose zero padding has been omitted. Before delivering it to a physical NIC model, QEMU materializes that padding. A packet transmitted by a NIC model, on the other hand, is the actual result of that model's TX path and should reach the backend unchanged. The original commit explicitly accepted the consequence that slirp/tap could no longer be used to inject a real runt frame into a NIC model. So I don't think do_not_pad should also be interpreted as "this physical NIC can receive and diagnose real runts". That is a different property. virtio-net uses do_not_pad because it accepts a length-delimited logical packet, not because it is modelling runt-frame reception. For the current cleanup, I think centralizing the padding as in your draft series is still the right direction, provided we document that contract. If we later want to support injecting genuine runt frames, we will need a separate per-packet indication for "padding omitted" versus "exact wire length". That indication would need to survive queues, filters and hubs; a source-side boolean alone would not be sufficient. For the VLAN case, after looking at it more closely, I agree that the generic padding code should handle it. A single-tagged Ethernet frame has a minimum length of ETH_ZLEN + VLAN_HLEN == 64 bytes without FCS, so padding it to 64 is not merely providing private tailroom for rtl8139. Ideally the helper could derive the minimum from the L2 header length, which would also cover double-tagged frames. There is also an independent rtl8139 loopback bug in rtl8139_transfer_frame(). When dot1q_buf is present, it allocates and fills buf2 using iov_size(iov, 3), which includes the inserted VLAN tag, but calls qemu_receive_packet() with the original size. We need to pass buf2_size there as well. Otherwise the padding helper will copy four bytes less than the buffer actually contains. With both changes, the loopback RX path receives a 64-byte tagged frame, and VLAN stripping leaves the expected 60-byte frame without relying on an out-of-bounds read. Regards, Bin
