Hi Ilya, I had a look at the patch that you have submitted. In push_tnl_action, I can see that with the patch you are trying to return error in case we don’t find the tunnel port or return the error that netdev_push header returns. And we are deciding to delete the batch in the calling function dp_execute_cb in case of error. But netdev_push_header always returns 0. Actually the following code in push_tnl_action is dead code.
if (!err) { return 0; } The actual issue is that the push_header function always return void . So any error happening during push_header is never captured or acted upon. I think the fix also be should be to add/capture error scenarios during tnl header push as well. Regards & Thanks Anju -----Original Message----- From: Ilya Maximets [mailto:i.maxim...@samsung.com] Sent: Monday, May 14, 2018 6:28 PM To: ovs-dev@openvswitch.org; Anju Thomas <anju.tho...@ericsson.com>; Ben Pfaff <b...@ovn.org> Cc: Tiago Lam <tiago....@intel.com> Subject: Re: [ovs-dev] [PATCH v1] Fix crash due to multiple tnl push action Hello Anju, Ben, Looks like I fixed a leak reported here in my recent patch: https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346892.html Could you, please, take a look at it? I actually managed to reproduce the packet leak on tunnel-push-pop unit tests with valgrind: ==6445== 2,912 (2,208 direct, 704 indirect) bytes in 4 blocks are definitely lost in loss record 400 of 410 ==6445== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==6445== by 0x516314: xmalloc (util.c:120) ==6445== by 0x466154: dp_packet_new (dp-packet.c:138) ==6445== by 0x466154: dp_packet_new_with_headroom (dp-packet.c:148) ==6445== by 0x4F6CFE: eth_from_hex (packets.c:498) ==6445== by 0x48EB43: eth_from_packet (netdev-dummy.c:1450) ==6445== by 0x48EB43: netdev_dummy_receive (netdev-dummy.c:1562) ==6445== by 0x515980: process_command (unixctl.c:313) ==6445== by 0x515980: run_connection (unixctl.c:347) ==6445== by 0x515980: unixctl_server_run (unixctl.c:398) ==6445== by 0x406F1E: main (ovs-vswitchd.c:120) Above patch fixes it. Best regards, Ilya Maximets. > Hi Ben, > > I will work on these changes as well. > > Regards > Anju > > -----Original Message----- > From: Ben Pfaff [mailto:blp at ovn.org] > Sent: Friday, May 11, 2018 2:00 AM > To: Anju Thomas <anju.thomas at ericsson.com> > Cc: dev at openvswitch.org > Subject: Re: [ovs-dev] [PATCH v1] Fix crash due to multiple tnl push > action > > On Thu, May 10, 2018 at 08:51:03AM +0000, Anju Thomas wrote: >> It looks like your commit message describes at least two other bugs >> in OVS, though. First, if OVS crashes when it pushes tunnel headers, >> even if there's not enough headroom, that's really bad. At worst, it >> should drop the packet. Do you know where the crash occurs? We >> should fix the problem, since it might recur in some other context. >> >> [Anju] OVS was actually crashing in the dpdk datapath. The crash is a manual >> assert in our case. >> The rootcause is that dp receives the actions after the upcall (say with >=3 >> tunnel pushes ) . Now as part of action processing , since it is a tunnel >> push action , we try to find space in the dpdk mbuf packet headroom (which >> Is 128 bytes). By the time we try to process the third tunnel push , there >> is no headroom left since each tunnel header is of 50 bytes (50 *3 > 128 >> bytes headroom). Hence it manually asserts . This assert is to catch any >> unexpected code flow . Do you think that in this case we should still go >> ahead and prevent the crash ? > > I don't understand why it's OK to crash in this case. Why do you think so? > >> Second, it's a little shocking to hear that an encapsulation action without >> a following output action causes a memory leak. We also need to fix that. >> Do you have any more details? >> [Anju] Now as explained above, the crash happens because we run out of >> headroom. But in case we have say 2 or less than 2 tunnel pushes we will >> have a mem leak as packet is never freed because the tnl push is the dp last >> action and there is no other output action or any other action like recirc >> that may translate to output action in the end leading to packet buffer not >> being freed. >> Are you proposing that we have some sort of preventive fix in the dp to >> handle an incorrect action list from the upcall handling? > > Yes. It's unacceptable to leak memory because there's a packet modification > without an output action. The kernel datapath never does this, for example. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev