diff mbox series

[net-next] net: bcmgenet: Add a check for oversized packets

Message ID	20230127000819.3934-1-f.fainelli@gmail.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Florian Fainelli <f.fainelli@gmail.com> To: netdev@vger.kernel.org Cc: maxime@cerno.tech, Florian Fainelli <f.fainelli@gmail.com>, Doug Berger <opendmb@gmail.com>, Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org (open list) Subject: [PATCH net-next] net: bcmgenet: Add a check for oversized packets Date: Thu, 26 Jan 2023 16:08:19 -0800 Message-Id: <20230127000819.3934-1-f.fainelli@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	[net-next] net: bcmgenet: Add a check for oversized packets \| [net-next] net: bcmgenet: Add a check for oversized packets

Commit Message

Florian Fainelli Jan. 27, 2023, 12:08 a.m. UTC

  Occasionnaly we may get oversized packets from the hardware which
exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
check which drops the packet to avoid invoking skb_over_panic() and move
on to processing the next packet.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Leon Romanovsky Jan. 29, 2023, 9:42 a.m. UTC | #1

On Thu, Jan 26, 2023 at 04:08:19PM -0800, Florian Fainelli wrote:
> Occasionnaly we may get oversized packets from the hardware which
> exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
> check which drops the packet to avoid invoking skb_over_panic() and move
> on to processing the next packet.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 21973046b12b..d937daa8ee88 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2316,6 +2316,14 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  			  __func__, p_index, ring->c_index,
>  			  ring->read_ptr, dma_length_status);
>  
> +		if (unlikely(len > RX_BUF_LENGTH)) {
> +			netif_err(priv, rx_status, dev, "oversized packet\n");

I don't think that it is wise move to print to dmesg something that can
be triggered by user over network.

Thanks

> +			dev->stats.rx_length_errors++;
> +			dev->stats.rx_errors++;
> +			dev_kfree_skb_any(skb);
> +			goto next;
> +		}
> +
>  		if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
>  			netif_err(priv, rx_status, dev,
>  				  "dropping fragmented packet!\n");
> -- 
> 2.25.1
>

Florian Fainelli Jan. 29, 2023, 9:17 p.m. UTC | #2

On 1/29/2023 1:42 AM, Leon Romanovsky wrote:
> On Thu, Jan 26, 2023 at 04:08:19PM -0800, Florian Fainelli wrote:
>> Occasionnaly we may get oversized packets from the hardware which
>> exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
>> check which drops the packet to avoid invoking skb_over_panic() and move
>> on to processing the next packet.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 21973046b12b..d937daa8ee88 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -2316,6 +2316,14 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>>   			  __func__, p_index, ring->c_index,
>>   			  ring->read_ptr, dma_length_status);
>>   
>> +		if (unlikely(len > RX_BUF_LENGTH)) {
>> +			netif_err(priv, rx_status, dev, "oversized packet\n");
> 
> I don't think that it is wise move to print to dmesg something that can
> be triggered by user over network.

A frame larger than RX_BUF_LENGTH intentionally received would be 
segmented by the MAC, we have seen this happen however while playing 
with unsafe clock ratios for instance or when there are insufficient 
credits given to the Ethernet MAC to write frames into DRAM. The print 
is consistent with other errors that are captured and is only enabled if 
the appropriate ethtool message level bitmask is set.

patchwork-bot+netdevbpf@kernel.org Jan. 30, 2023, 7:30 a.m. UTC | #3

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 26 Jan 2023 16:08:19 -0800 you wrote:
> Occasionnaly we may get oversized packets from the hardware which
> exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
> check which drops the packet to avoid invoking skb_over_panic() and move
> on to processing the next packet.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> [...]

Here is the summary with links:
  - [net-next] net: bcmgenet: Add a check for oversized packets
    https://git.kernel.org/netdev/net-next/c/5c0862c2c962

You are awesome, thank you!

Leon Romanovsky Jan. 30, 2023, 10:09 a.m. UTC | #4

On Sun, Jan 29, 2023 at 01:17:43PM -0800, Florian Fainelli wrote:
> 
> 
> On 1/29/2023 1:42 AM, Leon Romanovsky wrote:
> > On Thu, Jan 26, 2023 at 04:08:19PM -0800, Florian Fainelli wrote:
> > > Occasionnaly we may get oversized packets from the hardware which
> > > exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
> > > check which drops the packet to avoid invoking skb_over_panic() and move
> > > on to processing the next packet.
> > > 
> > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > >   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > > index 21973046b12b..d937daa8ee88 100644
> > > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > > @@ -2316,6 +2316,14 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
> > >   			  __func__, p_index, ring->c_index,
> > >   			  ring->read_ptr, dma_length_status);
> > > +		if (unlikely(len > RX_BUF_LENGTH)) {
> > > +			netif_err(priv, rx_status, dev, "oversized packet\n");
> > 
> > I don't think that it is wise move to print to dmesg something that can
> > be triggered by user over network.
> 
> A frame larger than RX_BUF_LENGTH intentionally received would be segmented
> by the MAC, we have seen this happen however while playing with unsafe clock
> ratios for instance or when there are insufficient credits given to the
> Ethernet MAC to write frames into DRAM. The print is consistent with other
> errors that are captured and is only enabled if the appropriate ethtool
> message level bitmask is set.

I saw other prints in that function, but you add new one.
Won't netif_err() be printed by default in almost all distro?

Thanks

> -- 
> Florian

Florian Fainelli Jan. 30, 2023, 6:19 p.m. UTC | #5

On 1/30/23 02:09, Leon Romanovsky wrote:
> On Sun, Jan 29, 2023 at 01:17:43PM -0800, Florian Fainelli wrote:
>>
>>
>> On 1/29/2023 1:42 AM, Leon Romanovsky wrote:
>>> On Thu, Jan 26, 2023 at 04:08:19PM -0800, Florian Fainelli wrote:
>>>> Occasionnaly we may get oversized packets from the hardware which
>>>> exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
>>>> check which drops the packet to avoid invoking skb_over_panic() and move
>>>> on to processing the next packet.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>    drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>>> index 21973046b12b..d937daa8ee88 100644
>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>>> @@ -2316,6 +2316,14 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>>>>    			  __func__, p_index, ring->c_index,
>>>>    			  ring->read_ptr, dma_length_status);
>>>> +		if (unlikely(len > RX_BUF_LENGTH)) {
>>>> +			netif_err(priv, rx_status, dev, "oversized packet\n");
>>>
>>> I don't think that it is wise move to print to dmesg something that can
>>> be triggered by user over network.
>>
>> A frame larger than RX_BUF_LENGTH intentionally received would be segmented
>> by the MAC, we have seen this happen however while playing with unsafe clock
>> ratios for instance or when there are insufficient credits given to the
>> Ethernet MAC to write frames into DRAM. The print is consistent with other
>> errors that are captured and is only enabled if the appropriate ethtool
>> message level bitmask is set.
> 
> I saw other prints in that function, but you add new one.
> Won't netif_err() be printed by default in almost all distro?

Do distributions alter the drive default message level:

   #define GENET_MSG_DEFAULT       (NETIF_MSG_DRV | NETIF_MSG_PROBE | \
                                   NETIF_MSG_LINK)
?

Leon Romanovsky Jan. 31, 2023, 8:16 a.m. UTC | #6

On Mon, Jan 30, 2023 at 10:19:07AM -0800, Florian Fainelli wrote:
> On 1/30/23 02:09, Leon Romanovsky wrote:
> > On Sun, Jan 29, 2023 at 01:17:43PM -0800, Florian Fainelli wrote:
> > > 
> > > 
> > > On 1/29/2023 1:42 AM, Leon Romanovsky wrote:
> > > > On Thu, Jan 26, 2023 at 04:08:19PM -0800, Florian Fainelli wrote:
> > > > > Occasionnaly we may get oversized packets from the hardware which
> > > > > exceed the nomimal 2KiB buffer size we allocate SKBs with. Add an early
> > > > > check which drops the packet to avoid invoking skb_over_panic() and move
> > > > > on to processing the next packet.
> > > > > 
> > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > > > ---
> > > > >    drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
> > > > >    1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > > > > index 21973046b12b..d937daa8ee88 100644
> > > > > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > > > > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > > > > @@ -2316,6 +2316,14 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
> > > > >    			  __func__, p_index, ring->c_index,
> > > > >    			  ring->read_ptr, dma_length_status);
> > > > > +		if (unlikely(len > RX_BUF_LENGTH)) {
> > > > > +			netif_err(priv, rx_status, dev, "oversized packet\n");
> > > > 
> > > > I don't think that it is wise move to print to dmesg something that can
> > > > be triggered by user over network.
> > > 
> > > A frame larger than RX_BUF_LENGTH intentionally received would be segmented
> > > by the MAC, we have seen this happen however while playing with unsafe clock
> > > ratios for instance or when there are insufficient credits given to the
> > > Ethernet MAC to write frames into DRAM. The print is consistent with other
> > > errors that are captured and is only enabled if the appropriate ethtool
> > > message level bitmask is set.
> > 
> > I saw other prints in that function, but you add new one.
> > Won't netif_err() be printed by default in almost all distro?
> 
> Do distributions alter the drive default message level:
> 
>   #define GENET_MSG_DEFAULT       (NETIF_MSG_DRV | NETIF_MSG_PROBE | \
>                                   NETIF_MSG_LINK)
> ?

Ohh, I didn't know about per-driver defaults.

Thanks for the explanation.

> -- 
> Florian
>

diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 21973046b12b..d937daa8ee88 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2316,6 +2316,14 @@  static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 			  __func__, p_index, ring->c_index,
 			  ring->read_ptr, dma_length_status);
 
+		if (unlikely(len > RX_BUF_LENGTH)) {
+			netif_err(priv, rx_status, dev, "oversized packet\n");
+			dev->stats.rx_length_errors++;
+			dev->stats.rx_errors++;
+			dev_kfree_skb_any(skb);
+			goto next;
+		}
+
 		if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
 			netif_err(priv, rx_status, dev,
 				  "dropping fragmented packet!\n");