Re: [PATCH] openvswitch: Add packet truncation support.
From: pravin shelar <hidden>
Date: 2016-06-09 04:30:12
On Wed, Jun 8, 2016 at 6:18 PM, William Tu [off-list ref] wrote:
quoted hunk ↗ jump to hunk
The patch adds a new OVS action, OVS_ACTION_ATTR_TRUNC, in order to truncate packets. A 'max_len' is added for setting up the maximum packet size, and a 'cutlen' field is to record the number of bytes to trim the packet when the packet is outputting to a port, or when the packet is sent to userspace. Signed-off-by: William Tu <redacted> --- include/uapi/linux/openvswitch.h | 7 +++++++ net/openvswitch/actions.c | 28 +++++++++++++++++++++++++++- net/openvswitch/datapath.c | 9 +++++++-- net/openvswitch/datapath.h | 2 ++ net/openvswitch/flow_netlink.c | 9 +++++++++ net/openvswitch/vport.c | 1 + 6 files changed, 53 insertions(+), 3 deletions(-)diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index bb0d515..6d7ac5b 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h@@ -580,6 +580,11 @@ enum ovs_userspace_attr { #define OVS_USERSPACE_ATTR_MAX (__OVS_USERSPACE_ATTR_MAX - 1) +struct ovs_action_trunc { + uint32_t max_len; /* Max packet size in bytes. */
This could uint16_t. as it is related to packet len.
+}; +#define ETH_MIN_FRAME_LEN 60 +
There is already symbol defined, ETH_ZLEN can used here. ...
quoted hunk ↗ jump to hunk
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 9a3eb7a..889b78b 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c@@ -750,6 +750,10 @@ static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port, if (likely(vport)) { u16 mru = OVS_CB(skb)->mru; + u16 cutlen = OVS_CB(skb)->cutlen; + + if (cutlen > 0) + pskb_trim(skb, skb->len - cutlen); if (likely(!mru || (skb->len <= mru + ETH_HLEN))) { ovs_vport_send(vport, skb);@@ -858,10 +862,19 @@ static int sample(struct datapath *dp, struct sk_buff *skb, return 0; /* The only known usage of sample action is having a single user-space + * action, or having a truncate action followed by a single user-space * action. Treat this usage as a special case. * The output_userspace() should clone the skb to be sent to the * user space. This skb will be consumed by its caller. */ + if (unlikely(nla_type(a) == OVS_ACTION_ATTR_TRUNC)) { + struct ovs_action_trunc *trunc = nla_data(a); + + OVS_CB(skb)->cutlen = skb->len > trunc->max_len ? + skb->len - trunc->max_len : 0;
There is no need to initialize the cutlen to zero if max-len is greater then skb len. it should be already zero.
+ a = nla_next(a, &rem); + } +
There cutlen is set in skb cb, but next action could be other than userspace, in that case cutlen is leaked to remain action list.
quoted hunk ↗ jump to hunk
if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE && nla_is_last(a, rem))) return output_userspace(dp, skb, key, a, actions, actions_len);@@ -1044,13 +1057,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, for (a = attr, rem = len; rem > 0; a = nla_next(a, &rem)) { int err = 0; + int cutlen = OVS_CB(skb)->cutlen; if (unlikely(prev_port != -1)) { struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC); - if (out_skb) + if (out_skb) { + /* skb_clone does not clone the CB. */
Are you sure about this? __copy_skb_header() seem to be doing the copy.
+ OVS_CB(out_skb)->cutlen = cutlen;
do_output(dp, out_skb, prev_port, key);
+ }
+ OVS_CB(skb)->cutlen = 0;You also need to set cutlen local variable. otherwise next output could pickup the value.
quoted hunk ↗ jump to hunk
prev_port = -1; }@@ -1059,6 +1077,14 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, prev_port = nla_get_u32(a); break; + case OVS_ACTION_ATTR_TRUNC: { + struct ovs_action_trunc *trunc = nla_data(a); + + OVS_CB(skb)->cutlen = skb->len > trunc->max_len ? + skb->len - trunc->max_len : 0;
similar to sample() function comment, no need to set OVS_CB cutlen to zero.
quoted hunk ↗ jump to hunk
+ break; + } + case OVS_ACTION_ATTR_USERSPACE: output_userspace(dp, skb, key, a, attr, len); break;diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 856bd8d..fb3e5a3 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c@@ -425,6 +425,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, size_t len; unsigned int hlen; int err, dp_ifindex; + int cutlen = OVS_CB(skb)->cutlen; dp_ifindex = get_dpifindex(dp); if (!dp_ifindex)@@ -461,6 +462,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, else hlen = skb->len; + if (cutlen > 0) + hlen -= cutlen; +
I do not think headline should be changed in case of sib-zero copy. This is size of linear data in skb, changing this value could result in wrong data copy in skb-zerocpy. Also no need to compare cut-lea to zero, it can be directly subtracted.