diff mbox series

[v2] net/af_packet: fix vlan_insert corruption

Message ID 20190408164112.12471-1-stephen@networkplumber.org (mailing list archive)
State Rejected, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] net/af_packet: fix vlan_insert corruption |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger April 8, 2019, 4:41 p.m. UTC 
  If the af_packet transmit is sending a VLAN packet,
and the transmit path to the kernel os full, then it would
mismanage the outgoing mbuf. The original mbuf would end up
being freed twice, once by AF_PACKET PMD and once by caller.

Reported-by: Chas Williams <3chas3@gmail.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - fix unnecessary parenthesis (was in original code)

 drivers/net/af_packet/rte_eth_af_packet.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Ferruh Yigit April 12, 2019, 4:28 p.m. UTC | #1 
On 4/8/2019 5:41 PM, Stephen Hemminger wrote:
> If the af_packet transmit is sending a VLAN packet,
> and the transmit path to the kernel os full, then it would
> mismanage the outgoing mbuf. The original mbuf would end up
> being freed twice, once by AF_PACKET PMD and once by caller.

This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
to duplicate the mbuf, right?

That patch looks like won't make the release, so I suggest this one wait that
patch, although this is harmless on its own, commit log is misleading.

[1]
https://patches.dpdk.org/patch/51870/

> 
> Reported-by: Chas Williams <3chas3@gmail.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - fix unnecessary parenthesis (was in original code)
> 
>  drivers/net/af_packet/rte_eth_af_packet.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 99e13fe48a30..91ceb94ecbaa 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -200,6 +200,11 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			continue;
>  		}
>  
> +		/* point at the next incoming frame */
> +		if (ppd->tp_status != TP_STATUS_AVAILABLE &&
> +		    poll(&pfd, 1, -1) < 0)
> +			break;
> +
>  		/* insert vlan info if necessary */
>  		if (mbuf->ol_flags & PKT_TX_VLAN_PKT) {
>  			if (rte_vlan_insert(&mbuf)) {
> @@ -208,11 +213,6 @@ eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			}
>  		}
>  
> -		/* point at the next incoming frame */
> -		if ((ppd->tp_status != TP_STATUS_AVAILABLE) &&
> -		    (poll(&pfd, 1, -1) < 0))
> -			break;
> -
>  		/* copy the tx frame data */
>  		pbuf = (uint8_t *) ppd + TPACKET2_HDRLEN -
>  			sizeof(struct sockaddr_ll);
>
Stephen Hemminger April 12, 2019, 10:08 p.m. UTC | #2 
On Fri, 12 Apr 2019 17:28:17 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:
> > If the af_packet transmit is sending a VLAN packet,
> > and the transmit path to the kernel os full, then it would
> > mismanage the outgoing mbuf. The original mbuf would end up
> > being freed twice, once by AF_PACKET PMD and once by caller.  
> 
> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
> to duplicate the mbuf, right?
> 
> That patch looks like won't make the release, so I suggest this one wait that
> patch, although this is harmless on its own, commit log is misleading.
> 
> [1]
> https://patches.dpdk.org/patch/51870/

It was always true, even with existing vlan_insert.
Existing vlan_insert has issues if it ever creates a clone packet.
Existing vlan_insert can duplicate mbuf through clone
Ferruh Yigit April 16, 2019, 9:37 a.m. UTC | #3 
On 4/12/2019 11:08 PM, Stephen Hemminger wrote:
> On Fri, 12 Apr 2019 17:28:17 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:
>>> If the af_packet transmit is sending a VLAN packet,
>>> and the transmit path to the kernel os full, then it would
>>> mismanage the outgoing mbuf. The original mbuf would end up
>>> being freed twice, once by AF_PACKET PMD and once by caller.  
>>
>> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
>> to duplicate the mbuf, right?
>>
>> That patch looks like won't make the release, so I suggest this one wait that
>> patch, although this is harmless on its own, commit log is misleading.
>>
>> [1]
>> https://patches.dpdk.org/patch/51870/
> 
> It was always true, even with existing vlan_insert.
> Existing vlan_insert has issues if it ever creates a clone packet.
> Existing vlan_insert can duplicate mbuf through clone
> 

Right, existing vlan_insert has same issue on af_packet.

But, should vlan_insert try to duplicate the mbuf when it is shared, does it
worth the complexity it brings? And when that support removed this patch won't
be needed.
Or perhaps can create a new API, that handles the shared mbuf and name
explicitly states it?

