[PWCI] "[ovs-dev] ovsdb-idl: Add a way to assert IDL txn as read only."#17
[PWCI] "[ovs-dev] ovsdb-idl: Add a way to assert IDL txn as read only."#17
Conversation
Moving Russell to emeritus status at his request. Thanks, Russell, for all your contributions and support! Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
OVS adds matches on the DF/CSUM/etc bits of the tunnel info flags, but the DF bit is never actually extracted from the outer IP header during the tunnel decapsulation. This is not a huge problem, as we'll only match on what was parsed out, since matching on DF flag is not exposed through OpenFlow to the users. And since the bit is never extracted, we'll just always match on -df, which sort of "works", because the bit is never extracted and so it is never set. However, this causes misleading -df matches in the datapath flow dumps even if the packets actually have the DF bit set, which it is by default. Fix that by actually extracting the bit from the outer header while decapsulating tunneled traffic. Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Add a way to assert IDL txn as read only, that is useful in cases when we want to ensure that the program doesn't write anything into the txn. It is done via assert because this is considered a bug which should be fixed in the IDL caller. Signed-off-by: Ales Musil <amusil@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideThe patch adds a read-only assertion mechanism to ovsdb-idl transactions by introducing a flag and injecting assertion checks into all mutating operations, extends the test suite to drive this flag, and includes minor fixes in the native tunnel parsing code to handle the IPv4 DF bit and preserve existing GRE checksum flags. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe changes add an IDL transaction read-only assertion API and wire it into tests, adjust GRE/Geneve/NSH tunnel flag handling and representations (notably DF and CSUM), update corresponding test expectations, and modify MAINTAINERS.rst to move a committer to emeritus. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as test-ovsdb (CLI)
participant I as OVSDB IDL
participant T as IDL Transaction
participant S as Write Path (insert/set/delete)
U->>I: idl_set(..., assert_read_only=true)
I->>T: ovsdb_idl_txn_create()
I->>T: ovsdb_idl_txn_assert_read_only(T, true)
U->>S: attempt write (e.g., insert/set/delete)
S->>T: check T.assert_read_only
alt read-only asserted
S-->>U: abort via assertion (write forbidden)
else not asserted
S->>I: proceed with write
I-->>U: normal completion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There's a typo in the test code:
aseert_read_onlyshould beassert_read_only. - The PR currently mixes unrelated OVSDB IDL changes with netdev-native-tnl fixes—consider splitting the netdev patches into a separate commit or PR.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There's a typo in the test code: `aseert_read_only` should be `assert_read_only`.
- The PR currently mixes unrelated OVSDB IDL changes with netdev-native-tnl fixes—consider splitting the netdev patches into a separate commit or PR.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
lib/ovsdb-idl.h (1)
388-389: Rename parameter and document API intent.Minor clarity nit: avoid using “assert” as a parameter name and add a short comment to state this is a debug-time assertion (no-op with NDEBUG).
Proposed tweak:
-void ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *txn, bool assert); +/* Enables a debug-only assertion that the transaction remains read-only. + * Has no effect in release builds compiled with NDEBUG. */ +void ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *txn, bool enable);(Please also update the .c implementation and any call sites accordingly.)
tests/ovsdb-idl.at (1)
3005-3046: Good coverage for read-only mode (insert/set/delete).The tests correctly expect an assertion failure and ignore exit status, then grep stderr. Consider guarding these with a skip if asserts are disabled (NDEBUG) to avoid false negatives in release builds. Also, you could match a slightly looser regex to reduce fragility across assert formatting changes.
Example adjustments:
- Skip on non-assert builds (pseudo-example, adapt to your build env):
AT_SKIP_IF([test "x$OVS_ENABLE_ASSERTS" = "xno"])
- Loosen grep to tolerate minor message format changes:
-AT_CHECK([grep -qE "assertion !.*txn->assert_read_only failed" stderr], [0]) +AT_CHECK([grep -qE "assert.*read[_ ]only.*failed" stderr], [0])lib/ovsdb-idl.c (7)
2841-2846: Asserting increment in RO mode is fine; add a clearer guard.Minor: assert the presence of a txn before dereferencing, then assert RO, for symmetry with other write paths.
Apply:
void ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn, const struct ovsdb_idl_row *row, const struct ovsdb_idl_column *column, bool force) { @@ - ovs_assert(!txn->assert_read_only); + ovs_assert(txn); + ovs_assert(!txn->assert_read_only);
3653-3658: Avoid potential NULL-deref chain; assert txn then RO.Defensive check before
row->table->idl->txn->....Apply:
- ovs_assert(!row->table->idl->txn->assert_read_only); + ovs_assert(row->table && row->table->idl && row->table->idl->txn); + ovs_assert(!row->table->idl->txn->assert_read_only);
3839-3843: Same guard for delete path.Mirror the write guard pattern.
Apply:
- ovs_assert(!is_index_row(row_)); - ovs_assert(!row->table->idl->txn->assert_read_only); + ovs_assert(!is_index_row(row_)); + ovs_assert(row->table && row->table->idl && row->table->idl->txn); + ovs_assert(!row->table->idl->txn->assert_read_only);
3868-3873: Same guard for insert path.Short, consistent guard improves readability.
Apply:
- ovs_assert(uuid || !persist_uuid); - ovs_assert(!txn->assert_read_only); + ovs_assert(uuid || !persist_uuid); + ovs_assert(txn); + ovs_assert(!txn->assert_read_only);
4230-4232: Same guard for partial map ops.Defensive assert before deref.
Apply:
- ovs_assert(!row->table->idl->txn->assert_read_only); + ovs_assert(row->table && row->table->idl && row->table->idl->txn); + ovs_assert(!row->table->idl->txn->assert_read_only);
4268-4270: Same guard for partial set ops.Keep guards uniform across write helpers.
Apply:
- ovs_assert(!row->table->idl->txn->assert_read_only); + ovs_assert(row->table && row->table->idl && row->table->idl->txn); + ovs_assert(!row->table->idl->txn->assert_read_only);
4580-4589: Rename parameter to avoid shadowing common macro name.Using “assert” as a parameter name is brittle. Prefer “enable”.
Apply:
-void -ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *txn, bool assert) +void +ovsdb_idl_txn_assert_read_only(struct ovsdb_idl_txn *txn, bool enable) { if (txn) { - txn->assert_read_only = assert; + txn->assert_read_only = enable; } }Note: mirror the rename in the declaration and call sites.
tests/test-ovsdb.c (6)
61-62: Fix typo in flag name (aseert_read_only → assert_read_only).Keeps the option readable and consistent.
Apply:
- bool aseert_read_only; + bool assert_read_only;
97-99: Use no_argument for --assert-read-only.The option doesn’t take a value; optional_argument is misleading.
Apply:
- {"assert-read-only", optional_argument, NULL, 'r'}, + {"assert-read-only", no_argument, NULL, 'r'},
136-139: Align field name with the typo fix.Update assignment accordingly.
Apply:
- case 'r': - pvt->aseert_read_only = true; + case 'r': + pvt->assert_read_only = true; break;
2919-2922: Align call site with field rename.Pass the corrected field name.
Apply:
- if (!idl_set(idl, arg, step++, pvt->aseert_read_only)) { + if (!idl_set(idl, arg, step++, pvt->assert_read_only)) {
160-265: Add CLI help for --assert-read-only.Expose the new flag in usage().
You can append this line in the “Other options” section:
printf(" -r, --assert-read-only assert IDL transactions are read-only (debug builds)\n");
85-158: Optional: initialize new context fields explicitly.Not required (designated init zeros others), but explicit init aids clarity.
Apply:
- struct test_ovsdb_pvt_context pvt = {.track = false}; + struct test_ovsdb_pvt_context pvt = { + .write_changed_only = false, + .track = false, + .assert_read_only = false, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
MAINTAINERS.rst(1 hunks)lib/netdev-native-tnl.c(2 hunks)lib/ovsdb-idl.c(9 hunks)lib/ovsdb-idl.h(1 hunks)tests/nsh.at(2 hunks)tests/ovsdb-idl.at(1 hunks)tests/packet-type-aware.at(8 hunks)tests/test-ovsdb.c(6 hunks)tests/tunnel-push-pop.at(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lib/netdev-native-tnl.c (1)
include/sparse/netinet/in.h (1)
htons(158-161)
lib/ovsdb-idl.h (1)
lib/ovsdb-idl.c (1)
ovsdb_idl_txn_assert_read_only(4583-4589)
tests/test-ovsdb.c (2)
tests/test-ovsdb.py (1)
idl_set(450-727)lib/ovsdb-idl.c (1)
ovsdb_idl_txn_assert_read_only(4583-4589)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: linux gcc dpdk check-afxdp
- GitHub Check: linux clang dpdk-shared
- GitHub Check: linux gcc m32 --disable-ssl
- GitHub Check: linux gcc dpdk --enable-shared
- GitHub Check: linux gcc --enable-afxdp
- GitHub Check: linux clang test --enable-shared
- GitHub Check: linux clang check check-dpdk dpdk
- GitHub Check: linux gcc test -ljemalloc
- GitHub Check: linux gcc test
- GitHub Check: linux clang c99
- GitHub Check: linux clang address,undefined test
- GitHub Check: linux clang test -ljemalloc
- GitHub Check: linux gcc test --enable-shared
- GitHub Check: linux gcc check check-dpdk dpdk
- GitHub Check: linux gcc c99
- GitHub Check: linux gcc --disable-ssl
- GitHub Check: clang-analyze-cache
- GitHub Check: linux deb no dpdk
- GitHub Check: linux rpm fedora
- GitHub Check: build oss-fuzz fuzzers
- GitHub Check: libreswan (ubuntu-24.04)
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-14-3-snap
🔇 Additional comments (11)
MAINTAINERS.rst (1)
94-95: Move to Emeritus looks correct and ordering is consistent.Russell Bryant is removed from Active and added under Emeritus; placement fits the (first-name) alphabetical order and email formatting matches surrounding entries.
tests/tunnel-push-pop.at (1)
688-689: Test expectations updated to reflect DF propagation — looks good.The switches from flags(-df-csum+key) to flags(+df-csum+key) align with DF being carried into tunnel metadata and GRE/Geneve parsing preserving existing flags. Expectations match the code changes.
Also applies to: 722-724, 776-779
lib/netdev-native-tnl.c (2)
155-158: Correctly propagate IPv4 DF into tunnel flags.Using htons(IP_DF) against ip_frag_off and OR-ing FLOW_TNL_F_DONT_FRAGMENT is the right approach; integrates cleanly with header build, which sets IP_DF based on tnl flags.
474-475: Preserve existing flags when GRE CSUM present — good fix.Changing from assignment to
|=avoids clobbering DF or previously set bits from ip_extract_tnl_md.tests/nsh.at (1)
1-3: No annotated changes to review in this file.Proceeding without comments.
tests/packet-type-aware.at (1)
327-327: GRE tunnel flags expectations updated correctly (+df-csum).The new dump-flow expectations align with preserving DF and GRE CSUM in tunnel metadata. Looks consistent across L2 and L3 paths and through recirc. No further action.
Also applies to: 345-345, 363-363, 381-382, 400-401, 419-419, 1018-1019
lib/ovsdb-idl.h (1)
388-389: Ensure ovsdb_idl_txn_assert_read_only is exported in platform symbol listsPrototype: lib/ovsdb-idl.h:388; Definition: lib/ovsdb-idl.c:4584. No libopenvswitch.sym/.def found under lib/ in this check — confirm or add ovsdb_idl_txn_assert_read_only to your export list(s) (libopenvswitch.sym/.def or platform equivalents) so test-ovsdb links on all platforms.
lib/ovsdb-idl.c (2)
114-115: Good addition: per-txn read-only flag.Field name and placement are consistent with other txn state.
2775-2776: Initializer added correctly.Ensures default RW behavior unless explicitly asserted RO.
tests/test-ovsdb.c (2)
2468-2482: Good: RO assertion hooked into idl_set txn.This correctly enforces RO per txn when enabled.
2468-2654: Optional: gate RO assertion to only when writes are attempted — no change required. Current approach asserts at write sites; leave as-is. Verification attempt didn’t run (ripgrep: "No files were searched") — re-run the typo check to confirm no misspellings.
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
3 participants
NOTE: This is an auto submission for "[ovs-dev] ovsdb-idl: Add a way to assert IDL txn as read only.".
See "http://patchwork.ozlabs.org/project/openvswitch/list/?series=473167" for details.
Summary by Sourcery
Add a new mechanism to mark ovsdb IDL transactions as read-only with assertions to prevent accidental writes, update tests and CLI to exercise this mode, and correct tunnel flag handling in the native tunnel and GRE parsers.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation