[Qemu-devel] [PATCH] net: filter: correctly remove filter from the list during finalization

[Qemu-devel] [PATCH] net: filter: correctly remove filter from the list during finalization

[Jason Wang]

Qemu may crash when we want to add two filters on the same netdev but
the initialization of second fails (e.g missing parameters):

./qemu-system-x86_64 -netdev user,id=un0 \
 -object filter-buffer,id=f0,netdev=un0,interval=10 \
 -object filter-buffer,id=f1,netdev=un0
Segmentation fault (core dumped)

This is because we don't check whether or not the filter was in the
list of netdev. This patch fixes this.

Cc: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/filter.c b/net/filter.c
index d2a514e..7cdbc6c 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -196,7 +196,8 @@ static void netfilter_finalize(Object *obj)
         nfc->cleanup(nf);
     }
 
-    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters)) {
+    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters) &&
+        nf->next.tqe_prev) {
         QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
     }
     g_free(nf->netdev_id);
-- 
2.5.0

Re: [Qemu-devel] [PATCH] net: filter: correctly remove filter from the list during finalization

[Yang Hongyang]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

On Wed, Feb 24, 2016 at 11:44 AM, Jason Wang <jasowang@redhat.com> wrote:

> Qemu may crash when we want to add two filters on the same netdev but
> the initialization of second fails (e.g missing parameters):
>
> ./qemu-system-x86_64 -netdev user,id=un0 \
>  -object filter-buffer,id=f0,netdev=un0,interval=10 \
>  -object filter-buffer,id=f1,netdev=un0
> Segmentation fault (core dumped)
>
> This is because we don't check whether or not the filter was in the
> list of netdev. This patch fixes this.


Oops, thanks for catching this!


>
> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

---
>  net/filter.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/filter.c b/net/filter.c
> index d2a514e..7cdbc6c 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -196,7 +196,8 @@ static void netfilter_finalize(Object *obj)
>          nfc->cleanup(nf);
>      }
>
> -    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters)) {
> +    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters) &&
> +        nf->next.tqe_prev) {
>

Using queue's inner member tqe_prev directly might not be a good idea，but
seems there's no better way to do this.
Are there any chance that we could add a QTAILQ_XXX helper to check whether
a
member is in the queue or not?
Just some thoughts, I'm ok with the current patch though, so:

Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn>


>          QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
>      }
>      g_free(nf->netdev_id);
> --
> 2.5.0
>
>
>


-- 
Thanks,
Yang

[-- Attachment #2: Type: text/html, Size: 3135 bytes --]

Re: [Qemu-devel] [PATCH] net: filter: correctly remove filter from the list during finalization

[Jason Wang]

On 02/24/2016 07:53 PM, Yang Hongyang wrote:
>
> On Wed, Feb 24, 2016 at 11:44 AM, Jason Wang <jasowang@redhat.com
> <mailto:jasowang@redhat.com>> wrote:
>
>     Qemu may crash when we want to add two filters on the same netdev but
>     the initialization of second fails (e.g missing parameters):
>
>     ./qemu-system-x86_64 -netdev user,id=un0 \
>      -object filter-buffer,id=f0,netdev=un0,interval=10 \
>      -object filter-buffer,id=f1,netdev=un0
>     Segmentation fault (core dumped)
>
>     This is because we don't check whether or not the filter was in the
>     list of netdev. This patch fixes this.
>
>
> Oops, thanks for catching this!
>  
>
>
>     Cc: Yang Hongyang <hongyang.yang@easystack.cn
>     <mailto:hongyang.yang@easystack.cn>>
>     Signed-off-by: Jason Wang <jasowang@redhat.com
>     <mailto:jasowang@redhat.com>> 
>
>     ---
>      net/filter.c | 3 ++-
>      1 file changed, 2 insertions(+), 1 deletion(-)
>
>     diff --git a/net/filter.c b/net/filter.c
>     index d2a514e..7cdbc6c 100644
>     --- a/net/filter.c
>     +++ b/net/filter.c
>     @@ -196,7 +196,8 @@ static void netfilter_finalize(Object *obj)
>              nfc->cleanup(nf);
>          }
>
>     -    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters)) {
>     +    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters) &&
>     +        nf->next.tqe_prev) {
>
>
> Using queue's inner member tqe_prev directly might not be a good idea，but
> seems there's no better way to do this.
> Are there any chance that we could add a QTAILQ_XXX helper to check
> whether a
> member is in the queue or not?

Might be a good idea, but I'm not sure.

> Just some thoughts, I'm ok with the current patch though, so:
>
> Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn
> <mailto:hongyang.yang@easystack.cn>>

Applied to -net.

Thanks

>  
>
>              QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
>          }
>          g_free(nf->netdev_id);
>     --
>     2.5.0
>
>
>
>
>
> -- 
> Thanks,
> Yang