DPDK patches and discussions
 help / color / mirror / Atom feed
From: wangyunjian <wangyunjian@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "keith.wiles@intel.com" <keith.wiles@intel.com>,
	"Lilijun (Jerry)" <jerry.lilijun@huawei.com>,
	xudingke <xudingke@huawei.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/5] net/tap: fix mbuf double free when writev fails
Date: Thu, 9 Apr 2020 08:03:23 +0000	[thread overview]
Message-ID: <34EFBCA9F01B0748BEB6B629CE643AE60CF5FDB2@DGGEMM533-MBX.china.huawei.com> (raw)
In-Reply-To: <ad0fd5a9-5f24-a14a-bfe9-729c280273ac@intel.com>



> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, April 7, 2020 8:35 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: keith.wiles@intel.com; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>; stable@dpdk.org
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3 1/5] net/tap: fix mbuf double
> free when writev fails
> 
> On 4/7/2020 5:22 AM, wangyunjian wrote:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > When the tap_write_mbufs() function return with break, mbuf was freed
> > without incrementing num_packets. This may lead applications also free
> > the mbuf. And the pmd_tx_burst() function should returns the number of
> > original packets it actually sent excluding tso mbufs.
> >
> > Fixes: 9396ad334672 ("net/tap: fix reported number of Tx packets")
> > CC: stable@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index 05470a211..4c4b6b0b2 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -521,7 +521,7 @@ tap_tx_l3_cksum(char *packet, uint64_t ol_flags,
> unsigned int l2_len,
> >  	}
> >  }
> >
> > -static inline void
> > +static inline int
> >  tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs,
> >  			struct rte_mbuf **pmbufs,
> >  			uint16_t *num_packets, unsigned long *num_tx_bytes) @@
> -588,7
> > +588,7 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs,
> >  			seg_len = rte_pktmbuf_data_len(mbuf);
> >  			l234_hlen = mbuf->l2_len + mbuf->l3_len + mbuf->l4_len;
> >  			if (seg_len < l234_hlen)
> > -				break;
> > +				return -1;
> >
> >  			/* To change checksums, work on a * copy of l2, l3
> >  			 * headers + l4 pseudo header
> > @@ -634,10 +634,12 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t
> num_mbufs,
> >  		/* copy the tx frame data */
> >  		n = writev(process_private->txq_fds[txq->queue_id], iovecs, j);
> >  		if (n <= 0)
> > -			break;
> > +			return -1;
> > +
> >  		(*num_packets)++;
> >  		(*num_tx_bytes) += rte_pktmbuf_pkt_len(mbuf);
> >  	}
> > +	return 0;
> >  }
> >
> >  /* Callback to handle sending packets from the tap interface @@
> > -708,8 +710,15 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> >  			num_mbufs = 1;
> >  		}
> >
> > -		tap_write_mbufs(txq, num_mbufs, mbuf,
> > -				&num_packets, &num_tx_bytes);
> > +		ret = tap_write_mbufs(txq, num_mbufs, mbuf,
> > +				      &num_packets, &num_tx_bytes);
> 
> reusing 'ret' here breaks the logic at the end of the loop that free tso mbufs,
> which expects 'ret' is number of mbufs in tso case.
> 
> > +		if (ret != 0) {
> > +			txq->stats.errs++;
> > +			/* free tso mbufs */
> > +			for (j = 0; j < ret; j++)
> 
> 'ret' only can be '0' or '-1', and we take the branch only when it is '-1', so this
> block is not used at all and it doesn't free any mbuf.

I'm sorry for my mistakes. I will fix it in next version.
what about following:

error = tap_write_mbufs(txq, num_mbufs, mbuf,
                     &num_packets, &num_tx_bytes);
if (error == -1) {
    txq->stats.errs++;
    /* free tso mbufs */
    for (j = 0; j < ret; j++)
        rte_pktmbuf_free(mbuf[j]);
    break;
}

Thanks
Yunjian
> > +				rte_pktmbuf_free(mbuf[j]);
> 
> 
> In the no tso case, if the 'tap_write_mbufs()' fails, this doesn't free the
> 'mbuf_in'.
> 
> > +			break;
> > +		}
> >  		num_tx++;
> >  		/* free original mbuf */
> >  		rte_pktmbuf_free(mbuf_in);
> > @@ -722,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,
> uint16_t nb_pkts)
> >  	txq->stats.errs += nb_pkts - num_tx;
> >  	txq->stats.obytes += num_tx_bytes;
> >
> > -	return num_packets;
> > +	return num_tx;
> 
> +1 to return number of original packets.
> 
> >  }
> >
> >  static const char *
> >


  reply	other threads:[~2020-04-09  8:03 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-07  4:22 [dpdk-dev] " wangyunjian
2020-04-07 12:34 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2020-04-09  8:03   ` wangyunjian [this message]
2020-04-09  9:52     ` Ferruh Yigit
2020-04-09 12:53       ` wangyunjian
2020-04-09 13:03         ` Ferruh Yigit
2020-04-09 14:51     ` Stephen Hemminger
2020-04-10  1:41       ` wangyunjian
2020-04-10  7:45         ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=34EFBCA9F01B0748BEB6B629CE643AE60CF5FDB2@DGGEMM533-MBX.china.huawei.com \
    --to=wangyunjian@huawei.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerry.lilijun@huawei.com \
    --cc=keith.wiles@intel.com \
    --cc=stable@dpdk.org \
    --cc=xudingke@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).