netdev - Re: [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 06 Mar 2023 08:01:30 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Geoff Levand <geoff@...radead.org>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error

On Sun, 2023-03-05 at 02:08 +0000, Geoff Levand wrote:
> The current Gelic Etherenet driver was checking the return value of its
> dma_map_single call, and not using the dma_mapping_error() routine.
> 
> Fixes runtime problems like these:
> 
>   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
> 
> Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
> Signed-off-by: Geoff Levand <geoff@...radead.org>
> ---
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index b0ebe0e603b4..40261947e0ea 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -323,7 +323,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
>  				       GELIC_DESCR_SIZE,
>  				       DMA_BIDIRECTIONAL);
>  
> -		if (!descr->bus_addr)
> +		if (dma_mapping_error(ctodev(card), descr->bus_addr))
>  			goto iommu_error;
>  
>  		descr->next = descr + 1;

The bus_addr value is __be32 and the dma_mapping_error should be CPU
ordered. I think there was a byteswap using cpu_to_be32 missing here.
In addition you will probably need to have an intermediate variable to
store it in to test the DMA address before you byte swap it and store
it in the descriptor.

> @@ -401,7 +401,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  						     descr->skb->data,
>  						     GELIC_NET_MAX_FRAME,
>  						     DMA_FROM_DEVICE));
> -	if (!descr->buf_addr) {
> +	if (dma_mapping_error(ctodev(card), descr->buf_addr)) {
>  		dev_kfree_skb_any(descr->skb);
>  		descr->skb = NULL;
>  		dev_info(ctodev(card),

This is happening AFTER the DMA is passed through a cpu_to_be32 right?
The test should be on the raw value, not the byteswapped value.

> @@ -781,7 +781,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
>  
>  	buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
>  
> -	if (!buf) {
> +	if (dma_mapping_error(ctodev(card), buf)) {
>  		dev_err(ctodev(card),
>  			"dma map 2 failed (%p, %i). Dropping packet\n",
>  			skb->data, skb->len);

This one is correct from what I can tell. I would recommend using it as
a template and applying it to the two above so that you can sort out
the byte ordering issues and perform the test and the CPU ordered DMA
variable.