ethernet: et131x: Add missing check after DMA map

[PATCH net] ethernet: et131x: Add missing check after DMA map

Posted by Thomas Fourier 3 months ago

The DMA map functions can fail and should be tested for errors.
If the mapping fails, unmap and return an error.

Fixes: 38df6492eb51 ("et131x: Add PCIe gigabit ethernet driver et131x to drivers/net")
Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
 drivers/net/ethernet/agere/et131x.c | 37 +++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/agere/et131x.c b/drivers/net/ethernet/agere/et131x.c
index 678eddb36172..b32e2a5af779 100644
--- a/drivers/net/ethernet/agere/et131x.c
+++ b/drivers/net/ethernet/agere/et131x.c
@@ -2459,6 +2459,10 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
 							  skb->data,
 							  skb_headlen(skb),
 							  DMA_TO_DEVICE);
+				if (dma_mapping_error(&adapter->pdev->dev,
+						      dma_addr))
+					return -ENOMEM;
+
 				desc[frag].addr_lo = lower_32_bits(dma_addr);
 				desc[frag].addr_hi = upper_32_bits(dma_addr);
 				frag++;
@@ -2468,6 +2472,10 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
 							  skb->data,
 							  skb_headlen(skb) / 2,
 							  DMA_TO_DEVICE);
+				if (dma_mapping_error(&adapter->pdev->dev,
+						      dma_addr))
+					return -ENOMEM;
+
 				desc[frag].addr_lo = lower_32_bits(dma_addr);
 				desc[frag].addr_hi = upper_32_bits(dma_addr);
 				frag++;
@@ -2478,6 +2486,10 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
 							  skb_headlen(skb) / 2,
 							  skb_headlen(skb) / 2,
 							  DMA_TO_DEVICE);
+				if (dma_mapping_error(&adapter->pdev->dev,
+						      dma_addr))
+					goto unmap_first_out;
+
 				desc[frag].addr_lo = lower_32_bits(dma_addr);
 				desc[frag].addr_hi = upper_32_bits(dma_addr);
 				frag++;
@@ -2489,6 +2501,9 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
 						    0,
 						    desc[frag].len_vlan,
 						    DMA_TO_DEVICE);
+			if (dma_mapping_error(&adapter->pdev->dev, dma_addr))
+				goto unmap_out;
+
 			desc[frag].addr_lo = lower_32_bits(dma_addr);
 			desc[frag].addr_hi = upper_32_bits(dma_addr);
 			frag++;
@@ -2578,6 +2593,28 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
 		       &adapter->regs->global.watchdog_timer);
 	}
 	return 0;
+
+unmap_out:
+	// Unmap everything from i-1 to 1
+	while (--i) {
+		frag--;
+		dma_addr = desc[frag].addr_lo;
+		dma_addr |= (u64)desc[frag].addr_hi << 32;
+		dma_unmap_page(&adapter->pdev->dev, dma_addr,
+			       desc[frag].len_vlan, DMA_TO_DEVICE);
+	}
+
+unmap_first_out:
+	// unmap header
+	while (frag--) {
+		frag--;
+		dma_addr = desc[frag].addr_lo;
+		dma_addr |= (u64)desc[frag].addr_hi << 32;
+		dma_unmap_single(&adapter->pdev->dev, dma_addr,
+				 desc[frag].len_vlan, DMA_TO_DEVICE);
+	}
+
+	return -ENOMEM;
 }
 
 static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)
-- 
2.43.0

Re: [PATCH net] ethernet: et131x: Add missing check after DMA map

Posted by Simon Horman 3 months ago

On Mon, Jul 07, 2025 at 11:09:49AM +0200, Thomas Fourier wrote:
> The DMA map functions can fail and should be tested for errors.
> If the mapping fails, unmap and return an error.
> 
> Fixes: 38df6492eb51 ("et131x: Add PCIe gigabit ethernet driver et131x to drivers/net")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>

nits:

1) There are two spaces after "et131x:" in the subject.
   One is enough.

2) I think you can drop "ethernet: " from the subject.
   "et131x: " seems to be an appropriate prefix based on git history.

...

> @@ -2578,6 +2593,28 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
>  		       &adapter->regs->global.watchdog_timer);
>  	}
>  	return 0;
> +
> +unmap_out:
> +	// Unmap everything from i-1 to 1
> +	while (--i) {
> +		frag--;
> +		dma_addr = desc[frag].addr_lo;
> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
> +		dma_unmap_page(&adapter->pdev->dev, dma_addr,
> +			       desc[frag].len_vlan, DMA_TO_DEVICE);
> +	}

I'm probably missing something obvious. But it seems to me that frag is
incremented iff a mapping is successful. So I think only the loop below is
needed.

> +
> +unmap_first_out:
> +	// unmap header
> +	while (frag--) {
> +		frag--;

I don't think you want to decrement frag twice here.

> +		dma_addr = desc[frag].addr_lo;
> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
> +		dma_unmap_single(&adapter->pdev->dev, dma_addr,
> +				 desc[frag].len_vlan, DMA_TO_DEVICE);
> +	}
> +
> +	return -ENOMEM;
>  }
>  
>  static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)

-- 
pw-bot: changes-requested

Re: [PATCH net] ethernet: et131x: Add missing check after DMA map

Posted by Thomas Fourier 3 months ago

On 07/07/2025 22:01, Simon Horman wrote:

> On Mon, Jul 07, 2025 at 11:09:49AM +0200, Thomas Fourier wrote:
>> The DMA map functions can fail and should be tested for errors.
>> If the mapping fails, unmap and return an error.
>>
>> Fixes: 38df6492eb51 ("et131x: Add PCIe gigabit ethernet driver et131x to drivers/net")
>> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> nits:
>
> 1) There are two spaces after "et131x:" in the subject.
>     One is enough.
>
> 2) I think you can drop "ethernet: " from the subject.
>     "et131x: " seems to be an appropriate prefix based on git history.
>
> ...
Ok, I'll make sure to fix that.
>
>> @@ -2578,6 +2593,28 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
>>   		       &adapter->regs->global.watchdog_timer);
>>   	}
>>   	return 0;
>> +
>> +unmap_out:
>> +	// Unmap everything from i-1 to 1
>> +	while (--i) {
>> +		frag--;
>> +		dma_addr = desc[frag].addr_lo;
>> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
>> +		dma_unmap_page(&adapter->pdev->dev, dma_addr,
>> +			       desc[frag].len_vlan, DMA_TO_DEVICE);
>> +	}
> I'm probably missing something obvious. But it seems to me that frag is
> incremented iff a mapping is successful. So I think only the loop below is
> needed.
Yes, frag is incremented only after a successful mapping.

The first loop is to unmap the body of the packet (i >= 1) which is mapped

with skb_frag_dma_map() (and unmapped with dma_unmap_page).  The

second is to unmap the header which is either one or two mappings of

map_single.  Should I make the comments more explicit?

>
>> +
>> +unmap_first_out:
>> +	// unmap header
>> +	while (frag--) {
>> +		frag--;
> I don't think you want to decrement frag twice here.
>
>> +		dma_addr = desc[frag].addr_lo;
>> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
>> +		dma_unmap_single(&adapter->pdev->dev, dma_addr,
>> +				 desc[frag].len_vlan, DMA_TO_DEVICE);
>> +	}
>> +
>> +	return -ENOMEM;
>>   }
>>   
>>   static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)