[ovs-discuss] Possible double-free on ofproto.c:delete_flows_loose

[Anup Khadka]

On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com> wrote:

> It looks like OVS tries to double-free in delete_flows_loose if the
> rules->rules (inside struct rule_collection *rules is not equal to
> rules->stub).
>
> A little more detail:
> In the function delete_flows_loose, the call to the function
> collect_rules_loose takes care of freeing rules (again struct
> rule_collection *rules) if there is any error while collecting the rule.
>
> The function returns back to delete_flows_loose where it calls
> rule_collection_destroy again.
>
> Because rules->rules is still not rules->stab, it attempts to free the
> rules structure again, resulting in a double-free.
>
> Perhaps rules->rules can be set to rules->stab inside
> rule_collection_destroy function after its freed. Or perhaps,
> rule_collection_destroy should only be called from delete_flows_loose if
> there is no error, or perhaps collect_rules_loose should not take care of
> freeing the data structure.
>
> Please let me know if its a bug.
>
> Thanks,
> Anup
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/discuss/attachments/20140917/0b39233e/attachment.html>

[Ben Pfaff]

On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
> On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com> wrote:
>
> > It looks like OVS tries to double-free in delete_flows_loose if the
> > rules->rules (inside struct rule_collection *rules is not equal to
> > rules->stub).
> >
> > A little more detail:
> > In the function delete_flows_loose, the call to the function
> > collect_rules_loose takes care of freeing rules (again struct
> > rule_collection *rules) if there is any error while collecting the rule.
> >
> > The function returns back to delete_flows_loose where it calls
> > rule_collection_destroy again.
> >
> > Because rules->rules is still not rules->stab, it attempts to free the
> > rules structure again, resulting in a double-free.
> >
> > Perhaps rules->rules can be set to rules->stab inside
> > rule_collection_destroy function after its freed. Or perhaps,
> > rule_collection_destroy should only be called from delete_flows_loose if
> > there is no error, or perhaps collect_rules_loose should not take care of
> > freeing the data structure.

rule_collection_destroy() already reinitializes 'rules' after it
destroys it:

void
rule_collection_destroy(struct rule_collection *rules)
{
if (rules->rules != rules->stub) {
free(rules->rules);
}

/* Make repeated destruction harmless. */
rule_collection_init(rules);
}

[Anup Khadka]

Looks like this code was added in July:
https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51
3a63e2b68c
I was using an older code-base.
Thanks,
Anup

On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
> > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com>
> wrote:
> >
> > > It looks like OVS tries to double-free in delete_flows_loose if the
> > > rules->rules (inside struct rule_collection *rules is not equal to
> > > rules->stub).
> > >
> > > A little more detail:
> > > In the function delete_flows_loose, the call to the function
> > > collect_rules_loose takes care of freeing rules (again struct
> > > rule_collection *rules) if there is any error while collecting the
> rule.
> > >
> > > The function returns back to delete_flows_loose where it calls
> > > rule_collection_destroy again.
> > >
> > > Because rules->rules is still not rules->stab, it attempts to free the
> > > rules structure again, resulting in a double-free.
> > >
> > > Perhaps rules->rules can be set to rules->stab inside
> > > rule_collection_destroy function after its freed. Or perhaps,
> > > rule_collection_destroy should only be called from delete_flows_loose
> if
> > > there is no error, or perhaps collect_rules_loose should not take care
> of
> > > freeing the data structure.
>
> rule_collection_destroy() already reinitializes 'rules' after it
> destroys it:
>
> void
> rule_collection_destroy(struct rule_collection *rules)
> {
> if (rules->rules != rules->stub) {
> free(rules->rules);
> }
>
> /* Make repeated destruction harmless. */
> rule_collection_init(rules);
> }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/discuss/attachments/20140917/8c824b94/attachment-0001.html>

[Ben Pfaff]

According to the commit message, the bug could not cause a real problem
in practice. Do you see a way that it could?

