| 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: <785d512e-6c64-48bc-c4eb-54d21e23b9be@csgroup.eu>
Date: Thu, 5 Aug 2021 07:10:26 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Geoff Levand <geoff@...radead.org>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v4 10/10] net/ps3_gelic: Fix DMA mapping problems
Le 23/07/2021 à 22:31, Geoff Levand a écrit :
> Fixes several DMA mapping problems with the PS3's gelic network driver:
>
> * Change from checking the return value of dma_map_single to using the
> dma_mapping_error routine.
> * Use the correct buffer length when mapping the RX skb.
> * Improved error checking and debug logging.
>
> Fixes runtime errors like these, and also other randomly occurring errors:
>
> IP-Config: Complete:
> DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
> WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>
> Signed-off-by: Geoff Levand <geoff@...radead.org>
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#55: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:351:
+ descr->link.cpu_addr = dma_map_single(dev, descr,
+ descr->link.size, DMA_BIDIRECTIONAL);
WARNING:BRACES: braces {} are not necessary for single statement blocks
#62: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:358:
+ if (descr->link.cpu_addr) {
+ gelic_unmap_link(dev, descr);
+ }
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#157: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:440:
+ cpu_addr = dma_map_single(dev, descr->skb->data,
+ descr->hw_regs.payload.size, DMA_FROM_DEVICE);
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#262: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:612:
+ dev_info_ratelimited(dev,
+ "%s:%d: forcing end of tx descriptor with status %x\n",
CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#323: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:846:
+ cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
+ DMA_TO_DEVICE);
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Commit cf6041cd6b17 ("net/ps3_gelic: Fix DMA mapping problems") has style problems, please review.
NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH
EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 183 +++++++++++--------
> 1 file changed, 108 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 42f4de9ad5fe..11ddeacb1159 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -336,22 +336,31 @@ static int gelic_card_init_chain(struct gelic_card *card,
> struct gelic_descr_chain *chain, struct gelic_descr *start_descr,
> int descr_count)
> {
> - int i;
> - struct gelic_descr *descr;
> + struct gelic_descr *descr = start_descr;
> struct device *dev = ctodev(card);
> + unsigned int index;
>
> - descr = start_descr;
> - memset(descr, 0, sizeof(*descr) *descr_count);
> + memset(start_descr, 0, descr_count * sizeof(*start_descr));
>
> - for (i = 0; i < descr_count; i++, descr++) {
> - descr->link.size = sizeof(struct gelic_hw_regs);
> + for (index = 0, descr = start_descr; index < descr_count;
> + index++, descr++) {
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> - descr->link.cpu_addr =
> - dma_map_single(dev, descr, descr->link.size,
> - DMA_BIDIRECTIONAL);
>
> - if (!descr->link.cpu_addr)
> - goto iommu_error;
> + descr->link.size = sizeof(struct gelic_hw_regs);
> + descr->link.cpu_addr = dma_map_single(dev, descr,
> + descr->link.size, DMA_BIDIRECTIONAL);
> +
> + if (unlikely(dma_mapping_error(dev, descr->link.cpu_addr))) {
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
> + __LINE__);
> +
> + for (index--, descr--; index > 0; index--, descr--) {
> + if (descr->link.cpu_addr) {
> + gelic_unmap_link(dev, descr);
> + }
> + }
> + return -ENOMEM;
> + }
>
> descr->next = descr + 1;
> descr->prev = descr - 1;
> @@ -360,8 +369,9 @@ static int gelic_card_init_chain(struct gelic_card *card,
> (descr - 1)->next = start_descr;
> start_descr->prev = (descr - 1);
>
> - descr = start_descr;
> - for (i = 0; i < descr_count; i++, descr++) {
> + /* chain bus addr of hw descriptor */
> + for (index = 0, descr = start_descr; index < descr_count;
> + index++, descr++) {
> descr->hw_regs.next_descr_addr =
> cpu_to_be32(descr->next->link.cpu_addr);
> }
> @@ -373,12 +383,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
> (descr - 1)->hw_regs.next_descr_addr = 0;
>
> return 0;
> -
> -iommu_error:
> - for (i--, descr--; 0 <= i; i--, descr--)
> - if (descr->link.cpu_addr)
> - gelic_unmap_link(dev, descr);
> - return -ENOMEM;
> }
>
> /**
> @@ -395,49 +399,63 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> struct device *dev = ctodev(card);
> - int offset;
> - unsigned int bufsize;
> + struct aligned_buff {
> + unsigned int total_bytes;
> + unsigned int offset;
> + };
> + struct aligned_buff a_buf;
> + dma_addr_t cpu_addr;
>
> if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
> dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
> }
>
> - /* we need to round up the buffer size to a multiple of 128 */
> - bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> + a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
> + + GELIC_NET_RXBUF_ALIGN;
> +
> + descr->skb = dev_alloc_skb(a_buf.total_bytes);
>
> - /* and we need to have it 128 byte aligned, therefore we allocate a
> - * bit more */
> - descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> if (!descr->skb) {
> - descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch memory */
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> return -ENOMEM;
> }
> - descr->hw_regs.payload.size = cpu_to_be32(bufsize);
> +
> + a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
> + - descr->skb->data;
> +
> + if (a_buf.offset) {
> + dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
> + a_buf.offset);
> + skb_reserve(descr->skb, a_buf.offset);
> + }
> +
> descr->hw_regs.dmac_cmd_status = 0;
> descr->hw_regs.result_size = 0;
> descr->hw_regs.valid_size = 0;
> descr->hw_regs.data_error = 0;
>
> - offset = ((unsigned long)descr->skb->data) &
> - (GELIC_NET_RXBUF_ALIGN - 1);
> - if (offset)
> - skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> - /* io-mmu-map the skb */
> - descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(dev,
> - descr->skb->data,
> - GELIC_NET_MAX_MTU,
> - DMA_FROM_DEVICE));
> - if (!descr->hw_regs.payload.dev_addr) {
> + descr->hw_regs.payload.size = a_buf.total_bytes - a_buf.offset;
> + cpu_addr = dma_map_single(dev, descr->skb->data,
> + descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
> +
> + if (unlikely(dma_mapping_error(dev, cpu_addr))) {
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> +
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> - dev_info(dev,
> - "%s:Could not iommu-map rx buffer\n", __func__);
> +
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> +
> return -ENOMEM;
> - } else {
> - gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - return 0;
> }
> +
> + gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> + return 0;
> }
>
> /**
> @@ -454,13 +472,18 @@ static void gelic_card_release_rx_chain(struct gelic_card *card)
> if (descr->skb) {
> dma_unmap_single(dev,
> be32_to_cpu(descr->hw_regs.payload.dev_addr),
> - descr->skb->len, DMA_FROM_DEVICE);
> - descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> +
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> +
> gelic_descr_set_status(descr,
> GELIC_DESCR_DMA_NOT_IN_USE);
> }
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> +
> descr = descr->next;
> } while (descr != card->rx_chain.head);
> }
> @@ -526,17 +549,19 @@ static void gelic_descr_release_tx(struct gelic_card *card,
> GELIC_DESCR_TX_TAIL));
>
> dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> - skb->len, DMA_TO_DEVICE);
> - dev_kfree_skb_any(skb);
> + descr->hw_regs.payload.size, DMA_TO_DEVICE);
>
> descr->hw_regs.payload.dev_addr = 0;
> descr->hw_regs.payload.size = 0;
> +
> + dev_kfree_skb_any(skb);
> + descr->skb = NULL;
> +
> descr->hw_regs.next_descr_addr = 0;
> descr->hw_regs.result_size = 0;
> descr->hw_regs.valid_size = 0;
> descr->hw_regs.data_status = 0;
> descr->hw_regs.data_error = 0;
> - descr->skb = NULL;
>
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> }
> @@ -565,31 +590,34 @@ static void gelic_card_wake_queues(struct gelic_card *card)
> static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> {
> struct gelic_descr_chain *tx_chain;
> - enum gelic_descr_dma_status status;
> struct device *dev = ctodev(card);
> - struct net_device *netdev;
> - int release = 0;
> + int release;
> +
> + for (release = 0, tx_chain = &card->tx_chain;
> + tx_chain->head != tx_chain->tail && tx_chain->tail;
> + tx_chain->tail = tx_chain->tail->next) {
> + enum gelic_descr_dma_status status;
> + struct gelic_descr *descr;
> + struct net_device *netdev;
> +
> + descr = tx_chain->tail;
> + status = gelic_descr_get_status(descr);
> + netdev = descr->skb->dev;
>
> - for (tx_chain = &card->tx_chain;
> - tx_chain->head != tx_chain->tail && tx_chain->tail;
> - tx_chain->tail = tx_chain->tail->next) {
> - status = gelic_descr_get_status(tx_chain->tail);
> - netdev = tx_chain->tail->skb->dev;
> switch (status) {
> case GELIC_DESCR_DMA_RESPONSE_ERROR:
> case GELIC_DESCR_DMA_PROTECTION_ERROR:
> case GELIC_DESCR_DMA_FORCE_END:
> - dev_info_ratelimited(dev,
> - "%s:%d: forcing end of tx descriptor with status %x\n",
> - __func__, __LINE__, status);
> + dev_info_ratelimited(dev,
> + "%s:%d: forcing end of tx descriptor with status %x\n",
> + __func__, __LINE__, status);
> netdev->stats.tx_dropped++;
> break;
>
> case GELIC_DESCR_DMA_COMPLETE:
> - if (tx_chain->tail->skb) {
> + if (descr->skb) {
> netdev->stats.tx_packets++;
> - netdev->stats.tx_bytes +=
> - tx_chain->tail->skb->len;
> + netdev->stats.tx_bytes += descr->skb->len;
> }
> break;
>
> @@ -599,7 +627,7 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> }
> }
>
> - gelic_descr_release_tx(card, tx_chain->tail);
> + gelic_descr_release_tx(card, descr);
> release++;
> }
> out:
> @@ -703,19 +731,19 @@ int gelic_net_stop(struct net_device *netdev)
> *
> * returns the address of the next descriptor, or NULL if not available.
> */
> -static struct gelic_descr *
> -gelic_card_get_next_tx_descr(struct gelic_card *card)
> +static struct gelic_descr *gelic_card_get_next_tx_descr(struct gelic_card *card)
> {
> if (!card->tx_chain.head)
> return NULL;
> +
> /* see if the next descriptor is free */
> if (card->tx_chain.tail != card->tx_chain.head->next &&
> - gelic_descr_get_status(card->tx_chain.head) ==
> - GELIC_DESCR_DMA_NOT_IN_USE)
> + (gelic_descr_get_status(card->tx_chain.head) ==
> + GELIC_DESCR_DMA_NOT_IN_USE)) {
> return card->tx_chain.head;
> - else
> - return NULL;
> + }
>
> + return NULL;
> }
>
> /**
> @@ -809,18 +837,23 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
> if (!skb_tmp) {
> return -ENOMEM;
> }
> +
> skb = skb_tmp;
> }
>
> - cpu_addr = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
> + descr->hw_regs.payload.size = skb->len;
> + cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
> + DMA_TO_DEVICE);
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
>
> - if (!cpu_addr) {
> + if (unlikely(dma_mapping_error(dev, cpu_addr))) {
> dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> return -ENOMEM;
> }
>
> - descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
> - descr->hw_regs.payload.size = cpu_to_be32(skb->len);
> descr->skb = skb;
> descr->hw_regs.data_status = 0;
> descr->hw_regs.next_descr_addr = 0; /* terminate hw descr */
> @@ -948,9 +981,9 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
>
> data_status = be32_to_cpu(descr->hw_regs.data_status);
> data_error = be32_to_cpu(descr->hw_regs.data_error);
> - /* unmap skb buffer */
> +
> dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> - GELIC_NET_MAX_MTU, DMA_FROM_DEVICE);
> + descr->hw_regs.payload.size, DMA_FROM_DEVICE);
>
> skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size) ?
> be32_to_cpu(descr->hw_regs.valid_size) :
>
Powered by blists - more mailing lists