btw, 'continue' in our Tx loop is also not good, when the application gets less
'num_tx' because of 'continue' in 'vlan_insert' failure, it will think last
packets in the array not sent and will try to free them which will cause double
free again.
Bruce Richardson April 16, 2019, 9:42 a.m. UTC | #4 
On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote:
> On 4/12/2019 11:08 PM, Stephen Hemminger wrote:
> > On Fri, 12 Apr 2019 17:28:17 +0100
> > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > 
> >> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:
> >>> If the af_packet transmit is sending a VLAN packet,
> >>> and the transmit path to the kernel os full, then it would
> >>> mismanage the outgoing mbuf. The original mbuf would end up
> >>> being freed twice, once by AF_PACKET PMD and once by caller.  
> >>
> >> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
> >> to duplicate the mbuf, right?
> >>
> >> That patch looks like won't make the release, so I suggest this one wait that
> >> patch, although this is harmless on its own, commit log is misleading.
> >>
> >> [1]
> >> https://patches.dpdk.org/patch/51870/
> > 
> > It was always true, even with existing vlan_insert.
> > Existing vlan_insert has issues if it ever creates a clone packet.
> > Existing vlan_insert can duplicate mbuf through clone
> > 
> 
> Right, existing vlan_insert has same issue on af_packet.
> 
> But, should vlan_insert try to duplicate the mbuf when it is shared, does it
> worth the complexity it brings? And when that support removed this patch won't
> be needed.

I don't think vlan insert or other mbuf manipulation APIs should be
checking for shared state or not - that's the job of the app. We could have
cases where the user does want to modify a shared mbuf.

Regards,
/Bruce
Stephen Hemminger April 16, 2019, 3:03 p.m. UTC | #5 
On Tue, 16 Apr 2019 10:42:13 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote:
> > On 4/12/2019 11:08 PM, Stephen Hemminger wrote:  
> > > On Fri, 12 Apr 2019 17:28:17 +0100
> > > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > >   
> > >> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:  
> > >>> If the af_packet transmit is sending a VLAN packet,
> > >>> and the transmit path to the kernel os full, then it would
> > >>> mismanage the outgoing mbuf. The original mbuf would end up
> > >>> being freed twice, once by AF_PACKET PMD and once by caller.    
> > >>
> > >> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
> > >> to duplicate the mbuf, right?
> > >>
> > >> That patch looks like won't make the release, so I suggest this one wait that
> > >> patch, although this is harmless on its own, commit log is misleading.
> > >>
> > >> [1]
> > >> https://patches.dpdk.org/patch/51870/  
> > > 
> > > It was always true, even with existing vlan_insert.
> > > Existing vlan_insert has issues if it ever creates a clone packet.
> > > Existing vlan_insert can duplicate mbuf through clone
> > >   
> > 
> > Right, existing vlan_insert has same issue on af_packet.
> > 
> > But, should vlan_insert try to duplicate the mbuf when it is shared, does it
> > worth the complexity it brings? And when that support removed this patch won't
> > be needed.  
> 
> I don't think vlan insert or other mbuf manipulation APIs should be
> checking for shared state or not - that's the job of the app. We could have
> cases where the user does want to modify a shared mbuf.
> 
> Regards,
> /Bruce

The vlan_insert code is called on transmit, and there are lots of
cases where a transmit mbuf might be shared (like TCP stack). And in that
case inserting the vlan must be non-destructive to the original mbuf.

Whether you want to push the problem to the driver or do it in the
library, it is still a problem.
Bruce Richardson April 16, 2019, 3:13 p.m. UTC | #6 
On Tue, Apr 16, 2019 at 08:03:36AM -0700, Stephen Hemminger wrote:
> On Tue, 16 Apr 2019 10:42:13 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote:
> > > On 4/12/2019 11:08 PM, Stephen Hemminger wrote:  
> > > > On Fri, 12 Apr 2019 17:28:17 +0100
> > > > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > > >   
> > > >> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:  
> > > >>> If the af_packet transmit is sending a VLAN packet,
> > > >>> and the transmit path to the kernel os full, then it would
> > > >>> mismanage the outgoing mbuf. The original mbuf would end up
> > > >>> being freed twice, once by AF_PACKET PMD and once by caller.    
> > > >>
> > > >> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
> > > >> to duplicate the mbuf, right?
> > > >>
> > > >> That patch looks like won't make the release, so I suggest this one wait that
> > > >> patch, although this is harmless on its own, commit log is misleading.
> > > >>
> > > >> [1]
> > > >> https://patches.dpdk.org/patch/51870/  
> > > > 
> > > > It was always true, even with existing vlan_insert.
> > > > Existing vlan_insert has issues if it ever creates a clone packet.
> > > > Existing vlan_insert can duplicate mbuf through clone
> > > >   
> > > 
> > > Right, existing vlan_insert has same issue on af_packet.
> > > 
> > > But, should vlan_insert try to duplicate the mbuf when it is shared, does it
> > > worth the complexity it brings? And when that support removed this patch won't
> > > be needed.  
> > 
> > I don't think vlan insert or other mbuf manipulation APIs should be
> > checking for shared state or not - that's the job of the app. We could have
> > cases where the user does want to modify a shared mbuf.
> > 
> > Regards,
> > /Bruce
> 
> The vlan_insert code is called on transmit, and there are lots of
> cases where a transmit mbuf might be shared (like TCP stack). And in that
> case inserting the vlan must be non-destructive to the original mbuf.
> 
> Whether you want to push the problem to the driver or do it in the
> library, it is still a problem.