On Wed, Sep 17, 2014 at 04:50:04PM -0400, Anup Khadka wrote:
> Looks like this code was added in July:
> https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51
> 3a63e2b68c
> I was using an older code-base.
> Thanks,
> Anup
>
> On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> > On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
> > > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com>
> > wrote:
> > >
> > > > It looks like OVS tries to double-free in delete_flows_loose if the
> > > > rules->rules (inside struct rule_collection *rules is not equal to
> > > > rules->stub).
> > > >
> > > > A little more detail:
> > > > In the function delete_flows_loose, the call to the function
> > > > collect_rules_loose takes care of freeing rules (again struct
> > > > rule_collection *rules) if there is any error while collecting the
> > rule.
> > > >
> > > > The function returns back to delete_flows_loose where it calls
> > > > rule_collection_destroy again.
> > > >
> > > > Because rules->rules is still not rules->stab, it attempts to free the
> > > > rules structure again, resulting in a double-free.
> > > >
> > > > Perhaps rules->rules can be set to rules->stab inside
> > > > rule_collection_destroy function after its freed. Or perhaps,
> > > > rule_collection_destroy should only be called from delete_flows_loose
> > if
> > > > there is no error, or perhaps collect_rules_loose should not take care
> > of
> > > > freeing the data structure.
> >
> > rule_collection_destroy() already reinitializes 'rules' after it
> > destroys it:
> >
> > void
> > rule_collection_destroy(struct rule_collection *rules)
> > {
> > if (rules->rules != rules->stub) {
> > free(rules->rules);
> > }
> >
> > /* Make repeated destruction harmless. */
> > rule_collection_init(rules);
> > }
> >

[Anup Khadka]

Yes, I got a crash with 100 rules which led me to inspect the code.

The collect_rule function inside collect_rules_loose returns
OFPROTO_POSTPONE if rule->pending is non-zero (this is possible if
the ofproto vendor class in not done inserting the rules).

In such cases, collect_rules_loose function will call a free on rules.

Without your commit in July, delete_flows_loose will call free again on the
rules structure.

This is what was happening in my code base too. I was using OVS 2.1.

Thanks,
Anup






On Wed, Sep 17, 2014 at 5:01 PM, Ben Pfaff <blp at nicira.com> wrote:

> According to the commit message, the bug could not cause a real problem
> in practice. Do you see a way that it could?
>
> On Wed, Sep 17, 2014 at 04:50:04PM -0400, Anup Khadka wrote:
> > Looks like this code was added in July:
> > https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51
> > 3a63e2b68c
> > I was using an older code-base.
> > Thanks,
> > Anup
> >
> > On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
> > > > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com>
> > > wrote:
> > > >
> > > > > It looks like OVS tries to double-free in delete_flows_loose if the
> > > > > rules->rules (inside struct rule_collection *rules is not equal to
> > > > > rules->stub).
> > > > >
> > > > > A little more detail:
> > > > > In the function delete_flows_loose, the call to the function
> > > > > collect_rules_loose takes care of freeing rules (again struct
> > > > > rule_collection *rules) if there is any error while collecting the
> > > rule.
> > > > >
> > > > > The function returns back to delete_flows_loose where it calls
> > > > > rule_collection_destroy again.
> > > > >
> > > > > Because rules->rules is still not rules->stab, it attempts to free
> the
> > > > > rules structure again, resulting in a double-free.
> > > > >
> > > > > Perhaps rules->rules can be set to rules->stab inside
> > > > > rule_collection_destroy function after its freed. Or perhaps,
> > > > > rule_collection_destroy should only be called from
> delete_flows_loose
> > > if
> > > > > there is no error, or perhaps collect_rules_loose should not take
> care
> > > of
> > > > > freeing the data structure.
> > >
> > > rule_collection_destroy() already reinitializes 'rules' after it
> > > destroys it:
> > >
> > > void
> > > rule_collection_destroy(struct rule_collection *rules)
> > > {
> > > if (rules->rules != rules->stub) {
> > > free(rules->rules);
> > > }
> > >
> > > /* Make repeated destruction harmless. */
> > > rule_collection_init(rules);
> > > }
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/discuss/attachments/20140917/6a81e70c/attachment.html>

[Ben Pfaff]

Thanks for reporting the bug.

I'll look into backporting the fix.

