| Message ID | 1459494489-3532-1-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/01/2016 03:08 PM, zhanghailiang wrote: > After commit 338d3f, we support 'status' property for filter object. > The segfault can be triggered by starting qemu with 'status=off' property > for filter, when the s->incoming_queue is NULL, we reference it directly > in qemu_net_queue_flush(). > > Let's check the value of 's->incoming_queue' before calling > qemu_net_queue_flush(). > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > net/filter-buffer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/filter-buffer.c b/net/filter-buffer.c > index cc6bd94..79e2ce3 100644 > --- a/net/filter-buffer.c > +++ b/net/filter-buffer.c > @@ -34,7 +34,7 @@ static void filter_buffer_flush(NetFilterState *nf) > { > FilterBufferState *s = FILTER_BUFFER(nf); > > - if (!qemu_net_queue_flush(s->incoming_queue)) { > + if (s->incoming_queue && !qemu_net_queue_flush(s->incoming_queue)) { > /* Unable to empty the queue, purge remaining packets */ > qemu_net_queue_purge(s->incoming_queue, nf->netdev); > } We'd better handle this at generic layer and don't let a specific net filter need to worry about this. Looks like the issue is we may trigger status_changed() too early (even before the the filter was initialized). How about not call status_changed() if the initialization is not done?
On 2016/4/1 15:39, Jason Wang wrote: > > > On 04/01/2016 03:08 PM, zhanghailiang wrote: >> After commit 338d3f, we support 'status' property for filter object. >> The segfault can be triggered by starting qemu with 'status=off' property >> for filter, when the s->incoming_queue is NULL, we reference it directly >> in qemu_net_queue_flush(). >> >> Let's check the value of 's->incoming_queue' before calling >> qemu_net_queue_flush(). >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> --- >> net/filter-buffer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/filter-buffer.c b/net/filter-buffer.c >> index cc6bd94..79e2ce3 100644 >> --- a/net/filter-buffer.c >> +++ b/net/filter-buffer.c >> @@ -34,7 +34,7 @@ static void filter_buffer_flush(NetFilterState *nf) >> { >> FilterBufferState *s = FILTER_BUFFER(nf); >> >> - if (!qemu_net_queue_flush(s->incoming_queue)) { >> + if (s->incoming_queue && !qemu_net_queue_flush(s->incoming_queue)) { >> /* Unable to empty the queue, purge remaining packets */ >> qemu_net_queue_purge(s->incoming_queue, nf->netdev); >> } > > We'd better handle this at generic layer and don't let a specific net > filter need to worry about this. > > Looks like the issue is we may trigger status_changed() too early (even > before the the filter was initialized). > Yes ~ > How about not call status_changed() if the initialization is not done? > But seems that it is difficult to confirm if the filter is initialized or not ... > . >
On 04/01/2016 04:24 PM, Hailiang Zhang wrote: > On 2016/4/1 15:39, Jason Wang wrote: >> >> >> On 04/01/2016 03:08 PM, zhanghailiang wrote: >>> After commit 338d3f, we support 'status' property for filter object. >>> The segfault can be triggered by starting qemu with 'status=off' property >>> for filter, when the s->incoming_queue is NULL, we reference it directly >>> in qemu_net_queue_flush(). >>> >>> Let's check the value of 's->incoming_queue' before calling >>> qemu_net_queue_flush(). >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> --- >>> net/filter-buffer.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/filter-buffer.c b/net/filter-buffer.c >>> index cc6bd94..79e2ce3 100644 >>> --- a/net/filter-buffer.c >>> +++ b/net/filter-buffer.c >>> @@ -34,7 +34,7 @@ static void filter_buffer_flush(NetFilterState *nf) >>> { >>> FilterBufferState *s = FILTER_BUFFER(nf); >>> >>> - if (!qemu_net_queue_flush(s->incoming_queue)) { >>> + if (s->incoming_queue && !qemu_net_queue_flush(s->incoming_queue)) { >>> /* Unable to empty the queue, purge remaining packets */ >>> qemu_net_queue_purge(s->incoming_queue, nf->netdev); >>> } >> >> We'd better handle this at generic layer and don't let a specific net >> filter need to worry about this. >> >> Looks like the issue is we may trigger status_changed() too early (even >> before the the filter was initialized). >> > > Yes ~ > >> How about not call status_changed() if the initialization is not done? >> > > But seems that it is difficult to confirm if the filter is initialized > or not ... If nfc->setup() is not called, nf->netdev is NULL. Thanks Wen Congyang > >> . >> > > > > >
On 2016/4/1 17:10, Wen Congyang wrote: > On 04/01/2016 04:24 PM, Hailiang Zhang wrote: >> On 2016/4/1 15:39, Jason Wang wrote: >>> >>> >>> On 04/01/2016 03:08 PM, zhanghailiang wrote: >>>> After commit 338d3f, we support 'status' property for filter object. >>>> The segfault can be triggered by starting qemu with 'status=off' property >>>> for filter, when the s->incoming_queue is NULL, we reference it directly >>>> in qemu_net_queue_flush(). >>>> >>>> Let's check the value of 's->incoming_queue' before calling >>>> qemu_net_queue_flush(). >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> --- >>>> net/filter-buffer.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/filter-buffer.c b/net/filter-buffer.c >>>> index cc6bd94..79e2ce3 100644 >>>> --- a/net/filter-buffer.c >>>> +++ b/net/filter-buffer.c >>>> @@ -34,7 +34,7 @@ static void filter_buffer_flush(NetFilterState *nf) >>>> { >>>> FilterBufferState *s = FILTER_BUFFER(nf); >>>> >>>> - if (!qemu_net_queue_flush(s->incoming_queue)) { >>>> + if (s->incoming_queue && !qemu_net_queue_flush(s->incoming_queue)) { >>>> /* Unable to empty the queue, purge remaining packets */ >>>> qemu_net_queue_purge(s->incoming_queue, nf->netdev); >>>> } >>> >>> We'd better handle this at generic layer and don't let a specific net >>> filter need to worry about this. >>> >>> Looks like the issue is we may trigger status_changed() too early (even >>> before the the filter was initialized). >>> >> >> Yes ~ >> >>> How about not call status_changed() if the initialization is not done? >>> >> >> But seems that it is difficult to confirm if the filter is initialized >> or not ... > > If nfc->setup() is not called, nf->netdev is NULL. > Yes, you right, Jason, what's opinion ? Thanks, hailiang > Thanks > Wen Congyang > >> >>> . >>> >> >> >> >> >> > > > > > . >
On 04/01/2016 05:33 PM, Hailiang Zhang wrote: > On 2016/4/1 17:10, Wen Congyang wrote: >> On 04/01/2016 04:24 PM, Hailiang Zhang wrote: >>> On 2016/4/1 15:39, Jason Wang wrote: >>>> >>>> >>>> On 04/01/2016 03:08 PM, zhanghailiang wrote: >>>>> After commit 338d3f, we support 'status' property for filter object. >>>>> The segfault can be triggered by starting qemu with 'status=off' >>>>> property >>>>> for filter, when the s->incoming_queue is NULL, we reference it >>>>> directly >>>>> in qemu_net_queue_flush(). >>>>> >>>>> Let's check the value of 's->incoming_queue' before calling >>>>> qemu_net_queue_flush(). >>>>> >>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>>> --- >>>>> net/filter-buffer.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/net/filter-buffer.c b/net/filter-buffer.c >>>>> index cc6bd94..79e2ce3 100644 >>>>> --- a/net/filter-buffer.c >>>>> +++ b/net/filter-buffer.c >>>>> @@ -34,7 +34,7 @@ static void filter_buffer_flush(NetFilterState *nf) >>>>> { >>>>> FilterBufferState *s = FILTER_BUFFER(nf); >>>>> >>>>> - if (!qemu_net_queue_flush(s->incoming_queue)) { >>>>> + if (s->incoming_queue && >>>>> !qemu_net_queue_flush(s->incoming_queue)) { >>>>> /* Unable to empty the queue, purge remaining packets */ >>>>> qemu_net_queue_purge(s->incoming_queue, nf->netdev); >>>>> } >>>> >>>> We'd better handle this at generic layer and don't let a specific net >>>> filter need to worry about this. >>>> >>>> Looks like the issue is we may trigger status_changed() too early >>>> (even >>>> before the the filter was initialized). >>>> >>> >>> Yes ~ >>> >>>> How about not call status_changed() if the initialization is not done? >>>> >>> >>> But seems that it is difficult to confirm if the filter is initialized >>> or not ... >> >> If nfc->setup() is not called, nf->netdev is NULL. >> > > Yes, you right, Jason, what's opinion ? > > Thanks, > hailiang Looks good. Please also add a comment of this in the patch. Thanks > >> Thanks >> Wen Congyang >> >>> >>>> . >>>> >>> >>> >>> >>> >>> >> >> >> >> >> . >> > >
diff --git a/net/filter-buffer.c b/net/filter-buffer.c index cc6bd94..79e2ce3 100644 --- a/net/filter-buffer.c +++ b/net/filter-buffer.c @@ -34,7 +34,7 @@ static void filter_buffer_flush(NetFilterState *nf) { FilterBufferState *s = FILTER_BUFFER(nf); - if (!qemu_net_queue_flush(s->incoming_queue)) { + if (s->incoming_queue && !qemu_net_queue_flush(s->incoming_queue)) { /* Unable to empty the queue, purge remaining packets */ qemu_net_queue_purge(s->incoming_queue, nf->netdev); }
After commit 338d3f, we support 'status' property for filter object. The segfault can be triggered by starting qemu with 'status=off' property for filter, when the s->incoming_queue is NULL, we reference it directly in qemu_net_queue_flush(). Let's check the value of 's->incoming_queue' before calling qemu_net_queue_flush(). Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- net/filter-buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)