On Tue, Jul 23, 2024 at 3:31 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > The commit under fixes added a questionable check to > virtio_net_hdr_to_skb(). I'm guessing the check was supposed > to protect from csum offset being outside of a segment > (especially if len is not multiple of segment size). > > The condition can't be right, tho, as it breaks previously > working sending of GSO frames with only one segment > (i.e. when gso_size <= len we silently ignore the GSO > request and send a single non-GSO frame). > > Fix the logic and move it to the GSO part. I missed the previous patch. Should we revert that and create a new fix against the original issue? Normally the checksum start + offset should always be in the header, so not even part of gso_size. So needed need not be related to gso_size. The exception to this is UDP fragmentation offload, I suppose. As there the network and transport headers are part of the UFO payload. But even for the normal TSO and USO cases we cannot verify in virtio_net_hdr_to_skb that the csum_start + csum_off passed from userspace are really pointing into the transport header. For SKB_GSO_UDP_L4 I added a minimal check that csum_off must be offsetof(struct udphdr, check). We can arguably tighten these csum_off for all requests, as only UDP and TCP offsets are valid. But no such simple check exists for csum_start. This requires full packet parsing, which we don't do until skb_gso_segment. One option may be to test csum_start in tcp_gso_segment and udp_gso_fragment and fail segmentation when it points not where expected. Btw, do we have a better idea what exact packet triggered this WARN_ON_ONCE in skb_checksum_help? Usually, more interesting than the skb_dump of the segment that reached the WARN is the skb_dump at the time of virtio_net_hdr_to_skb, along with the vnet_hdr. > This has been caught by net/tap and net/psock_send.sh tests. That's very nice! > Fixes: e269d79c7d35 ("net: missing check virtio") > Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx> > + if (csum_needed) { > + unsigned int p_rem, p_size; > + > + p_size = gso_size; > + p_rem = (skb->len - nh_off) % gso_size; > + if (p_rem) > + p_size = p_rem; > + > + /* Make sure csum still within packet after GSO */ > + if (p_size + nh_off < csum_needed) > + return -EINVAL; > + } > + A check could even go in the below branch. The warning apparently was not that csum_needed is outside the segment entirely, but that the segment is non-linear and csum_start points in the non-linear part (offset >= skb_headlen(skb)). I don't think we should be playing SKBFL_SHARED_FRAG tricks to trigger linearization, to be clear. We also cannot just silence the WARN and trust that the stack detects these bad packets and drops them (as ip_do_fragment does), as they might end up not in ip_do_fragment, but in a device ndo_start_xmit. > /* Too small packets are not really GSO ones. */ > if (skb->len - nh_off > gso_size) { > shinfo->gso_size = gso_size; > diff --git a/tools/testing/selftests/net/tap.c b/tools/testing/selftests/net/tap.c > index 247c3b3ac1c9..8527d51449cf 100644 > --- a/tools/testing/selftests/net/tap.c > +++ b/tools/testing/selftests/net/tap.c > @@ -418,6 +418,36 @@ TEST_F(tap, test_packet_valid_udp_csum) > ASSERT_EQ(ret, off); > } > > +TEST_F(tap, test_packet_invalid_udp_gso_csum) > +{ > + uint8_t pkt[TEST_PACKET_SZ]; > + uint16_t payload; > + size_t off; > + int ret; > + int i; > + > + payload = ETH_DATA_LEN - sizeof(struct iphdr) - sizeof(struct udphdr); > + > + memset(pkt, 0, sizeof(pkt)); > + off = build_test_packet_valid_udp_gso(pkt, payload); > + > + for (i = -16; i < 16; i++) { > + ret = write(self->fd, pkt, off + i); > + > + if (i <= 0 || > + i > __builtin_offsetof(struct udphdr, check) + 1) { > + EXPECT_EQ(ret, off + i) > + TH_LOG("mismatch with offset: %d (%zd)", > + i, off + i); > + } else { > + EXPECT_EQ(ret, -1) > + TH_LOG("mismatch with offset: %d (%zd)", > + i, off + i); > + EXPECT_EQ(errno, 22); > + } > + } > +} > + > TEST_F(tap, test_packet_crash_tap_invalid_eth_proto) > { > uint8_t pkt[TEST_PACKET_SZ]; > -- > 2.45.2 >