netdev - RE: [net-next v3 20/29] fm10k: Add support for netdev offloads

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 1 Oct 2014 12:22:18 +0000
From:	Sathya Perla <Sathya.Perla@...lex.Com>
To:	Alexander Duyck <alexander.h.duyck@...el.com>,
	Or Gerlitz <gerlitz.or@...il.com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
CC:	David Miller <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	"jogreene@...hat.com" <jogreene@...hat.com>,
	Tom Herbert <therbert@...gle.com>
Subject: RE: [net-next v3 20/29] fm10k: Add support for netdev offloads


> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-
> 
> On 10/01/2014 01:31 AM, Or Gerlitz wrote:
> > On Tue, Sep 23, 2014 at 2:16 PM, Jeff Kirsher
> > <jeffrey.t.kirsher@...el.com> wrote:
> >> From: Alexander Duyck <alexander.h.duyck@...el.com>
> >> This patch adds support for basic offloads including TSO, Tx checksum, Rx
> >> checksum, Rx hash, and the same features applied to VXLAN/NVGRE
> tunnels.
> >
> >> --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> >> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > [...]
> >> +#define VXLAN_HLEN (sizeof(struct udphdr) + 8)
> >> +static struct ethhdr *fm10k_port_is_vxlan(struct sk_buff *skb)
> >> +{
> >> +       struct fm10k_intfc *interface = netdev_priv(skb->dev);
> >> +       struct fm10k_vxlan_port *vxlan_port;
> >> +
> >> +       /* we can only offload a vxlan if we recognize it as such */
> >> +       vxlan_port = list_first_entry_or_null(&interface->vxlan_port,
> >> +                                             struct fm10k_vxlan_port, list);
> >> +
> >> +       if (!vxlan_port)
> >> +               return NULL;
> >> +       if (vxlan_port->port != udp_hdr(skb)->dest)
> >> +               return NULL;
> >> +
> >> +       /* return offset of udp_hdr plus 8 bytes for VXLAN header */
> >> +       return (struct ethhdr *)(skb_transport_header(skb) + VXLAN_HLEN);
> >> +}
> >> +
...
> 
> >> +static int fm10k_tso(struct fm10k_ring *tx_ring,
> >> +                    struct fm10k_tx_buffer *first)
> >> +{
> >> +       struct sk_buff *skb = first->skb;
> >> +       struct fm10k_tx_desc *tx_desc;
> >> +       unsigned char *th;
> >> +       u8 hdrlen;
> >> +
> >> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> +               return 0;
> >> +
> >> +       if (!skb_is_gso(skb))
> >> +               return 0;
> >> +
> >> +       /* compute header lengths */
> >> +       if (skb->encapsulation) {
> >> +               if (!fm10k_tx_encap_offload(skb))
> >> +                       goto err_vxlan;
> >> +               th = skb_inner_transport_header(skb);
> >> +       } else {
> >> +               th = skb_transport_header(skb);
> >> +       }
> >> +
> >> +       /* compute offset from SOF to transport header and add header len
> */
> >> +       hdrlen = (th - skb->data) + (((struct tcphdr *)th)->doff << 2);
> >> +
> >> +       first->tx_flags |= FM10K_TX_FLAGS_CSUM;
> >> +
> >> +       /* update gso size and bytecount with header size */
> >> +       first->gso_segs = skb_shinfo(skb)->gso_segs;
> >> +       first->bytecount += (first->gso_segs - 1) * hdrlen;
> >> +
> >> +       /* populate Tx descriptor header size and mss */
> >> +       tx_desc = FM10K_TX_DESC(tx_ring, tx_ring->next_to_use);
> >> +       tx_desc->hdrlen = hdrlen;
> >> +       tx_desc->mss = cpu_to_le16(skb_shinfo(skb)->gso_size);
> >> +
> >> +       return 1;
> >> +err_vxlan:
> >> +       tx_ring->netdev->features &= ~NETIF_F_GSO_UDP_TUNNEL;
> >> +       if (!net_ratelimit())
> >> +               netdev_err(tx_ring->netdev,
> >> +                          "TSO requested for unsupported tunnel, disabling
> offload\n");
> >> +       return -1;
> >> +}
> >
> > why? if TSO was requested for some packet the driver can't do, you disable
> GSO
> > for udp tunnels for any future packets too? maybe just disable it
> > permanently  till you
> > feel safer to run under the current stack?
> 
> That is essentially what I am doing.  If the user wants they can turn
> the feature back on via ethtool, but there is no point in having it on
> if the device itself cannot support the packets that are coming through.
> 

To turn off the offload, shouldn't you be using netdev_update_features()
(->ndo_fix_features()) instead of directly flipping bits in netdev->features?