diff mbox series

bnx2: Pass allocation size to build_skb()

Message ID 20221018085911.never.761-kees@kernel.org
State New
Headers
Series bnx2: Pass allocation size to build_skb() |

Commit Message

Kees Cook Oct. 18, 2022, 8:59 a.m. UTC 
  In preparation for requiring that build_skb() have a non-zero size
argument, pass the actual data allocation size explicitly into
build_skb().

Cc: Rasesh Mody <rmody@marvell.com>
Cc: GR-Linux-NIC-Dev@marvell.com
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/ethernet/broadcom/bnx2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Oct. 20, 2022, 12:02 a.m. UTC | #1 
On Tue, 18 Oct 2022 01:59:29 -0700 Kees Cook wrote:
> In preparation for requiring that build_skb() have a non-zero size
> argument, pass the actual data allocation size explicitly into
> build_skb().

build_skb(, 0) has the special meaning of "head buf has been kmalloc'd",
rather than alloc_page(). Was this changed and I missed it?
Kees Cook Oct. 22, 2022, 2:06 a.m. UTC | #2 
On Wed, Oct 19, 2022 at 05:02:55PM -0700, Jakub Kicinski wrote:
> On Tue, 18 Oct 2022 01:59:29 -0700 Kees Cook wrote:
> > In preparation for requiring that build_skb() have a non-zero size
> > argument, pass the actual data allocation size explicitly into
> > build_skb().
> 
> build_skb(, 0) has the special meaning of "head buf has been kmalloc'd",
> rather than alloc_page(). Was this changed and I missed it?

Hm, I'm not clear on it. I see ksize() being called, but I guess that
works for alloc_page() allocations too?

build_skb
	__build_skb:
		__build_skb_around:
		        unsigned int size = frag_size ? : ksize(data);

So I guess in this case, this patch is wrong, and should instead be this
to match the ksize() used in build_skb():

diff --git a/drivers/net/ethernet/broadcom/bnx2.c
b/drivers/net/ethernet/broadcom/bnx2.c
index fec57f1982c8..dbe310144780 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -5415,8 +5415,9 @@ bnx2_set_rx_ring_size(struct bnx2 *bp, u32 size)
 
        bp->rx_buf_use_size = rx_size;
        /* hw alignment + build_skb() overhead*/
-       bp->rx_buf_size = SKB_DATA_ALIGN(bp->rx_buf_use_size + BNX2_RX_ALIGN) +
-               NET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+       bp->rx_buf_size = kmalloc_size_roundup(
+               SKB_DATA_ALIGN(bp->rx_buf_use_size + BNX2_RX_ALIGN) +
+               NET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
        bp->rx_jumbo_thresh = rx_size - BNX2_RX_OFFSET;
        bp->rx_ring_size = size;
        bp->rx_max_ring = bnx2_find_max_ring(size, BNX2_MAX_RX_RINGS);
Jakub Kicinski Oct. 22, 2022, 3:44 a.m. UTC | #3 
On Fri, 21 Oct 2022 19:06:26 -0700 Kees Cook wrote:
> On Wed, Oct 19, 2022 at 05:02:55PM -0700, Jakub Kicinski wrote:
> > On Tue, 18 Oct 2022 01:59:29 -0700 Kees Cook wrote:  
> > > In preparation for requiring that build_skb() have a non-zero size
> > > argument, pass the actual data allocation size explicitly into
> > > build_skb().  
> > 
> > build_skb(, 0) has the special meaning of "head buf has been kmalloc'd",
> > rather than alloc_page(). Was this changed and I missed it?  
> 
> Hm, I'm not clear on it. I see ksize() being called, but I guess that
> works for alloc_page() allocations too?
> 
> build_skb
> 	__build_skb:
> 		__build_skb_around:
> 		        unsigned int size = frag_size ? : ksize(data);

Hm, what I'm saying is the definition of the frag_size is - the size of
the frag if page-backed, or 0 if kmalloc-backed.

So the ternary op above applies ksize only in the kmalloc case.

> So I guess in this case, this patch is wrong, and should instead be this
> to match the ksize() used in build_skb():
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> b/drivers/net/ethernet/broadcom/bnx2.c
> index fec57f1982c8..dbe310144780 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -5415,8 +5415,9 @@ bnx2_set_rx_ring_size(struct bnx2 *bp, u32 size)
>  
>         bp->rx_buf_use_size = rx_size;
>         /* hw alignment + build_skb() overhead*/
> -       bp->rx_buf_size = SKB_DATA_ALIGN(bp->rx_buf_use_size + BNX2_RX_ALIGN) +
> -               NET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +       bp->rx_buf_size = kmalloc_size_roundup(
> +               SKB_DATA_ALIGN(bp->rx_buf_use_size + BNX2_RX_ALIGN) +
> +               NET_SKB_PAD + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
>         bp->rx_jumbo_thresh = rx_size - BNX2_RX_OFFSET;
>         bp->rx_ring_size = size;
>         bp->rx_max_ring = bnx2_find_max_ring(size, BNX2_MAX_RX_RINGS);

IIUC you want the size of the allocation to match exactly to the result
of ksize()?  In that case - yup, the above looks good.

FWIW the kmalloc backed heads are actually a performance bottleneck 
so we'd be doing everyone a favor if we just converted the two drivers
which do this to use pages and killed the "feature".

But the roundup works well enough.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index fec57f1982c8..0b2d4972343a 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -3045,7 +3045,8 @@  bnx2_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u8 *data,
 
 	dma_unmap_single(&bp->pdev->dev, dma_addr, bp->rx_buf_use_size,
 			 DMA_FROM_DEVICE);
-	skb = build_skb(data, 0);
+
+	skb = build_skb(data, bp->rx_buf_size);
 	if (!skb) {
 		kfree(data);
 		goto error;