[PATCH iproute2 v2] tc/skbmod: Remove misinformation about the swap action
From: Peilin Ye <hidden>
Date: 2021-07-20 19:22:21
Subsystem:
the rest · Maintainer:
Linus Torvalds
From: Peilin Ye <redacted>
Currently man 8 tc-skbmod says that "...the swap action will occur after
any smac/dmac substitutions are executed, if they are present."
This is false. In fact, trying to "set" and "swap" in a single skbmod
command causes the "set" part to be completely ignored. As an example:
$ tc filter add dev eth0 parent 1: protocol ip prio 10 \
matchall action skbmod \
set dmac AA:AA:AA:AA:AA:AA smac BB:BB:BB:BB:BB:BB \
swap mac
The above command simply does a "swap", without setting DMAC or SMAC to
AA's or BB's. The root cause of this is in the kernel, see
net/sched/act_skbmod.c:tcf_skbmod_init():
parm = nla_data(tb[TCA_SKBMOD_PARMS]);
index = parm->index;
if (parm->flags & SKBMOD_F_SWAPMAC)
lflags = SKBMOD_F_SWAPMAC;
^^^^^^^^^^^^^^^^^^^^^^^^^^
Doing a "=" instead of "|=" clears all other "set" flags when doing a
"swap". Discourage using "set" and "swap" in the same command by
documenting it as undefined behavior, and update the "SYNOPSIS" section
as well as tc -help text accordingly.
If one really needs to e.g. "set" DMAC to all AA's then "swap" DMAC and
SMAC, one should do two separate commands and "pipe" them together.
Reviewed-by: Cong Wang <redacted>
Signed-off-by: Peilin Ye <redacted>
---
Changes in v2:
- Update tc -help text as well
- Update commit message accordingly
- Fix typo in commit message
- Change title from "man: tc-skbmod.8:" to "tc/skbmod:"
man/man8/tc-skbmod.8 | 24 +++++++++++++-----------
tc/m_skbmod.c | 5 ++---
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/man/man8/tc-skbmod.8 b/man/man8/tc-skbmod.8
index eb3c38fa6bf3..76512311b17d 100644
--- a/man/man8/tc-skbmod.8
+++ b/man/man8/tc-skbmod.8@@ -5,12 +5,12 @@ skbmod - user-friendly packet editor action .SH SYNOPSIS .in +8 .ti -8 -.BR tc " ... " "action skbmod " "{ [ " "set " -.IR SETTABLE " ] [ " +.BR tc " ... " "action skbmod " "{ " "set " +.IR SETTABLE " | " .BI swap " SWAPPABLE" -.RI " ] [ " CONTROL " ] [ " +.RI " } [ " CONTROL " ] [ " .BI index " INDEX " -] } +] .ti -8 .IR SETTABLE " := "
@@ -25,6 +25,7 @@ skbmod - user-friendly packet editor action .IR SWAPPABLE " := " .B mac .ti -8 + .IR CONTROL " := {" .BR reclassify " | " pipe " | " drop " | " shot " | " continue " | " pass " }" .SH DESCRIPTION
@@ -48,10 +49,7 @@ Change the source mac to the specified address. Change the ethertype to the specified value. .TP .BI mac -Used to swap mac addresses. The -.B swap mac -directive is performed -after any outstanding D/SMAC changes. +Used to swap mac addresses. .TP .I CONTROL The following keywords allow to control how the tree of qdisc, classes,
@@ -128,9 +126,13 @@ tc filter add dev eth3 parent 1: protocol ip prio 10 \\ .EE .RE -As mentioned above, the swap action will occur after any -.B " smac/dmac " -substitutions are executed, if they are present. +However, trying to +.B set +and +.B swap +in a single +.B skbmod +command will cause undefined behavior. .SH SEE ALSO .BR tc (8),
diff --git a/tc/m_skbmod.c b/tc/m_skbmod.c
index e13d3f16bfcb..3fe30651a7d8 100644
--- a/tc/m_skbmod.c
+++ b/tc/m_skbmod.c@@ -28,10 +28,9 @@ static void skbmod_explain(void) { fprintf(stderr, - "Usage:... skbmod {[set <SETTABLE>] [swap <SWAPABLE>]} [CONTROL] [index INDEX]\n" + "Usage:... skbmod { set <SETTABLE> | swap <SWAPPABLE> } [CONTROL] [index INDEX]\n" "where SETTABLE is: [dmac DMAC] [smac SMAC] [etype ETYPE]\n" - "where SWAPABLE is: \"mac\" to swap mac addresses\n" - "note: \"swap mac\" is done after any outstanding D/SMAC change\n" + "where SWAPPABLE is: \"mac\" to swap mac addresses\n" "\tDMAC := 6 byte Destination MAC address\n" "\tSMAC := optional 6 byte Source MAC address\n" "\tETYPE := optional 16 bit ethertype\n"
--
2.20.1