| Message ID | 20240607060958.2789886-1-joshwash@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] gve: ignore nonrelevant GSO type bits when processing TSO headers | expand |
On Fri, Jun 7, 2024 at 8:10 AM <joshwash@google.com> wrote: > > From: Joshua Washington <joshwash@google.com> > > TSO currently fails when the skb's gso_type field has more than one bit > set. > > TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a > few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes > virtualization, such as QEMU, a real use-case. > > The gso_type and gso_size fields as passed from userspace in > virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type > |= SKB_GSO_DODGY to force the packet to enter the software GSO stack > for verification. > > This issue might similarly come up when the CWR bit is set in the TCP > header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit > to be set. > > Fixes: a57e5de476be ("gve: DQO: Add TX path") > Signed-off-by: Joshua Washington <joshwash@google.com> > Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com> > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com> > Reviewed-by: Willem de Bruijn <willemb@google.com> > Suggested-by: Eric Dumazet <edumazet@google.com> > Acked-by: Andrei Vagin <avagin@gmail.com> Reviewed-by: Eric Dumazet <edumazet@google.com>
joshwash@ wrote: > From: Joshua Washington <joshwash@google.com> > > TSO currently fails when the skb's gso_type field has more than one bit > set. > > TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a > few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes > virtualization, such as QEMU, a real use-case. > > The gso_type and gso_size fields as passed from userspace in > virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type > |= SKB_GSO_DODGY to force the packet to enter the software GSO stack > for verification. > > This issue might similarly come up when the CWR bit is set in the TCP > header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit > to be set. > > Fixes: a57e5de476be ("gve: DQO: Add TX path") > Signed-off-by: Joshua Washington <joshwash@google.com> > Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com> > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com> > Reviewed-by: Willem de Bruijn <willemb@google.com> > Suggested-by: Eric Dumazet <edumazet@google.com> > Acked-by: Andrei Vagin <avagin@gmail.com> I did not mean to ask for a revision. When you send a v2, please do include a changelog > --- > drivers/net/ethernet/google/gve/gve_tx_dqo.c | 21 +++++--------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c > index fe1b26a4d736..a76b407a981b 100644 > --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c > +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c > @@ -551,32 +551,21 @@ static int gve_prep_tso(struct sk_buff *skb) > * - Hypervisor enforces a limit of 9K MTU > * - Kernel will not produce a TSO larger than 64k > */ > - Accidental removal? > if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO)) > return -1; > > + if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) > + return -EINVAL; > +
diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c index fe1b26a4d736..a76b407a981b 100644 --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c @@ -551,32 +551,21 @@ static int gve_prep_tso(struct sk_buff *skb) * - Hypervisor enforces a limit of 9K MTU * - Kernel will not produce a TSO larger than 64k */ - if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO)) return -1; + if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) + return -EINVAL; + /* Needed because we will modify header. */ err = skb_cow_head(skb, 0); if (err < 0) return err; tcp = tcp_hdr(skb); - - /* Remove payload length from checksum. */ paylen = skb->len - skb_transport_offset(skb); - - switch (skb_shinfo(skb)->gso_type) { - case SKB_GSO_TCPV4: - case SKB_GSO_TCPV6: - csum_replace_by_diff(&tcp->check, - (__force __wsum)htonl(paylen)); - - /* Compute length of segmentation header. */ - header_len = skb_tcp_all_headers(skb); - break; - default: - return -EINVAL; - } + csum_replace_by_diff(&tcp->check, (__force __wsum)htonl(paylen)); + header_len = skb_tcp_all_headers(skb); if (unlikely(header_len > GVE_TX_MAX_HDR_SIZE_DQO)) return -EINVAL;