netdev - Re: [net-next v2 4/5] igc: Add Rx checksum support

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]

Message-ID: <20191018160507.025c0918@cakuba.netronome.com>
Date:   Fri, 18 Oct 2019 16:05:07 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     davem@...emloft.net, Sasha Neftin <sasha.neftin@...el.com>,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        Aaron Brown <aaron.f.brown@...el.com>
Subject: Re: [net-next v2 4/5] igc: Add Rx checksum support

On Fri, 18 Oct 2019 14:13:39 -0700, Jeff Kirsher wrote:
> From: Sasha Neftin <sasha.neftin@...el.com>
> 
> Extend the socket buffer field process and add Rx checksum functionality
> Minor: fix indentation with tab instead of spaces.
> 
> Signed-off-by: Sasha Neftin <sasha.neftin@...el.com>
> Tested-by: Aaron Brown <aaron.f.brown@...el.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h |  5 ++-
>  drivers/net/ethernet/intel/igc/igc_main.c    | 43 ++++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 03f1aca3f57f..f3788f0b95b4 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -282,7 +282,10 @@
>  #define IGC_RCTL_BAM		0x00008000 /* broadcast enable */
>  
>  /* Receive Descriptor bit definitions */
> -#define IGC_RXD_STAT_EOP	0x02    /* End of Packet */
> +#define IGC_RXD_STAT_EOP	0x02	/* End of Packet */
> +#define IGC_RXD_STAT_IXSM	0x04	/* Ignore checksum */
> +#define IGC_RXD_STAT_UDPCS	0x10	/* UDP xsum calculated */
> +#define IGC_RXD_STAT_TCPCS	0x20	/* TCP xsum calculated */
>  
>  #define IGC_RXDEXT_STATERR_CE		0x01000000
>  #define IGC_RXDEXT_STATERR_SE		0x02000000
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 7d2f479b57cf..c1aa2762dc87 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1201,6 +1201,46 @@ static netdev_tx_t igc_xmit_frame(struct sk_buff *skb,
>  	return igc_xmit_frame_ring(skb, igc_tx_queue_mapping(adapter, skb));
>  }
>  
> +static inline void igc_rx_checksum(struct igc_ring *ring,

Sorry about the piecemeal review, but we don't really like static
inlines in C files :(

It's called once it will definitely get inlined.

> +				   union igc_adv_rx_desc *rx_desc,
> +				   struct sk_buff *skb)
> +{
> +	skb_checksum_none_assert(skb);
> +
> +	/* Ignore Checksum bit is set */
> +	if (igc_test_staterr(rx_desc, IGC_RXD_STAT_IXSM))
> +		return;
> +
> +	/* Rx checksum disabled via ethtool */
> +	if (!(ring->netdev->features & NETIF_F_RXCSUM))
> +		return;
> +
> +	/* TCP/UDP checksum error bit is set */
> +	if (igc_test_staterr(rx_desc,
> +			     IGC_RXDEXT_STATERR_TCPE |
> +			     IGC_RXDEXT_STATERR_IPE)) {

consider unlikely() on the error case? not sure the performance matter
that much for IGC, up to you

> +		/* work around errata with sctp packets where the TCPE aka
> +		 * L4E bit is set incorrectly on 64 byte (60 byte w/o crc)
> +		 * packets, (aka let the stack check the crc32c)

Looks like there is a loose comma towards the end there

> +		 */
> +		if (!(skb->len == 60 &&
> +		      test_bit(IGC_RING_FLAG_RX_SCTP_CSUM, &ring->flags))) {
> +			u64_stats_update_begin(&ring->rx_syncp);
> +			ring->rx_stats.csum_err++;
> +			u64_stats_update_end(&ring->rx_syncp);
> +		}
> +		/* let the stack verify checksum errors */
> +		return;
> +	}
> +	/* It must be a TCP or UDP packet with a valid checksum */
> +	if (igc_test_staterr(rx_desc, IGC_RXD_STAT_TCPCS |
> +				      IGC_RXD_STAT_UDPCS))
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> +	dev_dbg(ring->dev, "cksum success: bits %08X\n",
> +		le32_to_cpu(rx_desc->wb.upper.status_error));

This could be replaced with a well placed kprobe, but up to you..

> +}
> +
>  static inline void igc_rx_hash(struct igc_ring *ring,
>  			       union igc_adv_rx_desc *rx_desc,
>  			       struct sk_buff *skb)
> @@ -1227,6 +1267,8 @@ static void igc_process_skb_fields(struct igc_ring *rx_ring,
>  {
>  	igc_rx_hash(rx_ring, rx_desc, skb);
>  
> +	igc_rx_checksum(rx_ring, rx_desc, skb);
> +
>  	skb_record_rx_queue(skb, rx_ring->queue_index);
>  
>  	skb->protocol = eth_type_trans(skb, rx_ring->netdev);
> @@ -4391,6 +4433,7 @@ static int igc_probe(struct pci_dev *pdev,
>  		goto err_sw_init;
>  
>  	/* Add supported features to the features list*/
> +	netdev->features |= NETIF_F_RXCSUM;
>  	netdev->features |= NETIF_F_HW_CSUM;
>  	netdev->features |= NETIF_F_SCTP_CRC;