Note: This bug is displayed in read-only format because the product is no longer active in Red Hat Bugzilla.

The FDP team is no longer accepting new bugs in Bugzilla. Please report your issues under FDP project in Jira. Thanks.

Bug 1990130

Summary:	Be more assertive with tc_policy parameter
Product:	Red Hat Enterprise Linux Fast Datapath	Reporter:	Marcelo Ricardo Leitner <mleitner>
Component:	openvswitch	Assignee:	Michael Santana <msantana>
openvswitch sub component:	ovs-hw-offload	QA Contact:	qding
Status:	CLOSED EOL	Docs Contact:
Severity:	medium
Priority:	medium	CC:	ctrautma, fleitner, mhou, msantana, qding
Version:	FDP 21.F
Target Milestone:	---
Target Release:	FDP 21.G
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2024-10-08 17:49:14 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	2172625

Description Marcelo Ricardo Leitner 2021-08-04 19:55:02 UTC

Description of problem:
It currently accepts any value, and when it fails to parse it, it assumes "none" (because NONE is 0).

It is probably aggravated by the switching between "-" in "hw-offload" to "_" in "skip_X".

There is a warning in there, but it can easily go unnoticed like in
https://bugzilla.redhat.com/show_bug.cgi?id=1975085#c22
(This is not the first time I've seen this happening)

AFAIK we need ovs running to change that value, so we can't have ovs fail to start. But it would be good if we could improve this somehow. Perhaps block the "set" command to only known values?

  void
  tc_set_policy(const char *policy)
  {
      if (!policy) {
          return;
      }

      if (!strcmp(policy, "skip_sw")) {
          tc_policy = TC_POLICY_SKIP_SW;
      } else if (!strcmp(policy, "skip_hw")) {
          tc_policy = TC_POLICY_SKIP_HW;
      } else if (!strcmp(policy, "none")) {
          tc_policy = TC_POLICY_NONE;
      } else {
          VLOG_WARN("tc: Invalid policy '%s'", policy);
          return;
      }

      VLOG_INFO("tc: Using policy '%s'", policy);
  }

Comment 4 Marcelo Ricardo Leitner 2022-09-30 18:50:11 UTC

Any news here?

Comment 6 Michael Santana 2023-01-07 19:27:14 UTC

Hi,

Yes, I would love to make this a bit more strict with the arguments it takes in. The issue does not lie alone with the tc policy argument. It happens in all other_config configurations


A typical ovs set command looks like this
ovs-vsctl set Open_vSwitch . other_config:KEY=VALUE

The problem is that KEY and VALUE can be any arbitrary string and ovs-vsctl will happily accept it and not throw any warnings. Ideally ovs-vsctl should look up KEY and make sure it is valid and then make sure VALUE is valid for that particular KEY, otherwise throw an error.

We know that the "set" command does do some blocking, i.g. if you misspell "other_config" in the above command you will get an error about it being an invalid column. other_config is so well documented at this point with all the key:value pairs that there shouldnt be any reason why we couldnt be any strict about it

Let me look how to implement this in the code. We might need to make a lookup table with all the key:value pairs

Comment 7 Michael Santana 2023-05-24 16:33:06 UTC

*** Bug 2208960 has been marked as a duplicate of this bug. ***

Comment 8 Flavio Leitner 2024-03-11 19:10:55 UTC

*** Bug 2203140 has been marked as a duplicate of this bug. ***

Comment 9 ovs-bot 2024-10-08 17:49:14 UTC

This bug did not meet the criteria for automatic migration and is being closed.
If the issue remains, please open a new ticket in https://issues.redhat.com/browse/FDP