Re: [DPDK/ethdev Bug 1416] net/af_packet: tx_burst() can modify packets

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 16 Apr 2024 20:11:05 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > > > > > static uint16_t
> > > > > > eth_af_packet_tx(void *queue, struct rte_mbuf **bufs, uint16_t  
> > > > nb_pkts)  
> > > > > > {
> > > > > >         ...
> > > > > >         for (i = 0; i < nb_pkts; i++) {
> > > > > >                 mbuf = *bufs++;
> > > > > >
> > > > > >                 ...
> > > > > >
> > > > > >                 /* insert vlan info if necessary */
> > > > > >                 if (mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) {
> > > > > >                         if (rte_vlan_insert(&mbuf)) {
> > > > > >                                 rte_pktmbuf_free(mbuf);
> > > > > >                                 continue;
> > > > > >
> > > > > > AFAIU, it does copy of mbuf contents into pbuf anyway (just few  
> > line  
> > > > below).  
> > > > > > So the fix might be - simply insert VLAN tag at copying stage.
> > > > > > Feel free to correct me, if I missed something.  
> > > > >
> > > > > vlan_insert will fail if the mbuf is has refcnt > 1.
> > > > >
> > > > > static inline int rte_vlan_insert(struct rte_mbuf **m)
> > > > > {
> > > > > 	struct rte_ether_hdr *oh, *nh;
> > > > > 	struct rte_vlan_hdr *vh;
> > > > >
> > > > > 	/* Can't insert header if mbuf is shared */
> > > > > 	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
> > > > > 		return -EINVAL;  
> > > >
> > > > You are right, I missed that.
> > > > Will close it then.  
> > >
> > > Don't close, silent drop is also a bug.
> > >
> > > The VLAN tag could be insert when copying, as originally suggested.  
> > 
> > Agree, but to me that would be enhancement request, not a bug report.  
> 
> Hmm... there is still a bug, although slightly different:
> 
> net/af_packet: tx_burst() silently drops packets with RTE_MBUF_F_TX_VLAN if mbuf is shared
> 
> And the suggested fixes would fix this (other) bug.


In older DPDK, vlan_insert would try and clone the mbuf, but doing a rte_pktmbuf_clone().
But that was buggy and removed by:

commit 15a74163b12ed9b8b980b1576bdd8de16d60612b
Author: Ferruh Yigit <ferruh.yigit@intel.com>
Date:   Tue Apr 16 16:51:26 2019 +0100

    net: forbid VLAN insert in shared mbuf
    
    The vlan_insert() is buggy when it tries to handle the shared mbufs,
    instead don't support inserting VLAN tag into shared mbufs and return
    an error for that case.
    
    Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
    Acked-by: Olivier Matz <olivier.matz@6wind.com>