linux-kernel - Re: [PATCH net] net: ethernet: ti: cpsw: fix min eth packet size for non-switch use-cases

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:   Fri, 11 Jun 2021 23:52:53 +0300
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Ben Hutchings <ben.hutchings@...ensium.com>
CC:     "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        <linux-kernel@...r.kernel.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        <linux-omap@...r.kernel.org>, Lokesh Vutla <lokeshvutla@...com>,
        <stable@...r.kernel.org>
Subject: Re: [PATCH net] net: ethernet: ti: cpsw: fix min eth packet size for
 non-switch use-cases



On 11/06/2021 20:54, Ben Hutchings wrote:
> On Fri, Jun 11, 2021 at 04:27:32PM +0300, Grygorii Strashko wrote:
> [...]
>> --- a/drivers/net/ethernet/ti/cpsw_new.c
>> +++ b/drivers/net/ethernet/ti/cpsw_new.c
>> @@ -918,14 +918,17 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
>>   	struct cpts *cpts = cpsw->cpts;
>>   	struct netdev_queue *txq;
>>   	struct cpdma_chan *txch;
>> +	unsigned int len;
>>   	int ret, q_idx;
>>   
>> -	if (skb_padto(skb, CPSW_MIN_PACKET_SIZE)) {
>> +	if (skb_padto(skb, priv->tx_packet_min)) {
>>   		cpsw_err(priv, tx_err, "packet pad failed\n");
>>   		ndev->stats.tx_dropped++;
>>   		return NET_XMIT_DROP;
>>   	}
>>   
>> +	len = skb->len < priv->tx_packet_min ? priv->tx_packet_min : skb->len;
>> +
>>   	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
>>   	    priv->tx_ts_enabled && cpts_can_timestamp(cpts, skb))
>>   		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>> @@ -937,7 +940,7 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
>>   	txch = cpsw->txv[q_idx].ch;
>>   	txq = netdev_get_tx_queue(ndev, q_idx);
>>   	skb_tx_timestamp(skb);
>> -	ret = cpdma_chan_submit(txch, skb, skb->data, skb->len,
>> +	ret = cpdma_chan_submit(txch, skb, skb->data, len,
>>   				priv->emac_port);
>>   	if (unlikely(ret != 0)) {
>>   		cpsw_err(priv, tx_err, "desc submit failed\n");
> 
> This change is odd because cpdma_chan_submit() already pads the DMA
> length.
> 
> Would it not make more sense to update cpdma_params::min_packet_size
> instead of adding a second minimum?

i've been thinking about it, but cpdma parameter copied into cpdma context once at probe,
so change will be more complex.
Can be done if you insist.

> 
> [...]
>> @@ -1686,6 +1690,7 @@ static int cpsw_dl_switch_mode_set(struct devlink *dl, u32 id,
>>   
>>   			priv = netdev_priv(sl_ndev);
>>   			slave->port_vlan = vlan;
>> +			priv->tx_packet_min = CPSW_MIN_PACKET_SIZE_VLAN;
>>   			if (netif_running(sl_ndev))
>>   				cpsw_port_add_switch_def_ale_entries(priv,
>>   								     slave);
>> @@ -1714,6 +1719,7 @@ static int cpsw_dl_switch_mode_set(struct devlink *dl, u32 id,
>>   
>>   			priv = netdev_priv(slave->ndev);
>>   			slave->port_vlan = slave->data->dual_emac_res_vlan;
>> +			priv->tx_packet_min = CPSW_MIN_PACKET_SIZE;
>>   			cpsw_port_add_dual_emac_def_ale_entries(priv, slave);
>>   		}
>>
> [...]
> 
> What happens if this races with the TX path?  Should there be a
> netif_tx_lock() / netif_tx_unlock() around this change?

Mode change operation is heavy and expected to be done once when bridge is configured.
It will completely wipe out all ALE entries and so all VLAN setting -
which any way need to be configured (reconfigured) during bridge configuration.
So, traffic can be disturbed during mode change operation.

As result, in my opinion, it make no sense to add additional complexity here.

-- 
Best regards,
grygorii