On Wed, Sep 17, 2014 at 2:17 PM, Anup Khadka <khadka.py at gmail.com> wrote:
> Yes, I got a crash with 100 rules which led me to inspect the code.
>
> The collect_rule function inside collect_rules_loose returns
> OFPROTO_POSTPONE if rule->pending is non-zero (this is possible if the
> ofproto vendor class in not done inserting the rules).
>
> In such cases, collect_rules_loose function will call a free on rules.
>
> Without your commit in July, delete_flows_loose will call free again on the
> rules structure.
>
> This is what was happening in my code base too. I was using OVS 2.1.
>
> Thanks,
> Anup
>
>
>
>
>
>
> On Wed, Sep 17, 2014 at 5:01 PM, Ben Pfaff <blp at nicira.com> wrote:
>>
>> According to the commit message, the bug could not cause a real problem
>> in practice. Do you see a way that it could?
>>
>> On Wed, Sep 17, 2014 at 04:50:04PM -0400, Anup Khadka wrote:
>> > Looks like this code was added in July:
>> > https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51
>> > 3a63e2b68c
>> > I was using an older code-base.
>> > Thanks,
>> > Anup
>> >
>> > On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:
>> >
>> > > On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
>> > > > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com>
>> > > wrote:
>> > > >
>> > > > > It looks like OVS tries to double-free in delete_flows_loose if
>> > > > > the
>> > > > > rules->rules (inside struct rule_collection *rules is not equal to
>> > > > > rules->stub).
>> > > > >
>> > > > > A little more detail:
>> > > > > In the function delete_flows_loose, the call to the function
>> > > > > collect_rules_loose takes care of freeing rules (again struct
>> > > > > rule_collection *rules) if there is any error while collecting the
>> > > rule.
>> > > > >
>> > > > > The function returns back to delete_flows_loose where it calls
>> > > > > rule_collection_destroy again.
>> > > > >
>> > > > > Because rules->rules is still not rules->stab, it attempts to free
>> > > > > the
>> > > > > rules structure again, resulting in a double-free.
>> > > > >
>> > > > > Perhaps rules->rules can be set to rules->stab inside
>> > > > > rule_collection_destroy function after its freed. Or perhaps,
>> > > > > rule_collection_destroy should only be called from
>> > > > > delete_flows_loose
>> > > if
>> > > > > there is no error, or perhaps collect_rules_loose should not take
>> > > > > care
>> > > of
>> > > > > freeing the data structure.
>> > >
>> > > rule_collection_destroy() already reinitializes 'rules' after it
>> > > destroys it:
>> > >
>> > > void
>> > > rule_collection_destroy(struct rule_collection *rules)
>> > > {
>> > > if (rules->rules != rules->stub) {
>> > > free(rules->rules);
>> > > }
>> > >
>> > > /* Make repeated destruction harmless. */
>> > > rule_collection_init(rules);
>> > > }
>> > >
>
>



--
"I don't normally do acked-by's. I think it's my way of avoiding
getting blamed when it all blows up." Andrew Morton

[Ben Pfaff]

I applied this commit to branch-2.1 and it will be in the next release
from that branch.

Sorry it took me so long to get back to this.

On Wed, Sep 17, 2014 at 05:17:17PM -0400, Anup Khadka wrote:
> Yes, I got a crash with 100 rules which led me to inspect the code.
>
> The collect_rule function inside collect_rules_loose returns
> OFPROTO_POSTPONE if rule->pending is non-zero (this is possible if
> the ofproto vendor class in not done inserting the rules).
>
> In such cases, collect_rules_loose function will call a free on rules.
>
> Without your commit in July, delete_flows_loose will call free again on the
> rules structure.
>
> This is what was happening in my code base too. I was using OVS 2.1.
>
> Thanks,
> Anup
>
>
>
>
>
>
> On Wed, Sep 17, 2014 at 5:01 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> > According to the commit message, the bug could not cause a real problem
> > in practice. Do you see a way that it could?
> >
> > On Wed, Sep 17, 2014 at 04:50:04PM -0400, Anup Khadka wrote:
> > > Looks like this code was added in July:
> > > https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51
> > > 3a63e2b68c
> > > I was using an older code-base.
> > > Thanks,
> > > Anup
> > >
> > > On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:
> > >
> > > > On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
> > > > > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com>
> > > > wrote:
> > > > >
> > > > > > It looks like OVS tries to double-free in delete_flows_loose if the
> > > > > > rules->rules (inside struct rule_collection *rules is not equal to
> > > > > > rules->stub).
> > > > > >
> > > > > > A little more detail:
> > > > > > In the function delete_flows_loose, the call to the function
> > > > > > collect_rules_loose takes care of freeing rules (again struct
> > > > > > rule_collection *rules) if there is any error while collecting the
> > > > rule.
> > > > > >
> > > > > > The function returns back to delete_flows_loose where it calls
> > > > > > rule_collection_destroy again.
> > > > > >
> > > > > > Because rules->rules is still not rules->stab, it attempts to free
> > the
> > > > > > rules structure again, resulting in a double-free.
> > > > > >
> > > > > > Perhaps rules->rules can be set to rules->stab inside
> > > > > > rule_collection_destroy function after its freed. Or perhaps,
> > > > > > rule_collection_destroy should only be called from
> > delete_flows_loose
> > > > if
> > > > > > there is no error, or perhaps collect_rules_loose should not take
> > care
> > > > of
> > > > > > freeing the data structure.
> > > >
> > > > rule_collection_destroy() already reinitializes 'rules' after it
> > > > destroys it:
> > > >
> > > > void
> > > > rule_collection_destroy(struct rule_collection *rules)
> > > > {
> > > > if (rules->rules != rules->stub) {
> > > > free(rules->rules);
> > > > }
> > > >
> > > > /* Make repeated destruction harmless. */
> > > > rule_collection_init(rules);
> > > > }
> > > >
> >

