[DPDK] drivers/net: fix dereference after null check coverity
Checks
Commit Message
This patch tries to fix the coverity issues of dereference after null
check.
Coverity issue: 343452
Coverity issue: 343447
Coverity issue: 343422
Coverity issue: 343416
Coverity issue: 343407
Coverity issue: 343403
Coverity issue: 13245
Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
---
drivers/net/fm10k/fm10k_rxtx_vec.c | 3 +++
drivers/net/i40e/i40e_rxtx_vec_common.h | 3 +++
drivers/net/iavf/iavf_rxtx_vec_common.h | 3 +++
drivers/net/ice/ice_rxtx_vec_common.h | 3 +++
drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 3 +++
5 files changed, 15 insertions(+)
Comments
Hi, Xiao
Please explain what's the issue you fixed in the commit log.
And for the fix patch, fix line is needed, please refer to other merged patches.
commit e0474b94f8a36672d66be7408e3f7cf00e302329 is a good reference.
Qiming
-----Original Message-----
From: Zhang, Xiao
Sent: Saturday, July 13, 2019 12:40 AM
To: dev@dpdk.org
Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Xiao <xiao.zhang@intel.com>
Subject: [DPDK] drivers/net: fix dereference after null check coverity
This patch tries to fix the coverity issues of dereference after null check.
Coverity issue: 343452
Coverity issue: 343447
Coverity issue: 343422
Coverity issue: 343416
Coverity issue: 343407
Coverity issue: 343403
Coverity issue: 13245
Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
---
drivers/net/fm10k/fm10k_rxtx_vec.c | 3 +++
drivers/net/i40e/i40e_rxtx_vec_common.h | 3 +++
drivers/net/iavf/iavf_rxtx_vec_common.h | 3 +++
drivers/net/ice/ice_rxtx_vec_common.h | 3 +++
drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 3 +++
5 files changed, 15 insertions(+)
diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c b/drivers/net/fm10k/fm10k_rxtx_vec.c
index 788e248..cb840de 100644
--- a/drivers/net/fm10k/fm10k_rxtx_vec.c
+++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
@@ -602,6 +602,9 @@ fm10k_reassemble_packets(struct fm10k_rx_queue *rxq,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end != NULL) {
/* processing a split packet */
diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index 0e6ffa0..1351e41 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -20,6 +20,9 @@ reassemble_packets(struct i40e_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end != NULL) {
/* processing a split packet */
diff --git a/drivers/net/iavf/iavf_rxtx_vec_common.h b/drivers/net/iavf/iavf_rxtx_vec_common.h
index db509d7..ac3d62a 100644
--- a/drivers/net/iavf/iavf_rxtx_vec_common.h
+++ b/drivers/net/iavf/iavf_rxtx_vec_common.h
@@ -20,6 +20,9 @@ reassemble_packets(struct iavf_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned int pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end) {
/* processing a split packet */
diff --git a/drivers/net/ice/ice_rxtx_vec_common.h b/drivers/net/ice/ice_rxtx_vec_common.h
index c5f0d56..11da521 100644
--- a/drivers/net/ice/ice_rxtx_vec_common.h
+++ b/drivers/net/ice/ice_rxtx_vec_common.h
@@ -16,6 +16,9 @@ ice_rx_reassemble_packets(struct ice_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned int pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end) {
/* processing a split packet */
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
index a97c271..a95cc0a 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
@@ -19,6 +19,9 @@ reassemble_packets(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned int pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end != NULL) {
/* processing a split packet */
--
2.7.4
Hi Qiming,
I will update the commit log.
Thanks,
Xiao
> -----Original Message-----
> From: Yang, Qiming
> Sent: Monday, July 15, 2019 10:37 AM
> To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Xiao W
> <xiao.w.wang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: RE: [DPDK] drivers/net: fix dereference after null check coverity
>
> Hi, Xiao
> Please explain what's the issue you fixed in the commit log.
> And for the fix patch, fix line is needed, please refer to other merged patches.
> commit e0474b94f8a36672d66be7408e3f7cf00e302329 is a good reference.
>
> Qiming
> -----Original Message-----
> From: Zhang, Xiao
> Sent: Saturday, July 13, 2019 12:40 AM
> To: dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Xiao W
> <xiao.w.wang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Xiao <xiao.zhang@intel.com>
> Subject: [DPDK] drivers/net: fix dereference after null check coverity
>
> This patch tries to fix the coverity issues of dereference after null check.
>
> Coverity issue: 343452
> Coverity issue: 343447
> Coverity issue: 343422
> Coverity issue: 343416
> Coverity issue: 343407
> Coverity issue: 343403
> Coverity issue: 13245
>
> Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> ---
> drivers/net/fm10k/fm10k_rxtx_vec.c | 3 +++
> drivers/net/i40e/i40e_rxtx_vec_common.h | 3 +++
> drivers/net/iavf/iavf_rxtx_vec_common.h | 3 +++
> drivers/net/ice/ice_rxtx_vec_common.h | 3 +++
> drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 3 +++
> 5 files changed, 15 insertions(+)
>
> diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c
> b/drivers/net/fm10k/fm10k_rxtx_vec.c
> index 788e248..cb840de 100644
> --- a/drivers/net/fm10k/fm10k_rxtx_vec.c
> +++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
> @@ -602,6 +602,9 @@ fm10k_reassemble_packets(struct fm10k_rx_queue
> *rxq,
> struct rte_mbuf *end = rxq->pkt_last_seg;
> unsigned pkt_idx, buf_idx;
>
> + if (!start)
> + return 0;
> +
> for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> if (end != NULL) {
> /* processing a split packet */
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index 0e6ffa0..1351e41 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -20,6 +20,9 @@ reassemble_packets(struct i40e_rx_queue *rxq, struct
> rte_mbuf **rx_bufs,
> struct rte_mbuf *end = rxq->pkt_last_seg;
> unsigned pkt_idx, buf_idx;
>
> + if (!start)
> + return 0;
> +
> for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> if (end != NULL) {
> /* processing a split packet */
> diff --git a/drivers/net/iavf/iavf_rxtx_vec_common.h
> b/drivers/net/iavf/iavf_rxtx_vec_common.h
> index db509d7..ac3d62a 100644
> --- a/drivers/net/iavf/iavf_rxtx_vec_common.h
> +++ b/drivers/net/iavf/iavf_rxtx_vec_common.h
> @@ -20,6 +20,9 @@ reassemble_packets(struct iavf_rx_queue *rxq, struct
> rte_mbuf **rx_bufs,
> struct rte_mbuf *end = rxq->pkt_last_seg;
> unsigned int pkt_idx, buf_idx;
>
> + if (!start)
> + return 0;
> +
> for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> if (end) {
> /* processing a split packet */
> diff --git a/drivers/net/ice/ice_rxtx_vec_common.h
> b/drivers/net/ice/ice_rxtx_vec_common.h
> index c5f0d56..11da521 100644
> --- a/drivers/net/ice/ice_rxtx_vec_common.h
> +++ b/drivers/net/ice/ice_rxtx_vec_common.h
> @@ -16,6 +16,9 @@ ice_rx_reassemble_packets(struct ice_rx_queue *rxq,
> struct rte_mbuf **rx_bufs,
> struct rte_mbuf *end = rxq->pkt_last_seg;
> unsigned int pkt_idx, buf_idx;
>
> + if (!start)
> + return 0;
> +
> for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> if (end) {
> /* processing a split packet */
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> index a97c271..a95cc0a 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec_common.h
> @@ -19,6 +19,9 @@ reassemble_packets(struct ixgbe_rx_queue *rxq, struct
> rte_mbuf **rx_bufs,
> struct rte_mbuf *end = rxq->pkt_last_seg;
> unsigned int pkt_idx, buf_idx;
>
> + if (!start)
> + return 0;
> +
> for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
> if (end != NULL) {
> /* processing a split packet */
> --
> 2.7.4
On Sat, 13 Jul 2019 00:39:47 +0800
Xiao Zhang <xiao.zhang@intel.com> wrote:
> This patch tries to fix the coverity issues of dereference after null
> check.
>
> Coverity issue: 343452
> Coverity issue: 343447
> Coverity issue: 343422
> Coverity issue: 343416
> Coverity issue: 343407
> Coverity issue: 343403
> Coverity issue: 13245
>
> Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
I think this should be fixed deeper in the vector code.
Example for ixgbe.
static inline uint16_t
reassemble_packets(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_bufs,
uint16_t nb_bufs, uint8_t *split_flags)
{
struct rte_mbuf *pkts[nb_bufs]; /*finished pkts*/
struct rte_mbuf *start = rxq->pkt_first_seg;
So start is rxq->pkt_first_seg.
But caller has already checked for NULL here.
It has iterated across the first packets but not updated rxq->first_seg.
if (rxq->pkt_first_seg == NULL) {
/* find the first split flag, and only reassemble then*/
while (i < nb_bufs && !split_flags[i])
i++;
if (i == nb_bufs)
return nb_bufs;
}
return i + reassemble_packets(rxq, &rx_pkts[i], nb_bufs - i,
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, July 15, 2019 12:06 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Xiao W
> <xiao.w.wang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [DPDK] drivers/net: fix dereference after null check
> coverity
>
> On Sat, 13 Jul 2019 00:39:47 +0800
> Xiao Zhang <xiao.zhang@intel.com> wrote:
>
> > This patch tries to fix the coverity issues of dereference after null
> > check.
> >
> > Coverity issue: 343452
> > Coverity issue: 343447
> > Coverity issue: 343422
> > Coverity issue: 343416
> > Coverity issue: 343407
> > Coverity issue: 343403
> > Coverity issue: 13245
> >
> > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
>
> I think this should be fixed deeper in the vector code.
>
> Example for ixgbe.
>
>
> static inline uint16_t
> reassemble_packets(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_bufs,
> uint16_t nb_bufs, uint8_t *split_flags) {
> struct rte_mbuf *pkts[nb_bufs]; /*finished pkts*/
> struct rte_mbuf *start = rxq->pkt_first_seg;
>
> So start is rxq->pkt_first_seg.
>
> But caller has already checked for NULL here.
> It has iterated across the first packets but not updated rxq->first_seg.
Yes, this seems to be a bug but not a coverity issue. I will fix it.
> if (rxq->pkt_first_seg == NULL) {
> /* find the first split flag, and only reassemble then*/
> while (i < nb_bufs && !split_flags[i])
> i++;
> if (i == nb_bufs)
> return nb_bufs;
> }
> return i + reassemble_packets(rxq, &rx_pkts[i], nb_bufs - i,
@@ -602,6 +602,9 @@ fm10k_reassemble_packets(struct fm10k_rx_queue *rxq,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end != NULL) {
/* processing a split packet */
@@ -20,6 +20,9 @@ reassemble_packets(struct i40e_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end != NULL) {
/* processing a split packet */
@@ -20,6 +20,9 @@ reassemble_packets(struct iavf_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned int pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end) {
/* processing a split packet */
@@ -16,6 +16,9 @@ ice_rx_reassemble_packets(struct ice_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned int pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end) {
/* processing a split packet */
@@ -19,6 +19,9 @@ reassemble_packets(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_bufs,
struct rte_mbuf *end = rxq->pkt_last_seg;
unsigned int pkt_idx, buf_idx;
+ if (!start)
+ return 0;
+
for (buf_idx = 0, pkt_idx = 0; buf_idx < nb_bufs; buf_idx++) {
if (end != NULL) {
/* processing a split packet */