Skip to contentSkip to conversation

sky2: Make sure there is at least one frag_addr available

PATCH Superseded Kees Cook 3 participants 3 messages Sep 20, 2023 – Sep 21, 2023

Subsystems (3)
Networking Network drivers Marvell gigabit Ethernet drivers (skge/sky2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 sky2: Make sure there is at least one frag_addr available

In the likely pathological case of building sky2 with 16k PAGE_SIZE, make sure there is at least 1 frag_addr in struct rx_ring_info:

In file included from include/linux/skbuff.h:28,
from include/net/net_namespace.h:43,
from include/linux/netdevice.h:38,
from drivers/net/ethernet/marvell/sky2.c:18:
drivers/net/ethernet/marvell/sky2.c: In function 'sky2_rx_unmap_skb':
include/linux/dma-mapping.h:416:36: warning: array subscript i is outside array bounds of 'dma_addr_t[0]' {aka 'long long unsigned int[]'} [-Warray-bounds=]

     416 | #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/marvell/sky2.c:1257:17: note: in expansion of macro 'dma_unmap_page'
    1257 |                 dma_unmap_page(&pdev->dev, re->frag_addr[i],
         |                 ^~~~~~~~~~~~~~
   In file included from drivers/net/ethernet/marvell/sky2.c:41:
   drivers/net/ethernet/marvell/sky2.h:2198:25: note: while referencing 'frag_addr'
    2198 |         dma_addr_t      frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT];
         |                         ^~~~~~~~~

With CONFIG_PAGE_SIZE_16KB=y, PAGE_SHIFT == 14, so:

  #define ETH_JUMBO_MTU   9000

causes "ETH_JUMBO_MTU >> PAGE_SHIFT" to be 0. Use "?: 1" to solve this build warning.

Cc Mirko Lindner <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc [email protected]
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
Signed-off-by Kees Cook <[email protected]>
---
 drivers/net/ethernet/marvell/sky2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/marvell/sky2.h +1 -1
index ddec1627f1a7..8d0bacf4e49c 100644
@@ -2195,7 +2195,7 @@ struct rx_ring_info {
 	struct sk_buff	*skb;
 	dma_addr_t	data_addr;
 	DEFINE_DMA_UNMAP_LEN(data_size);
-	dma_addr_t	frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT];
+	dma_addr_t	frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT ?: 1];
 };
 
 enum flow_control {
Signature · 1 line
2.34.1
#1 lore raw
Quoted 35 lines · On Wed, 20 Sep 2023 13:25:13 -0700 Kees Cook <[email protected]> wrote:

In the likely pathological case of building sky2 with 16k PAGE_SIZE, make sure there is at least 1 frag_addr in struct rx_ring_info:

In file included from include/linux/skbuff.h:28,
from include/net/net_namespace.h:43, from include/linux/netdevice.h:38, from drivers/net/ethernet/marvell/sky2.c:18:

   drivers/net/ethernet/marvell/sky2.c: In function 'sky2_rx_unmap_skb':
   include/linux/dma-mapping.h:416:36: warning: array subscript i is outside array bounds of 'dma_addr_t[0]' {aka 'long long unsigned int[]'} [-Warray-bounds=]
     416 | #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/marvell/sky2.c:1257:17: note: in expansion of macro 'dma_unmap_page'
    1257 |                 dma_unmap_page(&pdev->dev, re->frag_addr[i],
         |                 ^~~~~~~~~~~~~~
   In file included from drivers/net/ethernet/marvell/sky2.c:41:
   drivers/net/ethernet/marvell/sky2.h:2198:25: note: while referencing 'frag_addr'
    2198 |         dma_addr_t      frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT];
         |                         ^~~~~~~~~

With CONFIG_PAGE_SIZE_16KB=y, PAGE_SHIFT == 14, so:

  #define ETH_JUMBO_MTU   9000

causes "ETH_JUMBO_MTU >> PAGE_SHIFT" to be 0. Use "?: 1" to solve this build warning.

Cc Mirko Lindner <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc [email protected]
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
Signed-off-by Kees Cook <[email protected]>
Quoted 1 line

---

With page size of 16K the frag_addr[] array would never be used, so the original code was correct that size should be 0. But the compiler now gets upset with 0 size arrays thinking this is some flex array leftover? Or can't figure out that in this case an rx skb with fragments would never be created.

The workaround is fine, but could you add an explanatory comment?

Reviewed-by on v1 1/1
#2 lore raw
In reply to Kees Cook

From: Kees Cook <[email protected]>
Date: Wed, 20 Sep 2023 13:25:13 -0700

Quoted 25 lines

In the likely pathological case of building sky2 with 16k PAGE_SIZE, make sure there is at least 1 frag_addr in struct rx_ring_info:

In file included from include/linux/skbuff.h:28,
from include/net/net_namespace.h:43, from include/linux/netdevice.h:38, from drivers/net/ethernet/marvell/sky2.c:18:

   drivers/net/ethernet/marvell/sky2.c: In function 'sky2_rx_unmap_skb':
   include/linux/dma-mapping.h:416:36: warning: array subscript i is outside array bounds of 'dma_addr_t[0]' {aka 'long long unsigned int[]'} [-Warray-bounds=]
     416 | #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/marvell/sky2.c:1257:17: note: in expansion of macro 'dma_unmap_page'
    1257 |                 dma_unmap_page(&pdev->dev, re->frag_addr[i],
         |                 ^~~~~~~~~~~~~~
   In file included from drivers/net/ethernet/marvell/sky2.c:41:
   drivers/net/ethernet/marvell/sky2.h:2198:25: note: while referencing 'frag_addr'
    2198 |         dma_addr_t      frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT];
         |                         ^~~~~~~~~

With CONFIG_PAGE_SIZE_16KB=y, PAGE_SHIFT == 14, so:

  #define ETH_JUMBO_MTU   9000

causes "ETH_JUMBO_MTU >> PAGE_SHIFT" to be 0. Use "?: 1" to solve this build warning.

Quoted 10 lines
Cc Mirko Lindner <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc [email protected]
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
Signed-off-by Kees Cook <[email protected]>
Reviewed-by Alexander Lobakin <[email protected]>

That "nobody uses this HW on non-x86 systems, why bother" is fun each time.

Quoted 9 lines
---
 drivers/net/ethernet/marvell/sky2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/marvell/sky2.h +0 -0
index ddec1627f1a7..8d0bacf4e49c 100644

drivers/net/ethernet/marvell/sky2.h struct rx_ring_info { · line 2201

Quoted 8 lines
drivers/net/ethernet/marvell/sky2.h +1 -1
@@ -2195,7 +2195,7 @@ struct rx_ring_info {
 	struct sk_buff	*skb;
 	dma_addr_t	data_addr;
 	DEFINE_DMA_UNMAP_LEN(data_size);
-	dma_addr_t	frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT];
+	dma_addr_t	frag_addr[ETH_JUMBO_MTU >> PAGE_SHIFT ?: 1];
 };
 
 enum flow_control {

Thanks,
Olek