[samantha Andares]

Hi all,

Trying to find the reason why my packets are dropped on my last outgoing port.

I am receiving packets in an OVS bridge, sending those packets to another OVS bridge on the same node using patch-ports. Kind of the following:

__________________NODE A___________________ ___NODE B___
_______br_core______ _____br_access_______
eth0 --> patch-porteth0 ------> patch-porteth1 --> eth1 -----> eth1
192.168.168.50 192.168.167.50 192.168.167.51

I have a rule on my br_access that shows these packets hit the rule.... but rather than sending these packets out... they are all dropped. I use iperf to send udp packets and they are all dropped... Can someone help on showing me how to debug that thing? How can I log the reason why the drop counter increases. I don't even understand why it just blindly sends the packet out through that interface since the rule is saying it should.

The only reason why I created a br_access bridge was because I was not able to go out of the br_core to use normal ip routing..... I used a rule to force actions=NORMAL when receiving a certain packet with a dest ip in br_core.... but it failed.. so I created a patch bridge like shown below... but I'm still not able to get the packets I want.... I basically.. just want to go out of the OVS switch to use normal routing... can someone help on how to achieve this. Been blocked for some time now and my pressure is building up ;-)

thanks for your time,
samantha



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/discuss/attachments/20140917/2cc0581f/attachment.html>

[samantha Andares]

I just wanted to specify packets are dropped at port eth1 in br_access but I have no idea how to debug that... must have some debug available somewhere... but the logs in ovswitch don't tell me anything about the reasons why packets are dropped...

thanks for your time, I really need to fix that up...

From: sam.andares at hotmail.com
To: discuss at openvswitch.org
Date: Wed, 17 Sep 2014 17:04:55 -0400
Subject: [ovs-discuss] looking for why TX packets - or how to go out of OVS bridge for normal routing




Hi all,

Trying to find the reason why my packets are dropped on my last outgoing port.

I am receiving packets in an OVS bridge, sending those packets to another OVS bridge on the same node using patch-ports. Kind of the following:

__________________NODE A___________________ ___NODE B___
_______br_core______ _____br_access_______
eth0 --> patch-porteth0 ------> patch-porteth1 --> eth1 -----> eth1
192.168.168.50 192.168.167.50 192.168.167.51

I have a rule on my br_access that shows these packets hit the rule.... but rather than sending these packets out... they are all dropped. I use iperf to send udp packets and they are all dropped... Can someone help on showing me how to debug that thing? How can I log the reason why the drop counter increases. I don't even understand why it just blindly sends the packet out through that interface since the rule is saying it should.

The only reason why I created a br_access bridge was because I was not able to go out of the br_core to use normal ip routing..... I used a rule to force actions=NORMAL when receiving a certain packet with a dest ip in br_core.... but it failed.. so I created a patch bridge like shown below... but I'm still not able to get the packets I want.... I basically.. just want to go out of the OVS switch to use normal routing... can someone help on how to achieve this. Been blocked for some time now and my pressure is building up ;-)

thanks for your time,
samantha




_______________________________________________
discuss mailing list
discuss at openvswitch.org
http://openvswitch.org/mailman/listinfo/discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/discuss/attachments/20140917/242c5045/attachment.html>