* Re: [dpdk-dev] [PATCH] net/softnic: fix memory illegal access
2018-07-20 9:44 [dpdk-dev] [PATCH] net/softnic: fix memory illegal access Jasvinder Singh
@ 2018-07-20 10:29 ` Bruce Richardson
2018-07-20 10:31 ` Van Haaren, Harry
2018-07-20 11:05 ` [dpdk-dev] [PATCH v2] " Jasvinder Singh
2 siblings, 0 replies; 6+ messages in thread
From: Bruce Richardson @ 2018-07-20 10:29 UTC (permalink / raw)
To: Jasvinder Singh; +Cc: dev, cristian.dumitrescu
On Fri, Jul 20, 2018 at 10:44:39AM +0100, Jasvinder Singh wrote:
> While deleting the elements from the linked list, TAILQ_FOREACH causes
> read from the freed pointer. Fixes the issue by using for loop instead
> of TAILQ_FOREACH.
>
> Coverity issue: 302867
> Fixes: bef50bcb1c47 ("net/softnic: implement start and stop")
>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> ---
> drivers/net/softnic/rte_eth_softnic_swq.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/softnic/rte_eth_softnic_swq.c b/drivers/net/softnic/rte_eth_softnic_swq.c
> index 1944fbb..a1f1899 100644
> --- a/drivers/net/softnic/rte_eth_softnic_swq.c
> +++ b/drivers/net/softnic/rte_eth_softnic_swq.c
> @@ -36,9 +36,11 @@ softnic_swq_free(struct pmd_internals *p)
> void
> softnic_softnic_swq_free_keep_rxq_txq(struct pmd_internals *p)
> {
> - struct softnic_swq *swq;
> + struct softnic_swq *swq, *swq_next;
> +
> + for (swq = TAILQ_FIRST(&p->swq_list); swq != NULL; swq = swq_next) {
> + swq_next = TAILQ_NEXT(swq, node);
>
> - TAILQ_FOREACH(swq, &p->swq_list, node) {
> if ((strncmp(swq->name, "RXQ", strlen("RXQ")) == 0) ||
> (strncmp(swq->name, "TXQ", strlen("TXQ")) == 0))
TAILQ_FOREACH_SAFE is probably what you want to use here.
>From man page:
The macros TAILQ_FOREACH, TAILQ_FOREACH_REVERSE, TAILQ_FOREACH_SAFE, and
TAILQ_FOREACH_REVERSE_SAFE traverse the tail queue referenced by head in
the forward or reverse direction direction, assigning each element in
turn to var.
The SAFE versions use tmp to hold the next element, so var may be freed
or removed from the list.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH] net/softnic: fix memory illegal access
2018-07-20 9:44 [dpdk-dev] [PATCH] net/softnic: fix memory illegal access Jasvinder Singh
2018-07-20 10:29 ` Bruce Richardson
@ 2018-07-20 10:31 ` Van Haaren, Harry
2018-07-20 10:49 ` Singh, Jasvinder
2018-07-20 11:05 ` [dpdk-dev] [PATCH v2] " Jasvinder Singh
2 siblings, 1 reply; 6+ messages in thread
From: Van Haaren, Harry @ 2018-07-20 10:31 UTC (permalink / raw)
To: Singh, Jasvinder, dev; +Cc: Dumitrescu, Cristian
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jasvinder Singh
> Sent: Friday, July 20, 2018 10:45 AM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/softnic: fix memory illegal access
>
> While deleting the elements from the linked list, TAILQ_FOREACH causes
> read from the freed pointer. Fixes the issue by using for loop instead
> of TAILQ_FOREACH.
>
> Coverity issue: 302867
> Fixes: bef50bcb1c47 ("net/softnic: implement start and stop")
>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> ---
> drivers/net/softnic/rte_eth_softnic_swq.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/softnic/rte_eth_softnic_swq.c
> b/drivers/net/softnic/rte_eth_softnic_swq.c
> index 1944fbb..a1f1899 100644
> --- a/drivers/net/softnic/rte_eth_softnic_swq.c
> +++ b/drivers/net/softnic/rte_eth_softnic_swq.c
> @@ -36,9 +36,11 @@ softnic_swq_free(struct pmd_internals *p)
> void
> softnic_softnic_swq_free_keep_rxq_txq(struct pmd_internals *p)
> {
> - struct softnic_swq *swq;
> + struct softnic_swq *swq, *swq_next;
> +
> + for (swq = TAILQ_FIRST(&p->swq_list); swq != NULL; swq = swq_next) {
> + swq_next = TAILQ_NEXT(swq, node);
>
> - TAILQ_FOREACH(swq, &p->swq_list, node) {
> if ((strncmp(swq->name, "RXQ", strlen("RXQ")) == 0) ||
> (strncmp(swq->name, "TXQ", strlen("TXQ")) == 0))
> continue;
The TAILQ_FOREACH_SAFE() macro handles exactly this case. Although it is not
in the linux TAILQ header, DPDK provides it in rte_tailq.h:
http://git.dpdk.org/dpdk/tree/lib/librte_eal/common/include/rte_tailq.h#n130
I think it is cleaner to use the MACRO instead of manually doing the loop,
linked-list iter + delete is error prone enough already :)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH] net/softnic: fix memory illegal access
2018-07-20 10:31 ` Van Haaren, Harry
@ 2018-07-20 10:49 ` Singh, Jasvinder
0 siblings, 0 replies; 6+ messages in thread
From: Singh, Jasvinder @ 2018-07-20 10:49 UTC (permalink / raw)
To: Van Haaren, Harry, dev; +Cc: Dumitrescu, Cristian
> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Friday, July 20, 2018 11:32 AM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/softnic: fix memory illegal access
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jasvinder Singh
> > Sent: Friday, July 20, 2018 10:45 AM
> > To: dev@dpdk.org
> > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Subject: [dpdk-dev] [PATCH] net/softnic: fix memory illegal access
> >
> > While deleting the elements from the linked list, TAILQ_FOREACH causes
> > read from the freed pointer. Fixes the issue by using for loop instead
> > of TAILQ_FOREACH.
> >
> > Coverity issue: 302867
> > Fixes: bef50bcb1c47 ("net/softnic: implement start and stop")
> >
> > Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> > Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > ---
> > drivers/net/softnic/rte_eth_softnic_swq.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/softnic/rte_eth_softnic_swq.c
> > b/drivers/net/softnic/rte_eth_softnic_swq.c
> > index 1944fbb..a1f1899 100644
> > --- a/drivers/net/softnic/rte_eth_softnic_swq.c
> > +++ b/drivers/net/softnic/rte_eth_softnic_swq.c
> > @@ -36,9 +36,11 @@ softnic_swq_free(struct pmd_internals *p) void
> > softnic_softnic_swq_free_keep_rxq_txq(struct pmd_internals *p) {
> > - struct softnic_swq *swq;
> > + struct softnic_swq *swq, *swq_next;
> > +
> > + for (swq = TAILQ_FIRST(&p->swq_list); swq != NULL; swq = swq_next) {
> > + swq_next = TAILQ_NEXT(swq, node);
> >
> > - TAILQ_FOREACH(swq, &p->swq_list, node) {
> > if ((strncmp(swq->name, "RXQ", strlen("RXQ")) == 0) ||
> > (strncmp(swq->name, "TXQ", strlen("TXQ")) == 0))
> > continue;
>
>
> The TAILQ_FOREACH_SAFE() macro handles exactly this case. Although it is not
> in the linux TAILQ header, DPDK provides it in rte_tailq.h:
>
> http://git.dpdk.org/dpdk/tree/lib/librte_eal/common/include/rte_tailq.h#n13
> 0
>
> I think it is cleaner to use the MACRO instead of manually doing the loop,
> linked-list iter + delete is error prone enough already :)
Macro lessens the confusion as well :) thanks, Harry!
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2] net/softnic: fix memory illegal access
2018-07-20 9:44 [dpdk-dev] [PATCH] net/softnic: fix memory illegal access Jasvinder Singh
2018-07-20 10:29 ` Bruce Richardson
2018-07-20 10:31 ` Van Haaren, Harry
@ 2018-07-20 11:05 ` Jasvinder Singh
2018-07-20 15:19 ` Ferruh Yigit
2 siblings, 1 reply; 6+ messages in thread
From: Jasvinder Singh @ 2018-07-20 11:05 UTC (permalink / raw)
To: dev; +Cc: cristian.dumitrescu
While deleting the elements from the linked list, TAILQ_FOREACH causes
read from the freed pointer. Fixes the issue by using TAILQ_FOREACH_SAFE
instead.
Coverity issue: 302867
Fixes: bef50bcb1c47 ("net/softnic: implement start and stop")
Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
drivers/net/softnic/rte_eth_softnic_swq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/softnic/rte_eth_softnic_swq.c b/drivers/net/softnic/rte_eth_softnic_swq.c
index 1944fbb..2083d0a 100644
--- a/drivers/net/softnic/rte_eth_softnic_swq.c
+++ b/drivers/net/softnic/rte_eth_softnic_swq.c
@@ -6,6 +6,7 @@
#include <string.h>
#include <rte_string_fns.h>
+#include <rte_tailq.h>
#include "rte_eth_softnic_internals.h"
@@ -36,9 +37,9 @@ softnic_swq_free(struct pmd_internals *p)
void
softnic_softnic_swq_free_keep_rxq_txq(struct pmd_internals *p)
{
- struct softnic_swq *swq;
+ struct softnic_swq *swq, *tswq;
- TAILQ_FOREACH(swq, &p->swq_list, node) {
+ TAILQ_FOREACH_SAFE(swq, &p->swq_list, node, tswq) {
if ((strncmp(swq->name, "RXQ", strlen("RXQ")) == 0) ||
(strncmp(swq->name, "TXQ", strlen("TXQ")) == 0))
continue;
--
2.9.3
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/softnic: fix memory illegal access
2018-07-20 11:05 ` [dpdk-dev] [PATCH v2] " Jasvinder Singh
@ 2018-07-20 15:19 ` Ferruh Yigit
0 siblings, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2018-07-20 15:19 UTC (permalink / raw)
To: Jasvinder Singh, dev; +Cc: cristian.dumitrescu
On 7/20/2018 12:05 PM, Jasvinder Singh wrote:
> While deleting the elements from the linked list, TAILQ_FOREACH causes
> read from the freed pointer. Fixes the issue by using TAILQ_FOREACH_SAFE
> instead.
>
> Coverity issue: 302867
> Fixes: bef50bcb1c47 ("net/softnic: implement start and stop")
>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread