netdev - Re: [PATCH net-next RESEND] net: bcmgenet: hide status block before TX timestamping

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: <d4558414-f58f-2a51-5957-9e9454f3cad4@gmail.com>
Date:   Mon, 25 Apr 2022 13:03:59 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>, opendmb@...il.com,
        f.fainelli@...il.com, bcm-kernel-feedback-list@...adcom.com
Cc:     netdev@...r.kernel.org, kernel-team@...com, kuba@...nel.org
Subject: Re: [PATCH net-next RESEND] net: bcmgenet: hide status block before
 TX timestamping

On 4/24/22 09:53, Jonathan Lemon wrote:
> The hardware checksum offloading requires use of a transmit
> status block inserted before the outgoing frame data, this was
> updated in '9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")'
> 
> However, skb_tx_timestamp() assumes that it is passed a raw frame
> and PTP parsing chokes on this status block.
> 
> Fix this by calling __skb_pull(), which hides the TSB before calling
> skb_tx_timestamp(), so an outgoing PTP packet is parsed correctly.
> 
> As the data in the skb has already been set up for DMA, and the
> dma_unmap_* calls use a separately stored address, there is no
> no effective change in the data transmission.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>

Without restructuring the way we do time-stamping within the driver to 
be before bcmgenet_insert_tsb() this appears to get you what you want 
without being too intrusive. I would slap a:

Fixes: d03825fba459 ("net: bcmgenet: add skb_tx_timestamp call")

since this is a bug fix that should be fixed back when this was added. 
It was only made recently problematic by default with the enabling of 
the transmit checksum offload which implicit forces the use of a 64 byte 
transmit status block.

With that said:

Acked-by: Florian Fainelli <f.fainelli@...il.com>

Now, if we wanted to avoid playing tricks with the sk_buff, we could 
place skb_tx_timestamp() earlier in the transmit path with the risk that 
we are no longer as close as possible from hitting the DMA, and subject 
to DMA mapping or allocation failures. Food for thought.

Thanks!
-- 
Florian