From: Sven Van Asbroeck <[email protected]>
The ethernet frame length is calculated incorrectly. Depending on
the value of RX_HEAD_PADDING, this may result in ethernet frames
that are too short (cut off at the end), or too long (garbage added
to the end).
Fix by calculating the ethernet frame length correctly. For added
clarity, use the ETH_FCS_LEN constant in the calculation.
Many thanks to Heiner Kallweit for suggesting this solution.
Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2")
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70
To: Bryan Whitehead <[email protected]>
To: "David S. Miller" <[email protected]>
To: Jakub Kicinski <[email protected]>
To: George McCollister <[email protected]>
Cc: Heiner Kallweit <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
drivers/net/ethernet/microchip/lan743x_main.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 1c3e204d727c..7b6794aa8ea9 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu)
}
mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_);
- mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) &
- MAC_RX_MAX_SIZE_MASK_);
+ mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN)
+ << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_);
lan743x_csr_write(adapter, MAC_RX, mac_rx);
if (enabled) {
@@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index)
struct sk_buff *skb;
dma_addr_t dma_ptr;
- buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;
+ buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING;
descriptor = &rx->ring_cpu_ptr[index];
buffer_info = &rx->buffer_info[index];
@@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
dev_kfree_skb_irq(skb);
return NULL;
}
- frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4);
+ frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN);
if (skb->len > frame_length) {
skb->tail -= skb->len - frame_length;
skb->len = frame_length;
--
2.17.1
Hi Sven
> Many thanks to Heiner Kallweit for suggesting this solution.
Adding a Suggested-by: would be good. And it might sometime help
Johnathan Corbet extract some interesting statistics from the commit
messages if everybody uses the same format.
Andrew
Hi Andrew,
On Thu, Apr 8, 2021 at 9:02 PM Andrew Lunn <[email protected]> wrote:
>
> Adding a Suggested-by: would be good. And it might sometime help
> Johnathan Corbet extract some interesting statistics from the commit
> messages if everybody uses the same format.
Thank you for the suggestion. I'll definitely use that in the future.
On Thu, Apr 8, 2021 at 7:39 PM Sven Van Asbroeck <[email protected]> wrote:
>
> From: Sven Van Asbroeck <[email protected]>
>
> The ethernet frame length is calculated incorrectly. Depending on
> the value of RX_HEAD_PADDING, this may result in ethernet frames
> that are too short (cut off at the end), or too long (garbage added
> to the end).
>
> Fix by calculating the ethernet frame length correctly. For added
> clarity, use the ETH_FCS_LEN constant in the calculation.
>
> Many thanks to Heiner Kallweit for suggesting this solution.
>
> Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2")
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Sven Van Asbroeck <[email protected]>
I'm glad everyone was able to work together to get this fixed properly
without any figure pointing or mud slinging! Kudos everyone.
Reviewed-by: George McCollister <[email protected]>
Tested-By: George McCollister <[email protected]>
> ---
>
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70
>
> To: Bryan Whitehead <[email protected]>
> To: "David S. Miller" <[email protected]>
> To: Jakub Kicinski <[email protected]>
> To: George McCollister <[email protected]>
> Cc: Heiner Kallweit <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> drivers/net/ethernet/microchip/lan743x_main.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 1c3e204d727c..7b6794aa8ea9 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu)
> }
>
> mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_);
> - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) &
> - MAC_RX_MAX_SIZE_MASK_);
> + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN)
> + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_);
> lan743x_csr_write(adapter, MAC_RX, mac_rx);
>
> if (enabled) {
> @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index)
> struct sk_buff *skb;
> dma_addr_t dma_ptr;
>
> - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;
> + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING;
>
> descriptor = &rx->ring_cpu_ptr[index];
> buffer_info = &rx->buffer_info[index];
> @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
> dev_kfree_skb_irq(skb);
> return NULL;
> }
> - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4);
> + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN);
> if (skb->len > frame_length) {
> skb->tail -= skb->len - frame_length;
> skb->len = frame_length;
> --
> 2.17.1
>
Hi George,
On Fri, Apr 9, 2021 at 10:12 AM George McCollister
<[email protected]> wrote:
>
> I'm glad everyone was able to work together to get this fixed properly
> without any figure pointing or mud slinging! Kudos everyone.
Same, this is what the kernel community is supposed to be all about.
And thank you for testing+reviewing. And for not freaking out too much
when I lobbed that revert patch in your general direction :-]
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Thu, 8 Apr 2021 20:39:04 -0400 you wrote:
> From: Sven Van Asbroeck <[email protected]>
>
> The ethernet frame length is calculated incorrectly. Depending on
> the value of RX_HEAD_PADDING, this may result in ethernet frames
> that are too short (cut off at the end), or too long (garbage added
> to the end).
>
> [...]
Here is the summary with links:
- [net,v1] lan743x: fix ethernet frame cutoff issue
https://git.kernel.org/netdev/net/c/3bc41d6d2721
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On 14.04.21 15:19, Sven Van Asbroeck wrote:
> Hi Julian,
>
> On Wed, Apr 14, 2021 at 8:53 AM Julian Wiedmann <[email protected]> wrote:
>>
>> On a cursory glance, using __netdev_alloc_skb_ip_align() here should
>> allow you to get rid of all the RX_HEAD_PADDING gymnastics.
>>
>> And also avoid the need for setting RX_CFG_B_RX_PAD_2_, as the
>> NET_IP_ALIGN part would no longer get dma-mapped.
>
> That's an excellent suggestion, and I'll definitely keep that in mind
> for the future.
>
> In this case, I'm not sure if it could work. This NIC has multi-buffer
> frames. The dma-ed skbs represent frame fragments. A flag in the
> descriptor ring indicates if an skb is "first". If first, we can
> reserve the padding. Otherwise, we cannot. because that would corrupt
> a fragment in the middle. At the time of skb allocation, we do not
> know whether that skb will be "first".
>
__netdev_alloc_skb_ip_align() already reserves the NET_IP_ALIGN part.
So when the NIC stores into the dma-mapped skb->data parts, each
fragment will automatically have the required alignment - even when
you only care about the first fragment's alignment.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.12-rc7#n2125
>
> Maybe I'm missing a trick here? Feel free to suggest improvements,
> it's always much appreciated.
>
> Sven
>
On 09.04.21 02:39, Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <[email protected]>
>
> The ethernet frame length is calculated incorrectly. Depending on
> the value of RX_HEAD_PADDING, this may result in ethernet frames
> that are too short (cut off at the end), or too long (garbage added
> to the end).
>
> Fix by calculating the ethernet frame length correctly. For added
> clarity, use the ETH_FCS_LEN constant in the calculation.
>
> Many thanks to Heiner Kallweit for suggesting this solution.
>
> Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2")
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
>
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70
>
> To: Bryan Whitehead <[email protected]>
> To: "David S. Miller" <[email protected]>
> To: Jakub Kicinski <[email protected]>
> To: George McCollister <[email protected]>
> Cc: Heiner Kallweit <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> drivers/net/ethernet/microchip/lan743x_main.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 1c3e204d727c..7b6794aa8ea9 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu)
> }
>
> mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_);
> - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) &
> - MAC_RX_MAX_SIZE_MASK_);
> + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN)
> + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_);
> lan743x_csr_write(adapter, MAC_RX, mac_rx);
>
> if (enabled) {
> @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index)
> struct sk_buff *skb;
> dma_addr_t dma_ptr;
>
> - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;
> + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING;
>
On a cursory glance, using __netdev_alloc_skb_ip_align() here should
allow you to get rid of all the RX_HEAD_PADDING gymnastics.
And also avoid the need for setting RX_CFG_B_RX_PAD_2_, as the
NET_IP_ALIGN part would no longer get dma-mapped.
> descriptor = &rx->ring_cpu_ptr[index];
> buffer_info = &rx->buffer_info[index];
> @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length)
> dev_kfree_skb_irq(skb);
> return NULL;
> }
> - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4);
> + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN);
> if (skb->len > frame_length) {
> skb->tail -= skb->len - frame_length;
> skb->len = frame_length;
>
Hi Julian,
On Wed, Apr 14, 2021 at 8:53 AM Julian Wiedmann <[email protected]> wrote:
>
> On a cursory glance, using __netdev_alloc_skb_ip_align() here should
> allow you to get rid of all the RX_HEAD_PADDING gymnastics.
>
> And also avoid the need for setting RX_CFG_B_RX_PAD_2_, as the
> NET_IP_ALIGN part would no longer get dma-mapped.
That's an excellent suggestion, and I'll definitely keep that in mind
for the future.
In this case, I'm not sure if it could work. This NIC has multi-buffer
frames. The dma-ed skbs represent frame fragments. A flag in the
descriptor ring indicates if an skb is "first". If first, we can
reserve the padding. Otherwise, we cannot. because that would corrupt
a fragment in the middle. At the time of skb allocation, we do not
know whether that skb will be "first".
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.12-rc7#n2125
Maybe I'm missing a trick here? Feel free to suggest improvements,
it's always much appreciated.
Sven
Hi Julian,
On Wed, Apr 14, 2021 at 9:33 AM Julian Wiedmann <[email protected]> wrote:
>
> __netdev_alloc_skb_ip_align() already reserves the NET_IP_ALIGN part.
> So when the NIC stores into the dma-mapped skb->data parts, each
> fragment will automatically have the required alignment - even when
> you only care about the first fragment's alignment.
>
That's really interesting! I'm going to try that out.