From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 10751A0C49; Tue, 15 Jun 2021 09:57:37 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5B128410F7; Tue, 15 Jun 2021 09:57:36 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id C47004067A for ; Tue, 15 Jun 2021 09:57:34 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623743854; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=bYaMT9RF61P8rL1jMBy/P1alqSMElQsNgFyWKnLPufU=; b=J5pzG3A5+jv38KhFL5JdW3vuwPZKe/xohMtlKnEW4KEAmyOmS3rzkbsvP/F9WZwrCK7cmj X5VJM+mOXgWu8tkVUj6UP8X6FIj5qTHuoVLAI7KjBJFsCT2xvXlvz9vXpBXbEz58Mbvlbb id8CcmNbLrvS458dmtV5ZbvGYuomtJU= Received: from mail-vk1-f197.google.com (mail-vk1-f197.google.com [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-10-ybnkBWrdP1G9IU8K_8RWpQ-1; Tue, 15 Jun 2021 03:57:33 -0400 X-MC-Unique: ybnkBWrdP1G9IU8K_8RWpQ-1 Received: by mail-vk1-f197.google.com with SMTP id g198-20020a1f20cf0000b029023d152b0cdeso2386906vkg.19 for ; Tue, 15 Jun 2021 00:57:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bYaMT9RF61P8rL1jMBy/P1alqSMElQsNgFyWKnLPufU=; b=l/l6q0NprDC7xdRrmLPE/WPK2C/T16lfIljciDDPSSuiiqzHvzITTvvbLeaCI/v1mM SMMfz7wC4AxlCYi17R4F69kTrjfno+qHlfZehz2iBYBxUzEqGb0VEDRacemAmWXNxPaB gAS3/2UERCz18UuHvZEZ0raEXoAcPnuZmBjoTFeMqOA+rp91mdnPdEFobcLXM5CFuHm0 xVqV255l1uROBi310YdGm2kPS5z5+xnMuDbVc9zIZac+FyyTb0CAHyugfzVnTowEF7eh t3sPRqV1BsCwbOW4T7a9gJObm3tJAzBNZCVWsd4nZrCTHv1rQD2ZJTRTDSdR1SOTVfxg 1+SQ== X-Gm-Message-State: AOAM533CaS0m1soe4KNMw5m2IxT58QDkrBcdCGmm+2Qi8b4MXZL6uNK0 2D9EQ+ip2rkM+VV2Gvg7L8yvr904ZDzI8exH3oxqm5qb7Pt/4bKoY9jpb6z3SbDOwEIrqQXRPUf FnGajbgh9z9WePNKcHe4= X-Received: by 2002:a05:6102:67a:: with SMTP id z26mr877102vsf.10.1623743852088; Tue, 15 Jun 2021 00:57:32 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwNY+Z7A8dm0E398QENTR59f/Q+x6fWwawMIXXwRISBgvEV2Wf09/Zu8alLkb8eGw2UqzcN2pvvKfWe9ZGP6b0= X-Received: by 2002:a05:6102:67a:: with SMTP id z26mr877092vsf.10.1623743851802; Tue, 15 Jun 2021 00:57:31 -0700 (PDT) MIME-Version: 1.0 References: <20210317063109.135662-1-xiao.w.wang@intel.com> <20210615063507.18198-1-xiao.w.wang@intel.com> In-Reply-To: <20210615063507.18198-1-xiao.w.wang@intel.com> From: David Marchand Date: Tue, 15 Jun 2021 09:57:20 +0200 Message-ID: To: Xiao Wang Cc: Maxime Coquelin , "Xia, Chenbo" , Cheng Jiang , dev , dpdk stable Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v4] vhost: check header for legacy dequeue offload X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jun 15, 2021 at 9:06 AM Xiao Wang wrote: > diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c > index 8da8a86a10..351ff0a841 100644 > --- a/lib/vhost/virtio_net.c > +++ b/lib/vhost/virtio_net.c > @@ -2259,44 +2259,64 @@ virtio_net_with_host_offload(struct virtio_net *dev) > return false; > } > > -static void > -parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr) > +static int > +parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr, > + uint16_t *len) > { This function name is misleading, name could be parse_headers(). Its semantic gets more and more confusing with those l4_hdr and len pointers. This function fills ->lX_len in the mbuf, everything is available for caller. Caller can check that rte_pktmbuf_data_len() is >= m->l2_len + m->l3_len + somesize. => no need for len. l4_hdr can simply be deduced with rte_pktmbuf_mtod_offset(m, struct somestruct *, m->l2_len + m->l3_len). => no need for l4_hdr. > struct rte_ipv4_hdr *ipv4_hdr; > struct rte_ipv6_hdr *ipv6_hdr; > void *l3_hdr = NULL; No need for l3_hdr. > struct rte_ether_hdr *eth_hdr; > uint16_t ethertype; > + uint16_t data_len = m->data_len; Avoid direct access to mbuf internals, we have inline helpers: rte_pktmbuf_data_len(m). > + > + if (data_len <= sizeof(struct rte_ether_hdr)) Strictly speaking, < is enough. > + return -EINVAL; > > eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *); > > m->l2_len = sizeof(struct rte_ether_hdr); > ethertype = rte_be_to_cpu_16(eth_hdr->ether_type); > + data_len -= sizeof(struct rte_ether_hdr); No need to decrement data_len if checks below are all done for absolute value. See suggestions below. > > if (ethertype == RTE_ETHER_TYPE_VLAN) { > + if (data_len <= sizeof(struct rte_vlan_hdr)) > + return -EINVAL; if (data_len < sizeof(rte_ether_hdr) + sizeof(struct rte_vlan_hdr)) > + > struct rte_vlan_hdr *vlan_hdr = > (struct rte_vlan_hdr *)(eth_hdr + 1); > > m->l2_len += sizeof(struct rte_vlan_hdr); > ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto); > + data_len -= sizeof(struct rte_vlan_hdr); Idem. > } > > l3_hdr = (char *)eth_hdr + m->l2_len; > > switch (ethertype) { > case RTE_ETHER_TYPE_IPV4: > + if (data_len <= sizeof(struct rte_ipv4_hdr)) > + return -EINVAL; if (data_len < m->l2_len + sizeof(struct rte_ipv4_hdr)) > ipv4_hdr = l3_hdr; ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *, m->l2_len); > *l4_proto = ipv4_hdr->next_proto_id; > m->l3_len = rte_ipv4_hdr_len(ipv4_hdr); > + if (data_len <= m->l3_len) { if (data_len < m->l2_len + m->l3_len) > + m->l3_len = 0; > + return -EINVAL; Returning here leaves m->l2_len set. > + } > *l4_hdr = (char *)l3_hdr + m->l3_len; > m->ol_flags |= PKT_TX_IPV4; > + data_len -= m->l3_len; > break; > case RTE_ETHER_TYPE_IPV6: > + if (data_len <= sizeof(struct rte_ipv6_hdr)) > + return -EINVAL; if (data_len < m->l2_len + sizeof(struct rte_ipv6_hdr)) Returning here leaves m->l2_len set. > ipv6_hdr = l3_hdr; ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv6_hdr *, m->l2_len); > *l4_proto = ipv6_hdr->proto; > m->l3_len = sizeof(struct rte_ipv6_hdr); > *l4_hdr = (char *)l3_hdr + m->l3_len; > m->ol_flags |= PKT_TX_IPV6; > + data_len -= m->l3_len; > break; > default: > m->l3_len = 0; > @@ -2304,6 +2324,9 @@ parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr) > *l4_hdr = NULL; > break; > } > + > + *len = data_len; > + return 0; > } > > static __rte_always_inline void > @@ -2312,21 +2335,27 @@ vhost_dequeue_offload_legacy(struct virtio_net_hdr *hdr, struct rte_mbuf *m) > uint16_t l4_proto = 0; > void *l4_hdr = NULL; > struct rte_tcp_hdr *tcp_hdr = NULL; > + uint16_t len = 0, tcp_len; > + > + if (parse_ethernet(m, &l4_proto, &l4_hdr, &len) < 0) > + return; > > - parse_ethernet(m, &l4_proto, &l4_hdr); > if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) { > if (hdr->csum_start == (m->l2_len + m->l3_len)) { > switch (hdr->csum_offset) { > case (offsetof(struct rte_tcp_hdr, cksum)): > - if (l4_proto == IPPROTO_TCP) > + if (l4_proto == IPPROTO_TCP && > + len >= sizeof(struct rte_tcp_hdr)) if (rte_pktmbuf_data_len(m) >= m->l2_len + m->l3_len + sizeof(struct rte_tcp_hdr)) Then, if this check is wrong, we leave l2_len, l3_len + PKT_TX_IPVx flag set in mbuf. These two comments apply to other updates below. > m->ol_flags |= PKT_TX_TCP_CKSUM; > break; > case (offsetof(struct rte_udp_hdr, dgram_cksum)): > - if (l4_proto == IPPROTO_UDP) > + if (l4_proto == IPPROTO_UDP && > + len >= sizeof(struct rte_udp_hdr)) > m->ol_flags |= PKT_TX_UDP_CKSUM; > break; > case (offsetof(struct rte_sctp_hdr, cksum)): > - if (l4_proto == IPPROTO_SCTP) > + if (l4_proto == IPPROTO_SCTP && > + len >= sizeof(struct rte_sctp_hdr)) > m->ol_flags |= PKT_TX_SCTP_CKSUM; > break; > default: > @@ -2339,12 +2368,21 @@ vhost_dequeue_offload_legacy(struct virtio_net_hdr *hdr, struct rte_mbuf *m) > switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) { > case VIRTIO_NET_HDR_GSO_TCPV4: > case VIRTIO_NET_HDR_GSO_TCPV6: > + if (l4_proto != IPPROTO_TCP || > + len < sizeof(struct rte_tcp_hdr)) > + break; > tcp_hdr = l4_hdr; tcp_hdr = rte_pktmbuf_mtod_offset(m, struct rte_tcp_hdr *, m->l2_len + m->l3_len); > + tcp_len = (tcp_hdr->data_off & 0xf0) >> 2; > + if (len < tcp_len) > + break; > m->ol_flags |= PKT_TX_TCP_SEG; > m->tso_segsz = hdr->gso_size; > - m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2; > + m->l4_len = tcp_len; > break; > case VIRTIO_NET_HDR_GSO_UDP: > + if (l4_proto != IPPROTO_UDP || > + len < sizeof(struct rte_udp_hdr)) > + break; > m->ol_flags |= PKT_TX_UDP_SEG; > m->tso_segsz = hdr->gso_size; > m->l4_len = sizeof(struct rte_udp_hdr); > -- > 2.15.1 > -- David Marchand