netdev - Re: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio

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, 8 Feb 2023 21:30:19 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>
Cc:     intel-wired-lan@...osl.org, vinicius.gomes@...el.com,
        naamax.meir@...ux.intel.com, anthony.l.nguyen@...el.com,
        leon@...nel.org, davem@...emloft.net, pabeni@...hat.com,
        edumazet@...gle.com, tee.min.tan@...ux.intel.com,
        netdev@...r.kernel.org, sasha.neftin@...el.com
Subject: Re: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio

On Wed,  8 Feb 2023 08:33:27 +0800 Muhammad Husaini Zulkifli wrote:
> From: Tan Tee Min <tee.min.tan@...ux.intel.com>
> 
> Add support for configuring the max SDU for each Tx queue.
> If not specified, keep the default.

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 0cc327294dfb5..38ad437957ada 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1508,6 +1508,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  	__le32 launch_time = 0;
>  	u32 tx_flags = 0;
>  	unsigned short f;
> +	u32 max_sdu = 0;

This variable can be moved to the scope of the if() ?

>  	ktime_t txtime;
>  	u8 hdr_len = 0;
>  	int tso = 0;
> @@ -1527,6 +1528,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  		return NETDEV_TX_BUSY;
>  	}
>  
> +	if (tx_ring->max_sdu > 0) {
> +		max_sdu = tx_ring->max_sdu +
> +			  (skb_vlan_tagged(skb) ? VLAN_HLEN : 0);
> +
> +		if (skb->len > max_sdu)

You should increment some counter here. Otherwise it's a silent discard.

> +			goto skb_drop;
> +	}
> +
>  	if (!tx_ring->launchtime_enable)
>  		goto done;
>  
> @@ -1606,6 +1615,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  	dev_kfree_skb_any(first->skb);

first->skb is skb, as far as I can tell, you can reshuffle this code to
avoid adding the new return flow.

>  	first->skb = NULL;
>  
> +	return NETDEV_TX_OK;
> +
> +skb_drop:
> +	dev_kfree_skb_any(skb);
> +
>  	return NETDEV_TX_OK;
>  }

> @@ -6122,6 +6137,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  		}
>  	}
>  
> +	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct igc_ring *ring = adapter->tx_ring[i];
> +		struct net_device *dev = adapter->netdev;
> +
> +		if (qopt->max_sdu[i])
> +			ring->max_sdu = qopt->max_sdu[i] + dev->hard_header_len;

why hard_header_len? Isn't it always ETH_HLEN?

> +		else
> +			ring->max_sdu = 0;