| 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
| ||
|
Date: Wed, 10 May 2023 19:43:36 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Mengyuan Lou <mengyuanlou@...-swift.com>, <netdev@...r.kernel.org>
CC: <jiawenwu@...stnetic.com>
Subject: Re: [PATCH net-next v4 2/7] net: wangxun: libwx add rx offload
functions
On 2023/5/10 17:38, Mengyuan Lou wrote:
...
> +/**
> + * wx_rx_checksum - indicate in skb if hw indicated a good cksum
> + * @ring: structure containing ring specific data
> + * @rx_desc: current Rx descriptor being processed
> + * @skb: skb currently being received and modified
> + **/
> +static void wx_rx_checksum(struct wx_ring *ring,
> + union wx_rx_desc *rx_desc,
> + struct sk_buff *skb)
> +{
> + struct wx_dec_ptype dptype = wx_decode_ptype(WX_RXD_PKTTYPE(rx_desc));
> +
> + skb->ip_summed = CHECKSUM_NONE;
> + skb_checksum_none_assert(skb);
It does not make much to check skb->ip_summed when it is just
set one line above.
Also the "skb->ip_summed = CHECKSUM_NONE" seems unnecessary,
as alloc/build_skb() all have the below to make sure
skb->ip_summed is zero:
memset(skb, 0, offsetofstruct sk_buff, tail))
> + /* Rx csum disabled */
> + if (!(ring->netdev->features & NETIF_F_RXCSUM))
> + return;
> +
> + /* if IPv4 header checksum error */
> + if ((wx_test_staterr(rx_desc, WX_RXD_STAT_IPCS) &&
> + wx_test_staterr(rx_desc, WX_RXD_ERR_IPE)) ||
> + (wx_test_staterr(rx_desc, WX_RXD_STAT_OUTERIPCS) &&
> + wx_test_staterr(rx_desc, WX_RXD_ERR_OUTERIPER))) {
> + ring->rx_stats.csum_err++;
> + return;
> + }
> +
> + /* L4 checksum offload flag must set for the below code to work */
> + if (!wx_test_staterr(rx_desc, WX_RXD_STAT_L4CS))
> + return;
> +
> + /*likely incorrect csum if IPv6 Dest Header found */
What does "likely incorrect" mean here? If it is incorrect,
does ring->rx_stats.csum_err need incrementing?
> + if (dptype.prot != WX_DEC_PTYPE_PROT_SCTP && WX_RXD_IPV6EX(rx_desc))
> + return;
> +
> + /* if L4 checksum error */
> + if (wx_test_staterr(rx_desc, WX_RXD_ERR_TCPE)) {
> + ring->rx_stats.csum_err++;
> + return;
> + }
> +
> + /* If there is an outer header present that might contain a checksum
> + * we need to bump the checksum level by 1 to reflect the fact that
> + * we are indicating we validated the inner checksum.
> + */
> + if (dptype.etype >= WX_DEC_PTYPE_ETYPE_IG) {
> + skb->csum_level = 1;
> + skb->encapsulation = 1;
> + }
> +
> + /* It must be a TCP or UDP or SCTP packet with a valid checksum */
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + ring->rx_stats.csum_good_cnt++;
> +}
> +
> +/**
> + * wx_process_skb_fields - Populate skb header fields from Rx descriptor
> + * @rx_ring: rx descriptor ring packet is being transacted on
> + * @rx_desc: pointer to the EOP Rx descriptor
> + * @skb: pointer to current skb being populated
> + *
> + * This function checks the ring, descriptor, and packet information in
> + * order to populate the hash, checksum, VLAN, timestamp, protocol, and
For now VLAN, timestamp are not populated yet.
> + * other fields within the skb.
> + **/
> +static void wx_process_skb_fields(struct wx_ring *rx_ring,
> + union wx_rx_desc *rx_desc,
> + struct sk_buff *skb)
> +{
> + wx_rx_hash(rx_ring, rx_desc, skb);
> + wx_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);
> +}
> +
> /**
> * wx_clean_rx_irq - Clean completed descriptors from Rx ring - bounce buf
> * @q_vector: structure containing interrupt and ring information
> @@ -491,8 +586,8 @@ static int wx_clean_rx_irq(struct wx_q_vector *q_vector,
> /* probably a little skewed due to removing CRC */
> total_rx_bytes += skb->len;
>
> - skb_record_rx_queue(skb, rx_ring->queue_index);
> - skb->protocol = eth_type_trans(skb, rx_ring->netdev);
> + /* populate checksum, timestamp, VLAN, and protocol */
> + wx_process_skb_fields(rx_ring, rx_desc, skb);
> napi_gro_receive(&q_vector->napi, skb);
>
> /* update budget accounting */
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 70f5fd168e40..69a9ed7bc2df 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -321,8 +321,31 @@
...
> +
> +static inline struct wx_dec_ptype wx_decode_ptype(const u8 ptype)
If the above is only used in one .c file, maybe it does not need
to be in the .h file?
> +{
> + return wx_ptype_lookup[ptype];
> +}
> +
> /* Host Interface Command Structures */
> struct wx_hic_hdr {
> u8 cmd;
> @@ -624,6 +853,11 @@ struct wx_queue_stats {
> u64 bytes;
> };
>
> +struct wx_rx_queue_stats {
> + u64 csum_good_cnt;
> + u64 csum_err;
> +};
> +
> /* iterator for handling rings in ring container */
> #define wx_for_each_ring(posm, headm) \
> for (posm = (headm).ring; posm; posm = posm->next)
> @@ -665,6 +899,9 @@ struct wx_ring {
>
> struct wx_queue_stats stats;
> struct u64_stats_sync syncp;
> + union {
> + struct wx_rx_queue_stats rx_stats;
> + };
> } ____cacheline_internodealigned_in_smp;
>
> struct wx_q_vector {
> @@ -680,6 +917,7 @@ struct wx_q_vector {
> struct napi_struct napi;
> struct rcu_head rcu; /* to avoid race with update stats on free */
>
> + bool netpoll_rx;
Unused?
> char name[IFNAMSIZ + 17];
>
> /* for dynamic allocation of rings associated with this q_vector */
>
Powered by blists - more mailing lists