| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <81fa5067-88d3-4693-9e42-7072b5430ac8@csgroup.eu>
Date: Mon, 4 Nov 2024 15:34:26 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Breno Leitao <leitao@...ian.org>,
Madalin Bucur <madalin.bucur@....com>, Ioana Ciornei
<ioana.ciornei@....com>, Radu Bulie <radu-andrei.bulie@....com>,
Sean Anderson <sean.anderson@...ux.dev>, linux-kernel@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH net-next 2/3] net: dpaa_eth: add assertions about SGT
entry offsets in sg_fd_to_skb()
Le 29/10/2024 à 17:43, Vladimir Oltean a écrit :
> Multi-buffer frame descriptors (FDs) point to a buffer holding a
> scatter/gather table (SGT), which is a finite array of fixed-size
> entries, the last of which has qm_sg_entry_is_final(&sgt[i]) == true.
>
> Each SGT entry points to a buffer holding pieces of the frame.
> DPAARM.pdf explains in the figure called "Internal and External Margins,
> Scatter/Gather Frame Format" that the SGT table is located within its
> buffer at the same offset as the frame data start is located within the
> first packet buffer.
>
> +------------------------+
> Scatter/Gather Buffer | First Buffer | Last Buffer
> ^ +------------+ ^ +-|---->^ +------------+ +->+------------+
> | | | | ICEOF | | | | | |////////////|
> | +------------+ v | | | | | |////////////|
> BSM | |/ part of //| | |BSM | | | |////////////|
> | |/ Internal /| | | | | | |////////////|
> | |/ Context //| | | | | | |// Frame ///|
> | +------------+ | | | | | ... |/ content //|
> | | | | | | | | |////////////|
> | | | | | | | | |////////////|
> v +------------+ | | v +------------+ |////////////|
> | Scatter/ //| sgt[0]--+ | |// Frame ///| |////////////|
> | Gather List| ... | |/ content //| +------------+ ^
> |////////////| sgt[N]----+ |////////////| | | | BEM
> |////////////| |////////////| | | |
> +------------+ +------------+ +------------+ v
>
> BSM = Buffer Start Margin, BEM = Buffer End Margin, both are configured
> by dpaa_eth_init_rx_port() for the RX FMan port relevant here.
>
> sg_fd_to_skb() runs in the calling context of rx_default_dqrr() -
> the NAPI receive callback - which only expects to receive contiguous
> (qm_fd_contig) or scatter/gather (qm_fd_sg) frame descriptors.
> Everything else is irrelevant codewise.
>
> The processing done by sg_fd_to_skb() is weird because it does not
> conform to the expectations laid out by the aforementioned figure.
> Namely, it parses the OFFSET field only for SGT entries with i != 0
> (codewise, skb != NULL). In those cases, OFFSET should always be 0.
> Also, it does not parse the OFFSET field for the sgt[0] case, the only
> case where the buffer offset is meaningful in this context. There, it
> uses the fd_off, aka the offset to the Scatter/Gather List in the
> Scatter/Gather Buffer from the figure. By equivalence, they should both
> be equal to the BSM (in turn, equal to priv->rx_headroom).
>
> This can actually be explained due to the bug which we had in
> qm_sg_entry_get_off() until the previous change:
>
> - qm_sg_entry_get_off() did not actually _work_ for sgt[0]. It returned
> zero even with a non-zero offset, so fd_off had to be used as a fill-in.
>
> - qm_sg_entry_get_off() always returned zero for sgt[i>0], and that
> resulted in no user-visible bug, because the buffer offset _was
> supposed_ to be zero for those buffers. So remove it from calculations.
>
> Add assertions about the OFFSET field in both cases (first or subsequent
> SGT entries) to make it absolutely obvious when something is not well
> handled.
>
> Similar logic can be seen in the driver for the architecturally similar
> DPAA2, where dpaa2_eth_build_frag_skb() calls dpaa2_sg_get_offset() only
> for i == 0. For the rest, there is even a comment stating the same thing:
>
> * Data in subsequent SG entries is stored from the
> * beginning of the buffer, so we don't need to add the
> * sg_offset.
>
> Tested on LS1046A.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
Acked-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
> .../net/ethernet/freescale/dpaa/dpaa_eth.c | 24 ++++++++++++-------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index ac06b01fe934..e280013afa63 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -1820,7 +1820,6 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,
> struct page *page, *head_page;
> struct dpaa_bp *dpaa_bp;
> void *vaddr, *sg_vaddr;
> - int frag_off, frag_len;
> struct sk_buff *skb;
> dma_addr_t sg_addr;
> int page_offset;
> @@ -1863,6 +1862,11 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,
> * on Tx, if extra headers are added.
> */
> WARN_ON(fd_off != priv->rx_headroom);
> + /* The offset to data start within the buffer holding
> + * the SGT should always be equal to the offset to data
> + * start within the first buffer holding the frame.
> + */
> + WARN_ON_ONCE(fd_off != qm_sg_entry_get_off(&sgt[i]));
> skb_reserve(skb, fd_off);
> skb_put(skb, qm_sg_entry_get_len(&sgt[i]));
> } else {
> @@ -1876,21 +1880,23 @@ static struct sk_buff *sg_fd_to_skb(const struct dpaa_priv *priv,
> page = virt_to_page(sg_vaddr);
> head_page = virt_to_head_page(sg_vaddr);
>
> - /* Compute offset in (possibly tail) page */
> + /* Compute offset of sg_vaddr in (possibly tail) page */
> page_offset = ((unsigned long)sg_vaddr &
> (PAGE_SIZE - 1)) +
> (page_address(page) - page_address(head_page));
> - /* page_offset only refers to the beginning of sgt[i];
> - * but the buffer itself may have an internal offset.
> +
> + /* Non-initial SGT entries should not have a buffer
> + * offset.
> */
> - frag_off = qm_sg_entry_get_off(&sgt[i]) + page_offset;
> - frag_len = qm_sg_entry_get_len(&sgt[i]);
> + WARN_ON_ONCE(qm_sg_entry_get_off(&sgt[i]));
> +
> /* skb_add_rx_frag() does no checking on the page; if
> * we pass it a tail page, we'll end up with
> - * bad page accounting and eventually with segafults.
> + * bad page accounting and eventually with segfaults.
> */
> - skb_add_rx_frag(skb, i - 1, head_page, frag_off,
> - frag_len, dpaa_bp->size);
> + skb_add_rx_frag(skb, i - 1, head_page, page_offset,
> + qm_sg_entry_get_len(&sgt[i]),
> + dpaa_bp->size);
> }
>
> /* Update the pool count for the current {cpu x bpool} */
Powered by blists - more mailing lists