netdev - Re: [PATCH] net: broadcom: bcm4908_enet: report queued and transmitted bytes

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]

Message-ID: <bc15d5e0-1e48-d353-fc90-680c8039bf4f@gmail.com>
Date:   Wed, 26 Oct 2022 17:12:39 +0200
From:   Rafał Miłecki <zajec5@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH] net: broadcom: bcm4908_enet: report queued and
 transmitted bytes

On 26.10.2022 16:58, Florian Fainelli wrote:
> On 10/26/2022 7:26 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@...ecki.pl>
>>
>> This allows BQL to operate avoiding buffer bloat and reducing latency.
>>
>> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
>> ---
>>   drivers/net/ethernet/broadcom/bcm4908_enet.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
>> index 93ccf549e2ed..e672a9ef4444 100644
>> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
>> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
>> @@ -495,6 +495,7 @@ static int bcm4908_enet_stop(struct net_device *netdev)
>>       netif_carrier_off(netdev);
>>       napi_disable(&rx_ring->napi);
>>       napi_disable(&tx_ring->napi);
>> +    netdev_reset_queue(netdev);
>>       bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
>>       bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
>> @@ -564,6 +565,8 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
>>       enet->netdev->stats.tx_bytes += skb->len;
>>       enet->netdev->stats.tx_packets++;
>> +    netdev_sent_queue(enet->netdev, skb->len);
> 
> There is an opportunity for fixing an use after free here, after you call bcm4908_enet_dma_tx_ring_enable() the hardware can start transmission right away and also call the TX completion handler, so you could be de-referencing a freed skb reference at this point. Also, to ensure that DMA is actually functional, it is recommended to increase TX stats in the TX completion handler, since that indicates that you have a functional completion process.

I see the problem, thanks!

Actually hw may start transmission even earlier - right after filling
buf_desc coherent struct.


> So long story short, if you record the skb length *before* calling bcm4908_enet_dma_tx_ring_enable() and use that for reporting sent bytes, you should be good.

I may still end up calling netdev_completed_queue() for data for which
I didn't call netdev_sent_queue() yet. Is that safe?

Maybe I just just call netdev_sent_queue() before updating the buf_desc?