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] [day] [month] [year] [list]
Date:	Mon, 6 Oct 2014 06:19:43 +0000
From:	Sathya Perla <Sathya.Perla@...lex.Com>
To:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>,
	Or Gerlitz <gerlitz.or@...il.com>,
	"Kirsher, Jeffrey T" <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-
> owner@...r.kernel.org] On Behalf Of Duyck, Alexander H
...
> 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?
> 
> Why?  That would just be me calling my own ndo operation wouldn't it, since
> I am changing the features of the device covered by this driver.

I guess it would be to ensure that all dependencies are resolved and a 
FEAT_CHANGE notification is issued.

Documentation/networking/netdev-features.txt says the following:
" When current feature set (netdev->features) is to be changed, new set
is calculated and filtered by calling ndo_fix_features callback
and netdev_fix_features()."

" A driver that wants to trigger recalculation must do so by calling
netdev_update_features() while holding rtnl_lock. This should not be done
from ndo_*_features callbacks. netdev->features should not be modified by
driver except by means of ndo_fix_features callback."