Yes, I agree it's a problem. I'd prefer see it done in the driver than in the
library, since it's higher in the SW stack and has more context information
as to what is safe or not.

/Bruce.
Ferruh Yigit July 4, 2019, 6:43 p.m. UTC | #7 
On 4/16/2019 4:13 PM, Bruce Richardson wrote:
> On Tue, Apr 16, 2019 at 08:03:36AM -0700, Stephen Hemminger wrote:
>> On Tue, 16 Apr 2019 10:42:13 +0100
>> Bruce Richardson <bruce.richardson@intel.com> wrote:
>>
>>> On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote:
>>>> On 4/12/2019 11:08 PM, Stephen Hemminger wrote:  
>>>>> On Fri, 12 Apr 2019 17:28:17 +0100
>>>>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>>>   
>>>>>> On 4/8/2019 5:41 PM, Stephen Hemminger wrote:  
>>>>>>> If the af_packet transmit is sending a VLAN packet,
>>>>>>> and the transmit path to the kernel os full, then it would
>>>>>>> mismanage the outgoing mbuf. The original mbuf would end up
>>>>>>> being freed twice, once by AF_PACKET PMD and once by caller.    
>>>>>>
>>>>>> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()'
>>>>>> to duplicate the mbuf, right?
>>>>>>
>>>>>> That patch looks like won't make the release, so I suggest this one wait that
>>>>>> patch, although this is harmless on its own, commit log is misleading.
>>>>>>
>>>>>> [1]
>>>>>> https://patches.dpdk.org/patch/51870/  
>>>>>
>>>>> It was always true, even with existing vlan_insert.
>>>>> Existing vlan_insert has issues if it ever creates a clone packet.
>>>>> Existing vlan_insert can duplicate mbuf through clone
>>>>>   
>>>>
>>>> Right, existing vlan_insert has same issue on af_packet.
>>>>
>>>> But, should vlan_insert try to duplicate the mbuf when it is shared, does it
>>>> worth the complexity it brings? And when that support removed this patch won't
>>>> be needed.  
>>>
>>> I don't think vlan insert or other mbuf manipulation APIs should be
>>> checking for shared state or not - that's the job of the app. We could have
>>> cases where the user does want to modify a shared mbuf.
>>>
>>> Regards,
>>> /Bruce
>>
>> The vlan_insert code is called on transmit, and there are lots of
>> cases where a transmit mbuf might be shared (like TCP stack). And in that
>> case inserting the vlan must be non-destructive to the original mbuf.
>>
>> Whether you want to push the problem to the driver or do it in the
>> library, it is still a problem.
> 
> Yes, I agree it's a problem. I'd prefer see it done in the driver than in the
> library, since it's higher in the SW stack and has more context information
> as to what is safe or not.

The patch not more needed with current rte_vlan_insert()
diff mbox series

Patch

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 99e13fe48a30..91ceb94ecbaa 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -200,6 +200,11 @@  eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			continue;
 		}
 
+		/* point at the next incoming frame */
+		if (ppd->tp_status != TP_STATUS_AVAILABLE &&
+		    poll(&pfd, 1, -1) < 0)
+			break;
+
 		/* insert vlan info if necessary */
 		if (mbuf->ol_flags & PKT_TX_VLAN_PKT) {
 			if (rte_vlan_insert(&mbuf)) {
@@ -208,11 +213,6 @@  eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			}
 		}
 
-		/* point at the next incoming frame */
-		if ((ppd->tp_status != TP_STATUS_AVAILABLE) &&
-		    (poll(&pfd, 1, -1) < 0))
-			break;
-
 		/* copy the tx frame data */
 		pbuf = (uint8_t *) ppd + TPACKET2_HDRLEN -
 			sizeof(struct sockaddr_ll);