https://github.com/openvswitch/ovs/blob/69f0acc9af7e480e81c8ce002fe2dfa9c378d13b/datapath-windows/ovsext/Flow.c#L119-L120
Indeed I see it as optional for the nlFlowKeyPolicy, but I think we are
interested in: OVS_PACKET_ATTR_KEY/odp_key_from_dp_packet
https://github.com/openvswitch/ovs/blob/main/lib/odp-util.c#L6676-L6680
which seems has not changed for quite a while...
Unfortunately I don't have the cycles to dig into this further and since
this avoids a BSOD under certain circumstances I will add a todo on top
and merge the code.
Thank you the patch Frank and for driving the review Mike!
Alin.
On 3/10/2025 4:21 AM, Mike Pattrick wrote:
On Sat, Mar 8, 2025 at 6:07 AM Alin Serdean <alinserd...@gmail.com> wrote:
AFAIR the assumption was that attribute can't be empty since it always has to
be written by the userspace and validated prior.
In odp_flow_key_from_flow__() the value may not be written. And making
this an optional parameter matches the linux kernel module at
metadata_from_nlattrs().
Cheers,
M
The assert was there under debug to see if the userspace does not set it during
the initial porting.
That being said there is nothing wrong in adding the check, it just irks me
that this might hide a bigger issue.
Will try to reproduce it today/tomorrow and try to understand the issue better.
On Thu, Mar 6, 2025 at 7:51 PM Mike Pattrick <m...@redhat.com> wrote:
On Sat, Mar 1, 2025 at 7:35 AM Frank Wagner <frank.wag...@dbosoft.eu> wrote:
It can happen that ovs key attributes are not in keyAttrs of port.
In this case the call of NlAttrGetU32 will cause a BSOD in Release builds.
Signed-off-by: Frank Wagner <frank.wag...@dbosoft.eu>
---
datapath-windows/ovsext/User.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
index c4563b28b..b52124abf 100644
--- a/datapath-windows/ovsext/User.c
+++ b/datapath-windows/ovsext/User.c
@@ -407,7 +407,9 @@ _MapNlAttrToOvsPktExec(PNL_MSG_HDR nlMsgHdr, PNL_ATTR
*nlAttrs,
execute->actionsLen = NlAttrGetSize(nlAttrs[OVS_PACKET_ATTR_ACTIONS]);
ASSERT(keyAttrs[OVS_KEY_ATTR_IN_PORT]);
- execute->inPort = NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_IN_PORT]);
+ if (keyAttrs[OVS_KEY_ATTR_IN_PORT]) {
+ execute->inPort = NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_IN_PORT]);
+ }
After reading through the code a bit more, I believe this is nearly
correct, but the ASSERT should be removed.
However, I don't have a Windows kernel dev environment set up, so I
can't test this out.
Cheers,
M
execute->keyAttrs = keyAttrs;
if (nlAttrs[OVS_PACKET_ATTR_MRU]) {
--
2.48.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
