[PATCH net v1] lan743x: fix ethernet frame cutoff issue

[PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Sven Van Asbroeck]

From: Sven Van Asbroeck <thesven73@gmail.com>

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/20210408172353.21143-1-TheSven73@gmail.com/
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
---

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70

To: Bryan Whitehead <bryan.whitehead@microchip.com>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
To: George McCollister <george.mccollister@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: UNGLinuxDriver@microchip.com
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 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

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Andrew Lunn]

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

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Sven Van Asbroeck]

Hi Andrew,

On Thu, Apr 8, 2021 at 9:02 PM Andrew Lunn <andrew@lunn.ch> 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.

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[George McCollister]

On Thu, Apr 8, 2021 at 7:39 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> From: Sven Van Asbroeck <thesven73@gmail.com>
>
> 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/20210408172353.21143-1-TheSven73@gmail.com/
> Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>

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 <george.mccollister@gmail.com>
Tested-By: George McCollister <george.mccollister@gmail.com>

> ---
>
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70
>
> To: Bryan Whitehead <bryan.whitehead@microchip.com>
> To: "David S. Miller" <davem@davemloft.net>
> To: Jakub Kicinski <kuba@kernel.org>
> To: George McCollister <george.mccollister@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: UNGLinuxDriver@microchip.com
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
>  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
>

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Sven Van Asbroeck]

Hi George,

On Fri, Apr 9, 2021 at 10:12 AM George McCollister
<george.mccollister@gmail.com> 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 :-]

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[patchwork-bot+netdevbpf]

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 <thesven73@gmail.com>
> 
> 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

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Julian Wiedmann]

On 09.04.21 02:39, Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <thesven73@gmail.com>
> 
> 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/20210408172353.21143-1-TheSven73@gmail.com/
> Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
> ---
> 
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70
> 
> To: Bryan Whitehead <bryan.whitehead@microchip.com>
> To: "David S. Miller" <davem@davemloft.net>
> To: Jakub Kicinski <kuba@kernel.org>
> To: George McCollister <george.mccollister@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: UNGLinuxDriver@microchip.com
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
>  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;
> 

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Sven Van Asbroeck]

Hi Julian,

On Wed, Apr 14, 2021 at 8:53 AM Julian Wiedmann <jwi@linux.ibm.com> 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

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Julian Wiedmann]

On 14.04.21 15:19, Sven Van Asbroeck wrote:
> Hi Julian,
> 
> On Wed, Apr 14, 2021 at 8:53 AM Julian Wiedmann <jwi@linux.ibm.com> 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
> 

Re: [PATCH net v1] lan743x: fix ethernet frame cutoff issue

[Sven Van Asbroeck]

Hi Julian,

On Wed, Apr 14, 2021 at 9:33 AM Julian Wiedmann <jwi@linux.ibm.com> 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.