| Message ID | 20221018085911.never.761-kees@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnx2: Pass allocation size to build_skb() | expand |
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?
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);
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 --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;
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(-)