On 10/27/25 11:16 PM, Frode Nordahl wrote:
> On 10/27/25 19:40, Ilya Maximets wrote:
>> On 10/24/25 4:28 PM, Frode Nordahl wrote:
>>> The commit in the fixes tag extended struct dpif_execute with an
>>> upcall_pid member.
>>>
>>> This member was left uninitialized in dpif_execute_helper_cb(),
>>> leading to Open vSwitch providing the kernel with a PID for which
>>> it does not have a listener.
>>>
>>> This condition is caught by one of the existing system-traffic
>>> tests which is extended to explicitly check for presence of lost
>>> upcalls in the datapath.
>>>
>>> Fixes: 0d9dc8e9ca4a ("dpif-netlink: Provide original upcall pid in
>>> 'execute' commands.")
>>> Reported-at: https://launchpad.net/bugs/2127061
>>> Signed-off-by: Frode Nordahl <[email protected]>
>>> ---
>>> lib/dpif.c | 1 +
>>> tests/system-traffic.at | 2 ++
>>> 2 files changed, 3 insertions(+)
>>
>> Good catch, Frode! Thanks for the patch.
>
> Thank you for taking the time to review, Ilya, much appreciated!
>
>>>
>>> Wanted to note that retis was instrumental in tracking down the
>>> root cause if this issue, very useful tool!
>>>
>>> I also wanted to note that I spent considerable amonut of time
>>> trying to figure out a low touch way of creating a specifc test
>>> for this, but did not come up with any good alternatives.
>>>
>>> I appear to somehow have missed that an existing system test
>>> catches the condition, so I went ahead and expanded on that
>>> in the end.
>>
>> Hmm. I can't reproduce the problem with this particular test.
>> There should be no upcalls after the packet is re-injected via
>> the 'execute' request. And also the pcap file check should be
>> failing if the packet was dropped. Could you explain a bit more
>> what you see in your test run?
>
> I identified this particular test as interesting by running the entire
> testsuite with retis and looking for evidence of a -111 return for
> upcalls in the kernel code.
>
> Rechecking this locally I see a difference in behavior depending on
> which compiler is used to build Open vSwitch, specifically I see the
> condition when compiling with gcc 14.2.0.
>
> Recompiling with gcc 15.2.0 on otherwise identical system makes the test
> pass. I can only guess there is a difference in behavior for how the
> uninitialized value is treated.
>
> In any case, we cannot rely on compiler behavior for our test case, so I
> guess we need to find some other way of testing this. Any suggestions
> welcome :-)
One option, I believe, could be to have packet go through conntrack twice
(different zones) and add debug_slow into OpenFlow rules to force packets
to userspace after each recirculation. That should end up with the packets
dropped. At the same time it's still a gamble as we're relying on values
of the uninitialized memory either way. So, not particularly reliable.
We may omit the test for this issue entirely, since valgrind is actually
reporting the problem consistently while running this mpls test in
check-kernel-valgrind:
Thread 27 handler71:
Conditional jump or move depends on uninitialised value(s)
at 0x55986A: dpif_netlink_encode_execute (dpif-netlink.c:2028)
by 0x55986A: dpif_netlink_operate__ (dpif-netlink.c:2112)
by 0x559B75: dpif_netlink_operate_chunks (dpif-netlink.c:2428)
by 0x559B75: dpif_netlink_operate (dpif-netlink.c:2487)
by 0x48BDD9: dpif_operate (dpif.c:1370)
by 0x48C386: dpif_execute (dpif.c:1324)
by 0x48C386: dpif_execute (dpif.c:1314)
by 0x48C386: dpif_execute_helper_cb (dpif.c:1243)
by 0x4C277A: odp_execute_actions (odp-execute.c:1074)
by 0x489AE0: dpif_execute_with_help (dpif.c:1299)
by 0x48BD8A: dpif_operate (dpif.c:1435)
by 0x446A8F: handle_upcalls (ofproto-dpif-upcall.c:1746)
by 0x446A8F: recv_upcalls (ofproto-dpif-upcall.c:962)
by 0x446C1E: udpif_upcall_handler (ofproto-dpif-upcall.c:857)
by 0x5163C4: ovsthread_wrapper (ovs-thread.c:426)
by 0x51CB199: start_thread (pthread_create.c:443)
by 0x524F533: clone (clone.S:100)
Uninitialised value was created by a stack allocation
at 0x48C130: dpif_execute_helper_cb (dpif.c:1176)
Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
at 0x5250F4D: __libc_sendmsg (sendmsg.c:28)
by 0x5250F4D: sendmsg (sendmsg.c:25)
by 0x57355D: nl_sock_transact_multiple__ (netlink-socket.c:874)
by 0x573955: nl_sock_transact_multiple.part.0 (netlink-socket.c:1097)
by 0x574ADF: nl_sock_transact_multiple (netlink-socket.c:1062)
by 0x574ADF: nl_transact_multiple (netlink-socket.c:1862)
by 0x55993A: dpif_netlink_operate__ (dpif-netlink.c:2141)
by 0x559B75: dpif_netlink_operate_chunks (dpif-netlink.c:2428)
by 0x559B75: dpif_netlink_operate (dpif-netlink.c:2487)
by 0x48BDD9: dpif_operate (dpif.c:1370)
by 0x48C386: dpif_execute (dpif.c:1324)
by 0x48C386: dpif_execute (dpif.c:1314)
by 0x48C386: dpif_execute_helper_cb (dpif.c:1243)
by 0x4C277A: odp_execute_actions (odp-execute.c:1074)
by 0x489AE0: dpif_execute_with_help (dpif.c:1299)
by 0x48BD8A: dpif_operate (dpif.c:1435)
by 0x446A8F: handle_upcalls (ofproto-dpif-upcall.c:1746)
by 0x446A8F: recv_upcalls (ofproto-dpif-upcall.c:962)
by 0x446C1E: udpif_upcall_handler (ofproto-dpif-upcall.c:857)
by 0x5163C4: ovsthread_wrapper (ovs-thread.c:426)
by 0x51CB199: start_thread (pthread_create.c:443)
by 0x524F533: clone (clone.S:100)
Address 0xfeefdec is on thread 27's stack
in frame #4, created by dpif_netlink_operate__ (dpif-netlink.c:2039)
Uninitialised value was created by a stack allocation
at 0x48C130: dpif_execute_helper_cb (dpif.c:1176)
The tests are painfully slow, so we're not running them in CI. But we
really should be running them at least once per release or something.
I'll make a note for myself to do that.
>
>>>
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 070fc0131..a062a4179 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1240,6 +1240,7 @@ dpif_execute_helper_cb(void *aux_, struct
>>> dp_packet_batch *packets_,
>>> execute.probe = false;
>>> execute.mtu = 0;
>>> execute.hash = 0;
>>> + execute.upcall_pid = 0;
>>
>> Thinking a bit more about the problem, I think, we should be
>> getting the original upcall pid for this request. We have it
>> in the 'execute' structure before we're trying to execute with
>> help. We can store the value in the aux data and set it here.
>> The same as we do for the 'flow'. This will ensure that we're
>> keeping the upcall pid regardless if the execution is going
>> direct to kernel or some actions are executed in the userspace
>> first.
>
> Ack, I'll address this in a v2.
>
I have a vague memory of already suggesting this somewhere, but I don't
remember where... We seem to have a similar problem with the mtu and
the hash that are cleared, while they probably shouldn't be. So, what
we could do here is to store the entire original 'execute' in the aux,
then copy it and only update fields that need to be updated, i.e.
actions, packet and the needs_help. The rest should stay the same as
it was in the original request.
WDYT?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
