* [PATCH v2.42 1/5] odp: Allow VLAN actions after MPLS actions
From: Simon Horman @ 2013-10-04 8:09 UTC (permalink / raw)
To: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
Jesse Gross, Ben Pfaff
Cc: Isaku Yamahata, Ravi K
In-Reply-To: <1380874200-8981-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
From: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
OpenFlow 1.1 and 1.2, and 1.3 differ in their handling of MPLS actions in the
presence of VLAN tags. To allow correct behaviour to be committed in
each situation, this patch adds a second round of VLAN tag action
handling to commit_odp_actions(), which occurs after MPLS actions. This
is implemented with a new field in 'struct xlate_in' called 'vlan_tci'.
When an push_mpls action is composed, the flow's current VLAN state is
stored into xin->vlan_tci, and flow->vlan_tci is set to 0 (pop_vlan). If
a VLAN tag is present, it is stripped; if not, then there is no change.
Any later modifications to the VLAN state is written to xin->vlan_tci.
When committing the actions, flow->vlan_tci is used before MPLS actions,
and xin->vlan_tci is used afterwards. This retains the current datapath
behaviour, but allows VLAN actions to be applied in a more flexible
manner.
Both before and after this patch MPLS LSEs are pushed onto a packet after
any VLAN tags that may be present. This is the behaviour described in
OpenFlow 1.1 and 1.2. OpenFlow 1.3 specifies that MPLS LSEs should be
pushed onto a packet before any VLAN tags that are present. Support
for this will be added by a subsequent patch that makes use of
the infrastructure added by this patch.
Signed-off-by: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
---
v2.41
* Rework comments to make things a little clearer
v2.40
* Rebase for removal of mpls_depth from struct flow
v2.38 - v2.39
* No change
v2.37
* Rebase
v2.36
* No change
v2.5
* First post
---
lib/odp-util.c | 9 +-
lib/odp-util.h | 2 +-
ofproto/ofproto-dpif-xlate.c | 101 ++++++++++++++++-----
ofproto/ofproto-dpif-xlate.h | 5 ++
tests/ofproto-dpif.at | 209 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 303 insertions(+), 23 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 5c7ccfb..4028167 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3600,11 +3600,15 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base,
* key from 'base' into 'flow', and then changes 'base' the same way. Does not
* commit set_tunnel actions. Users should call commit_odp_tunnel_action()
* in addition to this function if needed. Sets fields in 'wc' that are
- * used as part of the action. */
+ * used as part of the action.
+ *
+ * VLAN actions may be committed twice; If vlan_tci in 'flow' differs from the
+ * one in 'base', then it is committed before MPLS actions. If 'final_vlan_tci'
+ * differs from 'flow->vlan_tci', it is committed afterwards. */
void
commit_odp_actions(const struct flow *flow, struct flow *base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc,
- int *mpls_depth_delta)
+ int *mpls_depth_delta, ovs_be16 final_vlan_tci)
{
commit_set_ether_addr_action(flow, base, odp_actions, wc);
commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
@@ -3615,6 +3619,7 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
* that it is no longer IP and thus nw and port actions are no longer valid.
*/
commit_mpls_action(flow, base, odp_actions, wc, mpls_depth_delta);
+ commit_vlan_action(final_vlan_tci, base, odp_actions, wc);
commit_set_priority_action(flow, base, odp_actions, wc);
commit_set_pkt_mark_action(flow, base, odp_actions, wc);
}
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 2712cb0..a7bca43 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -143,7 +143,7 @@ void commit_odp_tunnel_action(const struct flow *, struct flow *base,
struct ofpbuf *odp_actions);
void commit_odp_actions(const struct flow *, struct flow *base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc,
- int *mpls_depth_delta);
+ int *mpls_depth_delta, ovs_be16 final_vlan_tci);
\f
/* ofproto-dpif interface.
*
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index cced7cc..6757933 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -982,10 +982,11 @@ static void
output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
uint16_t vlan)
{
- ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
+ ovs_be16 *flow_tci = &ctx->xin->vlan_tci;
uint16_t vid;
ovs_be16 tci, old_tci;
struct xport *xport;
+ bool flow_tci_equal_to_xin = (*flow_tci == ctx->xin->flow.vlan_tci);
vid = output_vlan_to_vid(out_xbundle, vlan);
if (list_is_empty(&out_xbundle->xports)) {
@@ -1016,9 +1017,15 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
}
}
*flow_tci = tci;
+ if (flow_tci_equal_to_xin) {
+ ctx->xin->flow.vlan_tci = tci;
+ }
compose_output_action(ctx, xport->ofp_port);
*flow_tci = old_tci;
+ if (flow_tci_equal_to_xin) {
+ ctx->xin->flow.vlan_tci = old_tci;
+ }
}
/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
@@ -1251,7 +1258,7 @@ xlate_normal(struct xlate_ctx *ctx)
/* Drop malformed frames. */
if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
- !(flow->vlan_tci & htons(VLAN_CFI))) {
+ !(ctx->xin->vlan_tci & htons(VLAN_CFI))) {
if (ctx->xin->packet != NULL) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
@@ -1275,7 +1282,7 @@ xlate_normal(struct xlate_ctx *ctx)
}
/* Check VLAN. */
- vid = vlan_tci_to_vid(flow->vlan_tci);
+ vid = vlan_tci_to_vid(ctx->xin->vlan_tci);
if (!input_vid_is_valid(vid, in_xbundle, ctx->xin->packet != NULL)) {
xlate_report(ctx, "disallowed VLAN VID for this input port, dropping");
return;
@@ -1533,7 +1540,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
- ovs_be16 flow_vlan_tci;
+ ovs_be16 flow_vlan_tci, xin_vlan_tci;
uint32_t flow_pkt_mark;
uint8_t flow_nw_tos;
odp_port_t out_port, odp_port;
@@ -1602,6 +1609,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
}
flow_vlan_tci = flow->vlan_tci;
+ xin_vlan_tci = ctx->xin->vlan_tci;
flow_pkt_mark = flow->pkt_mark;
flow_nw_tos = flow->nw_tos;
@@ -1641,19 +1649,20 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
}
vlandev_port = vsp_realdev_to_vlandev(ctx->xbridge->ofproto, ofp_port,
- flow->vlan_tci);
+ ctx->xin->vlan_tci);
if (vlandev_port == ofp_port) {
out_port = odp_port;
} else {
out_port = ofp_port_to_odp_port(ctx->xbridge, vlandev_port);
flow->vlan_tci = htons(0);
+ ctx->xin->vlan_tci = htons(0);
}
}
if (out_port != ODPP_NONE) {
commit_odp_actions(flow, &ctx->base_flow,
&ctx->xout->odp_actions, &ctx->xout->wc,
- &ctx->mpls_depth_delta);
+ &ctx->mpls_depth_delta, ctx->xin->vlan_tci);
nl_msg_put_odp_port(&ctx->xout->odp_actions, OVS_ACTION_ATTR_OUTPUT,
out_port);
@@ -1665,6 +1674,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
out:
/* Restore flow */
flow->vlan_tci = flow_vlan_tci;
+ ctx->xin->vlan_tci = xin_vlan_tci;
flow->pkt_mark = flow_pkt_mark;
flow->nw_tos = flow_nw_tos;
}
@@ -1809,7 +1819,7 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
&ctx->xout->odp_actions, &ctx->xout->wc,
- &ctx->mpls_depth_delta);
+ &ctx->mpls_depth_delta, ctx->xin->vlan_tci);
odp_execute_actions(NULL, packet, &key, ctx->xout->odp_actions.data,
ctx->xout->odp_actions.size, NULL, NULL);
@@ -2197,7 +2207,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
&ctx->xout->odp_actions, &ctx->xout->wc,
- &ctx->mpls_depth_delta);
+ &ctx->mpls_depth_delta, ctx->xin->vlan_tci);
compose_flow_sample_cookie(os->probability, os->collector_set_id,
os->obs_domain_id, os->obs_point_id, &cookie);
@@ -2226,13 +2236,35 @@ may_receive(const struct xport *xport, struct xlate_ctx *ctx)
}
static void
+vlan_tci_restore(struct xlate_in *xin, ovs_be16 *tci_ptr, ovs_be16 orig_tci)
+{
+ /* If MPLS actions were executed after vlan actions then
+ * copy the final vlan_tci out and restore the intermediate VLAN state. */
+ if (xin->flow.vlan_tci != orig_tci && tci_ptr == &xin->vlan_tci) {
+ xin->vlan_tci = xin->flow.vlan_tci;
+ xin->flow.vlan_tci = orig_tci;
+ }
+}
+
+static void
do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
struct xlate_ctx *ctx)
{
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
+ ovs_be16 *vlan_tci;
const struct ofpact *a;
+
+ /* VLAN actions are stored in '*vlan_tci'. This variable initially
+ * points to 'xin->flow->vlan_tci', so that VLAN actions are applied
+ * before any MPLS actions. When an MPLS action is translated,
+ * 'vlan_tci' is updated to point to 'xin->vlan_tci'. This causes later
+ * VLAN actions to be applied after MPLS actions. For each iteration
+ * of the loop 'xin->vlan_tci' is updated to reflect the final VLAN
+ * state of the flow. */
+ vlan_tci = &ctx->xin->flow.vlan_tci;
+
OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
struct ofpact_controller *controller;
const struct ofpact_metadata *metadata;
@@ -2241,6 +2273,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
break;
}
+ /* Update the final vlan state to match the current state. */
+ ctx->xin->vlan_tci = *vlan_tci;
+
switch (a->type) {
case OFPACT_OUTPUT:
xlate_output_action(ctx, ofpact_get_OUTPUT(a)->port,
@@ -2264,28 +2299,28 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
case OFPACT_SET_VLAN_VID:
wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
- flow->vlan_tci &= ~htons(VLAN_VID_MASK);
- flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
- | htons(VLAN_CFI));
+ *vlan_tci &= ~htons(VLAN_VID_MASK);
+ *vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
+ | htons(VLAN_CFI));
break;
case OFPACT_SET_VLAN_PCP:
- wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
- flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
- flow->vlan_tci |=
+ wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
+ *vlan_tci &= ~htons(VLAN_PCP_MASK);
+ *vlan_tci |=
htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
| VLAN_CFI);
break;
case OFPACT_STRIP_VLAN:
memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
- flow->vlan_tci = htons(0);
+ *vlan_tci = htons(0);
break;
case OFPACT_PUSH_VLAN:
/* XXX 802.1AD(QinQ) */
memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
- flow->vlan_tci = htons(VLAN_CFI);
+ *vlan_tci = htons(VLAN_CFI);
break;
case OFPACT_SET_ETH_SRC:
@@ -2353,29 +2388,54 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
flow->skb_priority = ctx->orig_skb_priority;
break;
- case OFPACT_REG_MOVE:
+ case OFPACT_REG_MOVE: {
+ ovs_be16 orig_tci = flow->vlan_tci;
nxm_execute_reg_move(ofpact_get_REG_MOVE(a), flow, wc);
+ vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
break;
+ }
- case OFPACT_REG_LOAD:
+ case OFPACT_REG_LOAD: {
+ ovs_be16 orig_tci = flow->vlan_tci;
nxm_execute_reg_load(ofpact_get_REG_LOAD(a), flow);
+ vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
break;
+ }
- case OFPACT_STACK_PUSH:
+ case OFPACT_STACK_PUSH: {
+ ovs_be16 orig_tci = flow->vlan_tci;
+ flow->vlan_tci = *vlan_tci;
nxm_execute_stack_push(ofpact_get_STACK_PUSH(a), flow, wc,
&ctx->stack);
+ flow->vlan_tci = orig_tci;
break;
+ }
- case OFPACT_STACK_POP:
+ case OFPACT_STACK_POP: {
+ ovs_be16 orig_tci = flow->vlan_tci;
nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, wc,
&ctx->stack);
+ vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
break;
+ }
case OFPACT_PUSH_MPLS:
if (compose_mpls_push_action(ctx,
ofpact_get_PUSH_MPLS(a)->ethertype)) {
return;
}
+
+ /* Save and pop any existing VLAN tags. They will be pushed
+ * back onto the packet after pushing the MPLS LSE. The overall
+ * effect is to push the MPLS LSE after any VLAN tags that may
+ * be present. This is the behaviour described for OpenFlow 1.1
+ * and 1.2. This code needs to be enhanced to make this
+ * conditional to also and support pushing the MPLS LSE before
+ * any VLAN tags that may be present, the behaviour described
+ * for OpenFlow 1.3. */
+ ctx->xin->vlan_tci = *vlan_tci;
+ flow->vlan_tci = htons(0);
+ vlan_tci = &ctx->xin->vlan_tci;
break;
case OFPACT_POP_MPLS:
@@ -2477,6 +2537,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
{
xin->ofproto = ofproto;
xin->flow = *flow;
+ xin->vlan_tci = flow->vlan_tci;
xin->packet = packet;
xin->may_learn = packet != NULL;
xin->rule = rule;
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 6403f50..54fd36d 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -60,6 +60,11 @@ struct xlate_in {
* this flow when actions change header fields. */
struct flow flow;
+ /* If MPLS and VLAN actions were both present in the translation, and VLAN
+ * actions should occur after the MPLS actions, then this field is used
+ * to store the final vlan_tci state. */
+ ovs_be16 vlan_tci;
+
/* The packet corresponding to 'flow', or a null pointer if we are
* revalidating without a packet to refer to. */
const struct ofpbuf *packet;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ea36020..3d57f37 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -869,6 +869,215 @@ done
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - OF1.2 VLAN+MPLS handling])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+ON_EXIT([kill `cat ovs-ofctl.pid`])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [dnl
+cookie=0xa dl_src=40:44:44:44:54:50 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:51 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:52 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:53 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:54 actions=push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+cookie=0xa dl_src=40:44:44:44:54:55 actions=push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+cookie=0xa dl_src=40:44:44:44:54:56 actions=push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+cookie=0xa dl_src=40:44:44:44:54:57 actions=push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+])
+AT_CHECK([ovs-ofctl --protocols=OpenFlow12 add-flows br0 flows.txt])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the MPLS tag before pushing a VLAN tag, so we see
+dnl both of these in the final flow
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:50,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:51,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the MPLS tag before pushing a VLAN tag, so we see
+dnl both of these in the final flow
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:52,dst=52:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:52,dl_dst=52:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:52,dl_dst=52:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:52,dl_dst=52:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:53,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:53,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:53,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:53,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the VLAN tag before pushing a MPLS tag, but these
+dnl actions are reordered, so we see both of these in the final flow.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:54,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:54,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:54,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:54,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:55,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:55,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:55,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:55,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the VLAN tag before pushing a MPLS tag, but these
+dnl actions are reordered, so we see both of these in the final flow.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:56,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:56,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:56,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:56,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:57,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:57,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:57,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:57,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:50 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],mod_vlan_vid:99,mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:51 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],mod_vlan_vid:99,mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:52 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:53 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:54 actions=mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:55 actions=mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:56 actions=load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:57 actions=load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+NXST_FLOW reply:
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - fragment handling])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [2], [3], [4], [5], [6], [90])
--
1.8.4
^ permalink raw reply related
* [PATCH v2.42 0/5] MPLS actions and matches
From: Simon Horman @ 2013-10-04 8:09 UTC (permalink / raw)
To: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
Jesse Gross, Ben Pfaff
Cc: Isaku Yamahata, Ravi K
Hi,
This series implements MPLS actions and matches based on work by
Ravi K, Leo Alterman, Yamahata-san and Joe Stringer.
This series provides two changes
* Patches 1 - 3
Provide user-space support for the VLAN/MPLS tag insertion order
up to and including OpenFlow 1.2, and the different ordering
specified from OpenFlow 1.3. In a nutshell the datapath always
uses the OpenFlow 1.3 ordering, which is to always insert tags
immediately after the L2 header, regardless of the presence of other
tags. And ovs-vswtichd provides compatibility for the behaviour up
to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags
if present.
These patches have been updated since v2.40.
Ben, these are for you to review.
* Patches 4 and 5
Adding basic MPLS action and match support to the kernel datapath
These patches have not been updated since v2.40.
Jesse, these are for you to review.
Differences between v2.42 and v2.41:
v2.42
* Rebase for:
+ 0585f7a ("datapath: Simplify mega-flow APIs.")
+ a097c0b ("datapath: Restructure datapath.c and flow.c")
* As suggested by Jesse Gross
+ Take into account that push_mpls() will have freed the skb on error
+ Remove dubious !eth_p_mpls(skb->protocol) condition from push_mpls
The !eth_p_mpls(skb->protocol) condition on setting inner_protocol
has no effect. Its motivation was to ensure that inner_protocol was
only set the first time that mpls_push occured. However this is already
ensured by the !ovs_skb_get_inner_protocol(skb) condition.
+ Return -EINVAL instead of -ENOMEM from pop_mpls() if the skb is too short
+ Do not add @inner_protocol to kernel doc for struct ovs_skb_cb.
The patch no longer adds an inner_protocol member to struct ovs_skb_cb
+ Do not add and set otherwise unsued inner_protocol variable in
rpl_dev_queue_xmit()
* As suggested by Pravin Shelar
+ Implement compatibility code in existing rpl_skb_gso_segment
rather than introducing to use rpl___skb_gso_segment
Differences between v2.41 and v2.40:
* As suggested by Ben Pfaff
+ Expand struct ofpact_reg_load to include a mpls_before_vlan field
rather than using the compat field of the ofpact field of
struct ofpact_reg_load.
+ Pass version to ofpacts_pull_openflow11_actions and
ofpacts_pull_openflow11_instructions. This removes the invalid
assumption that that these functions are passed a full message and are
thus able to deduce the OpenFlow version.
Differences between v2.40 and v2.39:
* Rebase for:
+ New dev_queue_xmit compat code
+ Updated put_vlan()
+ Removal of mpls_depth field from struct flow
* As suggested by Jesse Gross
+ Remove bogus mac_len update from push_mpls()
+ Slightly simplify push_mpls() by using eth_hdr()
+ Remove dubious condition !eth_p_mpls(inner_protocol) on
an skb being considered to be MPLS in netdev_send()
+ Only use compatibility code for MPLS GSO segmentation on kernels
older than 3.11
+ Revamp setting of inner_protocol
1. Do not unconditionally set inner_protocol to the value of
skb->protocol in ovs_execute_actions().
2. Initialise inner_protocol it to zero only if compatibility code is in
use. In the case where compatibility code is not in use it will either
be zero due since the allocation of the skb or some other value set
by some other user.
3. Conditionally set the inner_protocol in push_mpls() to the value of
skb->protocol when entering push_mpls(). The condition is that
inner_protocol is zero and the value of skb->protocol is not an MPLS
ethernet type.
- This new scheme:
+ Pushes logic to set inner_protocol closer to the case where it is
needed.
+ Avoids over-writing values set by other users.
* As suggested by Pravin Shelar
+ Only set and restore skb->protocol in rpl___skb_gso_segment() in the
case of MPLS
+ Add inner_protocol field to struct ovs_gso_cb instead of ovs_skb_cb.
This moves compatibility code closer to where it is used
and creates fewer differences with mainline.
* Update comment on mac_len updates in datapath/actions.c
* Remove HAVE_INNER_PROCOTOL and instead just check
against kernel version 3.11 directly.
HAVE_INNER_PROCOTOL is a hang-over from work done prior
to the merge of inner_protocol into the kernel.
* Remove dubious condition !eth_p_mpls(inner_protocol) on
using inner_protocol as the type in rpl_skb_network_protocol()
* Do not update type of features in rpl_dev_queue_xmit.
Though arguably correct this is not an inherent part of
the changes made by this patch.
* Use skb_cow_head() in push_mpls()
+ Call skb_cow_head(skb, MPLS_HLEN) instead of
make_writable(skb, skb->mac_len) to ensure that there is enough head
room to push an MPLS LSE regardless of whether the skb is cloned or not.
+ This is consistent with the behaviour of rpl__vlan_put_tag().
+ This is a fix for crashes reported when performing mpls_push
with headroom less than 4. This problem was introduced in v3.36.
* Skip popping in mpls_pop if the skb is too short to contain an MPLS LSE
Differences between v2.39 and v2.38:
* Rebase for removal of vlan, checksum and skb->mark compat code
- This includes adding adding a new patch,
"[PATCH v2.39 6/7] datapath: Break out deacceleration portion of
vlan_push" to allow re-use of some existing code.
Differences between v2.38 and v2.37:
* Rebase for SCTP support
* Refactor validate_tp_port() to iterate over eth_types rather
than open-coding the loop. With the addition of SCTP this logic
is now used three times.
Differences between v2.37 and v2.36:
* Rebase
Differences between v2.36 and v2.35:
* Rebase
* Do not add set_ethertype() to datapath/actions.c.
As this patch has evolved this function had devolved into
to sets of functionality wrapped into a single function with
only one line of common code. Refactor things to simply
open-code setting the ether type in the two locations where
set_ethertype() was previously used. The aim here is to improve
readability.
* Update setting skb->ethertype after mpls push and pop.
- In the case of push_mpls it should be set unconditionally
as in v2.35 the behaviour of this function to always push
an MPLS LSE before any VLAN tags.
- In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
test than skb->protocol != htons(ETH_P_8021Q) as it will give the
correct behaviour in the presence of other VLAN ethernet types,
for example 0x88a8 which is used by 802.1ad. Moreover, it seems
correct to update the ethernet type if it was previously set
according to the top-most MPLS LSE.
* Deaccelerate VLANs when pushing MPLS tags the
- Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
This means that if an accelerated tag is present it should be
deaccelerated to ensure it ends up in the correct position.
* Update skb->mac_len in push_mpls() so that it will be correct
when used by a subsequent call to pop_mpls().
As things stand I do not believe this is strictly necessary as
ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
However, I have added this in order to code more defensively as I believe
that if such a sequence did occur it would be rather unobvious why
it didn't work.
* Do not add skb_cow_head() call in push_mpls().
It is unnecessary as there is a make_writable() call.
This change was also made in v2.30 but some how the
code regressed between then and v2.35.
Differences between v2.35 and v2.34:
* Add support for the tag ordering specified up until OpenFlow 1.2 and
the ordering specified from OpenFlow 1.3.
* Correct error in datapath patch's handling of GSO in the presence
of MPLS and absence of VLANs.
To aid review this series is available in git at:
git://github.com/horms/openvswitch.git devel/mpls-v2.42
Patch list and overall diffstat:
Joe Stringer (3):
odp: Allow VLAN actions after MPLS actions
ofp-actions: Add separate OpenFlow 1.3 action parser
lib: Support pushing of MPLS LSE before or after VLAN tag
Simon Horman (2):
datapath: Break out deacceleration portion of vlan_push
datapath: Add basic MPLS support to kernel
datapath/Modules.mk | 1 +
datapath/actions.c | 158 ++++++++-
datapath/datapath.c | 4 +-
datapath/flow.c | 29 ++
datapath/flow.h | 17 +-
datapath/flow_netlink.c | 286 +++++++++++++--
datapath/flow_netlink.h | 2 +-
datapath/linux/compat/gso.c | 70 +++-
datapath/linux/compat/gso.h | 41 +++
datapath/linux/compat/include/linux/netdevice.h | 6 +-
datapath/linux/compat/netdevice.c | 10 +-
datapath/mpls.h | 15 +
include/linux/openvswitch.h | 7 +-
lib/flow.c | 2 +-
lib/odp-util.c | 9 +-
lib/odp-util.h | 2 +-
lib/ofp-actions.c | 68 +++-
lib/ofp-actions.h | 9 +
lib/ofp-print.c | 2 +-
lib/ofp-util.c | 24 +-
lib/ofp-util.h | 2 +-
lib/packets.c | 10 +-
lib/packets.h | 2 +-
ofproto/ofproto-dpif-xlate.c | 110 ++++--
ofproto/ofproto-dpif-xlate.h | 5 +
tests/ofproto-dpif.at | 446 ++++++++++++++++++++++++
utilities/ovs-ofctl.c | 8 +-
27 files changed, 1233 insertions(+), 112 deletions(-)
create mode 100644 datapath/mpls.h
--
1.8.4
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Ingo Molnar @ 2013-10-04 7:51 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Heiko Carstens, Eric Dumazet, Paul Mackerras, H. Peter Anvin,
sparclinux, Nicolas Dichtel, linux-s390, Russell King, x86,
James Morris, Ingo Molnar, Alexey Kuznetsov, Paul E. McKenney,
Xi Wang, Matt Evans, Thomas Gleixner, linux-arm-kernel,
Stelian Nirlu, Nicolas Schichan, Hideaki YOSHIFUJI, netdev,
linux-kernel, David S. Miller, Mircea Gherzan, Daniel Borkmann
In-Reply-To: <1380853446-30537-1-git-send-email-ast@plumgrid.com>
* Alexei Starovoitov <ast@plumgrid.com> wrote:
> on x86 system with net.core.bpf_jit_enable = 1
>
> sudo tcpdump -i eth1 'tcp port 22'
>
> causes the warning:
> [ 56.766097] Possible unsafe locking scenario:
> [ 56.766097]
> [ 56.780146] CPU0
> [ 56.786807] ----
> [ 56.793188] lock(&(&vb->lock)->rlock);
> [ 56.799593] <Interrupt>
> [ 56.805889] lock(&(&vb->lock)->rlock);
> [ 56.812266]
> [ 56.812266] *** DEADLOCK ***
> [ 56.812266]
> [ 56.830670] 1 lock held by ksoftirqd/1/13:
> [ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
> [ 56.849757]
> [ 56.849757] stack backtrace:
> [ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
> [ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
> [ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
> [ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
> [ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
> [ 56.923006] Call Trace:
> [ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
> [ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
> [ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
> [ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
> [ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
> [ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
> [ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
> [ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
> [ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
> [ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
> [ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
> [ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
> [ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
> [ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
> [ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
> [ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
> [ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
> [ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
> [ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
> [ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
> [ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
> [ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
> [ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
> [ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
>
> cannot reuse jited filter memory, since it's readonly,
> so use original bpf insns memory to hold work_struct
>
> defer kfree of sk_filter until jit completed freeing
>
> tested on x86_64 and i386
>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> arch/arm/net/bpf_jit_32.c | 1 +
> arch/powerpc/net/bpf_jit_comp.c | 1 +
> arch/s390/net/bpf_jit_comp.c | 4 +++-
> arch/sparc/net/bpf_jit_comp.c | 1 +
> arch/x86/net/bpf_jit_comp.c | 20 +++++++++++++++-----
> include/linux/filter.h | 11 +++++++++--
> net/core/filter.c | 11 +++++++----
> 7 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index f50d223..99b44e0 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter)
> module_free(NULL, fp->bpf_func);
> + kfree(fp);
> }
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index bf56e33..2345bdb 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter)
> module_free(NULL, fp->bpf_func);
> + kfree(fp);
> }
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index 7092392..a5df511 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
> struct bpf_binary_header *header = (void *)addr;
>
> if (fp->bpf_func == sk_run_filter)
> - return;
> + goto free_filter;
> set_memory_rw(addr, header->pages);
> module_free(NULL, header);
> +free_filter:
> + kfree(fp);
> }
> diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
> index 9c7be59..218b6b2 100644
> --- a/arch/sparc/net/bpf_jit_comp.c
> +++ b/arch/sparc/net/bpf_jit_comp.c
> @@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter)
> module_free(NULL, fp->bpf_func);
> + kfree(fp);
> }
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 79c216a..1396a0a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -772,13 +772,23 @@ out:
> return;
> }
>
> +static void bpf_jit_free_deferred(struct work_struct *work)
> +{
> + struct sk_filter *fp = container_of((void *)work, struct sk_filter,
> + insns);
> + unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
> + struct bpf_binary_header *header = (void *)addr;
> +
> + set_memory_rw(addr, header->pages);
> + module_free(NULL, header);
> + kfree(fp);
> +}
Using the data type suggestions I make further below, this could be
written in a simpler form, as:
struct sk_filter *fp = container_of(work, struct sk_filter, work);
Also, a question, why do you mask with PAGE_MASK here:
unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
?
AFAICS bpf_func is the module_alloc() result - and module code is page
aligned. So ->bpf_func is always page aligned here. (The sk_run_filter
special case cannot happen here.)
> +
> void bpf_jit_free(struct sk_filter *fp)
> {
> if (fp->bpf_func != sk_run_filter) {
> - unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
> - struct bpf_binary_header *header = (void *)addr;
> -
> - set_memory_rw(addr, header->pages);
> - module_free(NULL, header);
> + struct work_struct *work = (struct work_struct *)fp->insns;
> + INIT_WORK(work, bpf_jit_free_deferred);
Missing newline between local variables and statements.
> + schedule_work(work);
> }
> }
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index a6ac848..5d66cd9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -25,15 +25,20 @@ struct sk_filter
> {
> atomic_t refcnt;
> unsigned int len; /* Number of filter blocks */
> + struct rcu_head rcu;
> unsigned int (*bpf_func)(const struct sk_buff *skb,
> const struct sock_filter *filter);
> - struct rcu_head rcu;
> + /* insns start right after bpf_func, so that sk_run_filter() fetches
> + * first insn from the same cache line that was used to call into
> + * sk_run_filter()
> + */
> struct sock_filter insns[0];
Please use the customary (multi-line) comment style:
/*
* Comment .....
* ...... goes here.
*/
specified in Documentation/CodingStyle.
> };
>
> static inline unsigned int sk_filter_len(const struct sk_filter *fp)
> {
> - return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
> + return max(fp->len * sizeof(struct sock_filter),
> + sizeof(struct work_struct)) + sizeof(*fp);
So, "sizeof(struct work_struct)) + sizeof(*fp)" is a pattern that repeats
a couple of times. Might make sense to stick that into a helper of its own
- but in general this open coded overlay allocation method looks a bit
fragile and not very obvious in isolation.
So it could be done a bit cleaner, using an anonymous union:
/*
* These two overlay, the work struct is used during workqueue
* driven teardown, when the instructions are not used anymore:
*/
union {
struct sock_filter insns[0];
struct work_struct work;
};
And then all the sizeof() calculations become obvious and sk_filter_len()
could be eliminated - I've marked the conversions in the code further
below.
> extern int sk_filter(struct sock *sk, struct sk_buff *skb);
> @@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
> }
> #define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
> #else
> +#include <linux/slab.h>
Inlines in the middle of header files are generally frowned upon.
The standard pattern is to put them at the top, that way it's easier to
see the dependencies and there's also less .config dependent inclusion,
which makes header hell cleanup work easier.
> static inline void bpf_jit_compile(struct sk_filter *fp)
> {
> }
> static inline void bpf_jit_free(struct sk_filter *fp)
> {
> + kfree(fp);
> }
> #define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
> #endif
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6438f29..ad5eaba 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
> struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
>
> bpf_jit_free(fp);
> - kfree(fp);
> }
> EXPORT_SYMBOL(sk_filter_release_rcu);
>
> @@ -677,13 +676,15 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
> {
> struct sk_filter *fp;
> unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
> + unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
> + + sizeof(*fp);
Using the structure definition I suggested, this could be replaced with
the more obvious:
unsigned int sk_fsize = max(fsize, sizeof(*fp));
> int err;
>
> /* Make sure new filter is there and in the right amounts. */
> if (fprog->filter == NULL)
> return -EINVAL;
>
> - fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
> + fp = kmalloc(sk_fsize, GFP_KERNEL);
> if (!fp)
> return -ENOMEM;
> memcpy(fp->insns, fprog->filter, fsize);
> @@ -723,6 +724,8 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
> {
> struct sk_filter *fp, *old_fp;
> unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
> + unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
> + + sizeof(*fp);
This too could be written as:
unsigned int sk_fsize = max(fsize, sizeof(*fp));
> int err;
>
> if (sock_flag(sk, SOCK_FILTER_LOCKED))
> @@ -732,11 +735,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
> if (fprog->filter == NULL)
> return -EINVAL;
>
> - fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
> + fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
> if (!fp)
> return -ENOMEM;
> if (copy_from_user(fp->insns, fprog->filter, fsize)) {
> - sock_kfree_s(sk, fp, fsize+sizeof(*fp));
> + sock_kfree_s(sk, fp, sk_fsize);
> return -EFAULT;
> }
A couple of questions/suggestions:
1)
I took a brief look at arch/x86/net/bpf_jit_comp.c while reviewing this
patch.
You need to split up bpf_jit_compile(), it's an obscenely large, ~600
lines long function. We don't do that in modern, maintainable kernel code.
2)
This 128 bytes extra padding:
/* Most of BPF filters are really small,
* but if some of them fill a page, allow at least
* 128 extra bytes to insert a random section of int3
*/
sz = round_up(proglen + sizeof(*header) + 128, PAGE_SIZE);
why is it done? It's not clear to me from the comment.
3)
It's nice code altogether! Are there any plans to generalize its
interfaces, to allow arbitrary bytecode to be used by other kernel
subsystems as well? In particular tracing filters could make use of it,
but it would also allow safe probe points.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH RFC 04/77] PCI/MSI/s390: Remove superfluous check of MSI type
From: Martin Schwidefsky @ 2013-10-04 7:40 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-kernel, Bjorn Helgaas, Ralf Baechle, Michael Ellerman,
Benjamin Herrenschmidt, Ingo Molnar, Tejun Heo, Dan Williams,
Andy King, Jon Mason, Matt Porter, stable, linux-pci, linux-mips,
linuxppc-dev, linux390, linux-s390, x86, linux-ide,
iss_storagedev, linux-nvme, linux-rdma, netdev, e1000-devel,
linux-driver, Solarflare linux maintainers, VMware, Inc., lin
In-Reply-To: <bae65aa3e30dfd23bd5ed47add7310cfbb96243a.1380703262.git.agordeev@redhat.com>
On Wed, 2 Oct 2013 12:48:20 +0200
Alexander Gordeev <agordeev@redhat.com> wrote:
> arch_setup_msi_irqs() hook can only be called from the generic
> MSI code which ensures correct MSI type parameter.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
> arch/s390/pci/pci.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index c79c6e4..61a3c2c 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -425,8 +425,6 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> int rc;
>
> pr_debug("%s: requesting %d MSI-X interrupts...", __func__, nvec);
> - if (type != PCI_CAP_ID_MSIX && type != PCI_CAP_ID_MSI)
> - return -EINVAL;
> if (type == PCI_CAP_ID_MSI && nvec > 1)
> return 1;
> msi_vecs = min(nvec, ZPCI_MSI_VEC_MAX);
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* Re: [PATCH RFC 03/77] PCI/MSI/s390: Fix single MSI only check
From: Martin Schwidefsky @ 2013-10-04 7:39 UTC (permalink / raw)
To: Alexander Gordeev
Cc: linux-kernel, Bjorn Helgaas, Ralf Baechle, Michael Ellerman,
Benjamin Herrenschmidt, Ingo Molnar, Tejun Heo, Dan Williams,
Andy King, Jon Mason, Matt Porter, stable, linux-pci, linux-mips,
linuxppc-dev, linux390, linux-s390, x86, linux-ide,
iss_storagedev, linux-nvme, linux-rdma, netdev, e1000-devel,
linux-driver, Solarflare linux maintainers, VMware, Inc., lin
In-Reply-To: <8c9811b13fd93e73641dab8e3bd1bd5b2dc37a61.1380703262.git.agordeev@redhat.com>
On Wed, 2 Oct 2013 12:48:19 +0200
Alexander Gordeev <agordeev@redhat.com> wrote:
> Multiple MSIs have never been supported on s390 architecture,
> but the platform code fails to report single MSI only.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
> arch/s390/pci/pci.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index f17a834..c79c6e4 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -427,6 +427,8 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
> pr_debug("%s: requesting %d MSI-X interrupts...", __func__, nvec);
> if (type != PCI_CAP_ID_MSIX && type != PCI_CAP_ID_MSI)
> return -EINVAL;
> + if (type == PCI_CAP_ID_MSI && nvec > 1)
> + return 1;
> msi_vecs = min(nvec, ZPCI_MSI_VEC_MAX);
> msi_vecs = min_t(unsigned int, msi_vecs, CONFIG_PCI_NR_MSI);
>
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* [PATCH v4 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 7:14 UTC (permalink / raw)
To: David S. Miller
Cc: Daniel Borkmann, Eric Dumazet, Heiko Carstens, linux-arm-kernel,
linuxppc-dev, linux-s390, netdev
on x86 system with net.core.bpf_jit_enable = 1
sudo tcpdump -i eth1 'tcp port 22'
causes the warning:
[ 56.766097] Possible unsafe locking scenario:
[ 56.766097]
[ 56.780146] CPU0
[ 56.786807] ----
[ 56.793188] lock(&(&vb->lock)->rlock);
[ 56.799593] <Interrupt>
[ 56.805889] lock(&(&vb->lock)->rlock);
[ 56.812266]
[ 56.812266] *** DEADLOCK ***
[ 56.812266]
[ 56.830670] 1 lock held by ksoftirqd/1/13:
[ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
[ 56.849757]
[ 56.849757] stack backtrace:
[ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
[ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
[ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
[ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
[ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
[ 56.923006] Call Trace:
[ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
[ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
[ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
[ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
[ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
[ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
[ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
[ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
[ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
[ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
[ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
[ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
[ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
[ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
[ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
[ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
[ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
[ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
[ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
[ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
[ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
[ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
cannot reuse jited filter memory, since it's readonly,
so use original bpf insns memory to hold work_struct
defer kfree of sk_filter until jit completed freeing
tested on x86_64 and i386
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/arm/net/bpf_jit_32.c | 1 +
arch/powerpc/net/bpf_jit_comp.c | 1 +
arch/s390/net/bpf_jit_comp.c | 4 +++-
arch/sparc/net/bpf_jit_comp.c | 1 +
arch/x86/net/bpf_jit_comp.c | 18 +++++++++++++-----
include/linux/filter.h | 15 +++++++++++----
include/net/sock.h | 6 ++----
net/core/filter.c | 8 ++++----
8 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index f50d223..99b44e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..2345bdb 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 7092392..a5df511 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
struct bpf_binary_header *header = (void *)addr;
if (fp->bpf_func == sk_run_filter)
- return;
+ goto free_filter;
set_memory_rw(addr, header->pages);
module_free(NULL, header);
+free_filter:
+ kfree(fp);
}
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 9c7be59..218b6b2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79c216a..516593e 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -772,13 +772,21 @@ out:
return;
}
+static void bpf_jit_free_deferred(struct work_struct *work)
+{
+ struct sk_filter *fp = container_of(work, struct sk_filter, work);
+ unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
+ struct bpf_binary_header *header = (void *)addr;
+
+ set_memory_rw(addr, header->pages);
+ module_free(NULL, header);
+ kfree(fp);
+}
+
void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter) {
- unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
- struct bpf_binary_header *header = (void *)addr;
-
- set_memory_rw(addr, header->pages);
- module_free(NULL, header);
+ INIT_WORK(&fp->work, bpf_jit_free_deferred);
+ schedule_work(&fp->work);
}
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..ff4e40c 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -6,6 +6,7 @@
#include <linux/atomic.h>
#include <linux/compat.h>
+#include <linux/workqueue.h>
#include <uapi/linux/filter.h>
#ifdef CONFIG_COMPAT
@@ -25,15 +26,19 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
- struct sock_filter insns[0];
+ union {
+ struct sock_filter insns[0];
+ struct work_struct work;
+ };
};
-static inline unsigned int sk_filter_len(const struct sk_filter *fp)
+static inline unsigned int sk_filter_size(unsigned int proglen)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(sizeof(struct sk_filter),
+ offsetof(struct sk_filter, insns[proglen]));
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
}
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
#else
+#include <linux/slab.h>
static inline void bpf_jit_compile(struct sk_filter *fp)
{
}
static inline void bpf_jit_free(struct sk_filter *fp)
{
+ kfree(fp);
}
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
#endif
diff --git a/include/net/sock.h b/include/net/sock.h
index e3bf213..1105357 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1612,16 +1612,14 @@ static inline void sk_filter_release(struct sk_filter *fp)
static inline void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp)
{
- unsigned int size = sk_filter_len(fp);
-
- atomic_sub(size, &sk->sk_omem_alloc);
+ atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
sk_filter_release(fp);
}
static inline void sk_filter_charge(struct sock *sk, struct sk_filter *fp)
{
atomic_inc(&fp->refcnt);
- atomic_add(sk_filter_len(fp), &sk->sk_omem_alloc);
+ atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
}
/*
diff --git a/net/core/filter.c b/net/core/filter.c
index 6438f29..01b7808 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
bpf_jit_free(fp);
- kfree(fp);
}
EXPORT_SYMBOL(sk_filter_release_rcu);
@@ -683,7 +682,7 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
if (fprog->filter == NULL)
return -EINVAL;
- fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
+ fp = kmalloc(sk_filter_size(fprog->len), GFP_KERNEL);
if (!fp)
return -ENOMEM;
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +722,7 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
{
struct sk_filter *fp, *old_fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = sk_filter_size(fprog->len);
int err;
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +732,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
if (fprog->filter == NULL)
return -EINVAL;
- fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
+ fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
- sock_kfree_s(sk, fp, fsize+sizeof(*fp));
+ sock_kfree_s(sk, fp, sk_fsize);
return -EFAULT;
}
--
1.7.9.5
^ permalink raw reply related
* LOAN OFFER
From: 005 @ 2013-10-04 7:02 UTC (permalink / raw)
To: Recipients
Do You need Business Loan or Personal if Yes?Name,Amount,Country,Loan Durations,Phone Number Email;changloanoffer@vip.se.com
^ permalink raw reply
* Re: [PATCH net-next] tcp: rcvbuf autotuning improvements
From: Michael Dalton @ 2013-10-04 6:56 UTC (permalink / raw)
To: Eric Dumazet
Cc: Daniel Borkmann, davem, netdev, Francesco Fusco, ycheng,
Neal Cardwell, Eric Northup
In-Reply-To: <1380805381.19002.206.camel@edumazet-glaptop.roam.corp.google.com>
Thanks Eric,
I believe this issue may be related to one that I encountered
recently - poor performance with MTU-sized packets in
virtio_net when mergeable receive buffers are enabled. Performance was
quite low relative to virtio_net where mergeable receive buffers are
disabled and MTU-sized packets are received. The issue can be reliably
reproduced via netperf TCP_STREAM when mergeable receive buffers is
enabled but GRO is disabled (to force MTU-sized packets on receive).
I found the root cause was the memory allocation strategy employed for
virtio_net -- when mergeable receive buffers are enabled, every
receive ring packet buffer is allocated using a full page via the page
allocator, so the SKB truesize is 4096 + skb header +
128 (GOOD_COPY_LEN). This means that there is >100% overhead
(true_size / number of bytes actually used to store packet data) for
MTU-sized packets, impacting TCP.
The issue can be resolved by switching mergeable receive packet's
packet allocation to use netdev_alloc_frag(), allocating MTU-sized (or
slightly larger) buffers, and handling the rare edge case where the
number of frags exceeds SKB_MAX_FRAGS (occurs for extremely large
GRO'd packets and is permitted by the virtio specification) by using
the SKB frag list. I will update this thread with a patch when one is
ready, hopefully in the next few days. Thanks!
Best,
Mike
On Thu, Oct 3, 2013 at 6:03 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-10-03 at 09:56 +0200, Daniel Borkmann wrote:
>> This is a complementary patch for commit 6ae705323 ("tcp: sndbuf
>> autotuning improvements") that fixes a performance regression on
>> receiver side in setups with low to mid latency, high throughput,
>> and senders with TSO/GSO off (receivers w/ default settings).
>>
>> The following measurements in Mbit/s were done for 60sec w/ netperf
>> on virtio w/ TSO/GSO off:
>>
>> (ms) 1) 2) 3)
>> 0 2762.11 1150.32 2906.17
>> 10 1083.61 538.89 1091.03
>> 25 471.81 313.18 474.60
>> 50 242.33 187.84 242.36
>> 75 162.14 134.45 161.95
>> 100 121.55 101.96 121.49
>> 150 80.64 57.75 80.48
>> 200 58.97 54.11 59.90
>> 250 47.10 46.92 47.31
>>
>> Same setup w/ TSO/GSO on:
>>
>> (ms) 1) 2) 3)
>> 0 12225.91 12366.89 16514.37
>> 10 1526.64 1525.79 2176.63
>> 25 655.13 647.79 871.52
>> 50 338.51 377.88 439.46
>> 75 246.49 278.46 295.62
>> 100 210.93 207.56 217.34
>> 150 127.88 129.56 141.33
>> 200 94.95 94.50 107.29
>> 250 67.39 73.88 88.35
>>
>> Similarly as in 6ae705323, we fixed up power-of-two rounding and
>> took cached mss into account, thus bringing per_mss calculations
>> closer to each other, the rest stays as is.
>>
>> We also renamed tcp_fixup_rcvbuf() to tcp_rcvbuf_expand() to be
>> consistent with tcp_sndbuf_expand().
>>
>> While we do think that 6ae705323b71 is the right way to go, also
>> this follow-up seems necessary to restore performance for
>> receivers.
>
> Hmm, I think you based this patch on some virtio requirements.
>
> I would rather fix virtio, because virtio has poor truesize/payload
> ratio.
>
> Michael Dalton is working on this right now.
>
> Really I don't understand how 'fixing' initial rcvbuf could explain such
> difference in a 60 second transfert.
>
> Normally, if autotuning was working, the first sk_rcvbuf value would
> only matter in the very beginning of a flow (maybe one, two or even
> three RTT)
>
> It looks like you only need to set sk_rcvbuf to tcp_rmem[2],
> so you probably have to fix the autotuning, or virtio to give normal
> skbs, not fat ones ;)
>
>
> Thanks
>
>
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Heiko Carstens @ 2013-10-04 6:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Eric Dumazet, Paul Mackerras, H. Peter Anvin, sparclinux,
Nicolas Dichtel, linux-s390, Russell King, x86, James Morris,
Ingo Molnar, Alexey Kuznetsov, Paul E. McKenney, Xi Wang,
Matt Evans, Thomas Gleixner, linux-arm-kernel, Stelian Nirlu,
Nicolas Schichan, Hideaki YOSHIFUJI, netdev, linux-kernel,
David S. Miller, Mircea Gherzan, Daniel Borkmann,
Martin Schwidefsky
In-Reply-To: <1380853446-30537-1-git-send-email-ast@plumgrid.com>
On Thu, Oct 03, 2013 at 07:24:06PM -0700, Alexei Starovoitov wrote:
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index 7092392..a5df511 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
> struct bpf_binary_header *header = (void *)addr;
>
> if (fp->bpf_func == sk_run_filter)
> - return;
> + goto free_filter;
> set_memory_rw(addr, header->pages);
> module_free(NULL, header);
> +free_filter:
> + kfree(fp);
> }
For the s390 part:
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v2.41 5/5] datapath: Add basic MPLS support to kernel
From: Simon Horman @ 2013-10-04 6:40 UTC (permalink / raw)
To: Pravin Shelar
Cc: dev@openvswitch.org, netdev, Jesse Gross, Ben Pfaff, Ravi K,
Isaku Yamahata, Joe Stringer
In-Reply-To: <CALnjE+o6TK_2OGbWjZ4EwXMnXw5bzMQH2PeTMJM6nJQV+V147Q@mail.gmail.com>
On Thu, Oct 03, 2013 at 07:46:46PM -0700, Pravin Shelar wrote:
> On Wed, Oct 2, 2013 at 5:20 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Wed, Oct 02, 2013 at 11:03:57AM -0700, Pravin Shelar wrote:
> >> On Mon, Sep 30, 2013 at 11:47 PM, Simon Horman <horms@verge.net.au> wrote:
> >> > Allow datapath to recognize and extract MPLS labels into flow keys
> >> > and execute actions which push, pop, and set labels on packets.
> >> >
> >> > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
> >> >
> >> > Cc: Ravi K <rkerur@gmail.com>
> >> > Cc: Leo Alterman <lalterman@nicira.com>
> >> > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> >> > Cc: Joe Stringer <joe@wand.net.nz>
> >> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >> >
> >> > ---
> >> >
> >> > +
> >> > + /* this hack needed to get regular skb_gso_segment() */
> >> > +#ifdef HAVE___SKB_GSO_SEGMENT
> >> > +#undef __skb_gso_segment
> >> > + skb_gso = __skb_gso_segment(skb, features, tx_path);
> >> > +#else
> >> > +#undef skb_gso_segment
> >> > + skb_gso = skb_gso_segment(skb, features);
> >> > +#endif
> >> > +
> >>
> >> We can get rid of #ifdefs by just using different name for
> >> rpl___skb_gso_segment(), something like mpls_vlan_skb_gso_segment().
> >> The way it is done for tnl-gso.
> >
> > Thanks.
> >
> > The reason that I had the code arranged this way was so that
> > calls to __skb_gso_segment() would go via rpl___skb_gso_segment()
> > on kernels older than v3.11. In particular calls outside of gso.c.
> >
> > On closer examination the only such case is in ovs_dp_upcall().
> > Currently there should be no need to perform MPLS GSO segmentation in that
> > case because MPLS GSO segmentation can only be needed after actions are
> > applied.
> >
> > However, I am concerned that it may be necessary later when
> > recirculation is introduced as in that case an upcall may occur
> > on a packet which has had actions applied.
>
> good point.
>
> currently we define __skb_gso_segment using skb_gso_segemt(). You have
> reversed it. Is there any reason?
> if you keep it as it is, it can simplify code a bit.
Thanks. I believe that I wanted to use the real __skb_gso_segment()
to back the compat version of __skb_gso_segment(). I can't recall
specifically why and that change seems to be orthogonal to the
MPLS patches.
I have switched things around as you suggested and the code
looks much cleaner. Thanks for the suggestion.
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 6:30 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Eric Dumazet, linux-arm-kernel, linuxppc-dev, linux-s390, netdev
In-Reply-To: <1380863798.3564.12.camel@edumazet-glaptop.roam.corp.google.com>
On Thu, Oct 3, 2013 at 10:16 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-10-03 at 21:11 -0700, Alexei Starovoitov wrote:
>
> -static inline unsigned int sk_filter_len(const struct sk_filter *fp)
> +static inline unsigned int sk_filter_size(const struct sk_filter *fp,
> + unsigned int proglen)
> {
> - return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
> + return max(sizeof(*fp),
> + offsetof(struct sk_filter, insns[proglen]));
> }
indeed that's cleaner.
Like this then:
-static inline unsigned int sk_filter_len(const struct sk_filter *fp)
+static inline unsigned int sk_filter_size(unsigned int proglen)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(sizeof(struct sk_filter),
+ offsetof(struct sk_filter, insns[proglen]));
}
testing it... will send v4 shortly
^ permalink raw reply
* Re: [PATCH 1/1] hso: fix problem with wrong status code sent by OPTION GTM601 during RING indication
From: Dr. H. Nikolaus Schaller @ 2013-10-04 6:22 UTC (permalink / raw)
To: David Miller
Cc: j.dumon-x9gZzRpC1QbQT0dZR+AlfA,
marek.belisko-Re5JQEeQqe8AvxtiuMwx3w,
linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20131003.160049.199880461500344692.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Am 03.10.2013 um 22:00 schrieb David Miller:
> From: "Dr. H. Nikolaus Schaller" <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> Date: Thu, 3 Oct 2013 21:40:34 +0200
>
>> I have made the bug observation from debug log that this bit is set in a response
>> each time the modem has a RING message. It might be specific to this modem
>> and firmware version, i.e. a firmware bug. But we have no means to verify or even
>> change it in the firmware.
>>
>> I.e. the driver must handle it in a better way.
>>
>> Because the notification is rejected by the driver, the driver will hang up and the
>> whole modem connection breaks down.
>>
>> With this patch, the problem was never observed again in ~2 years.
>>
>> I'd hope the maintainer of this driver can shed some light on it.
>
> I think if you suspect the bit is set in response to RING messages
> then you should define a macro so that the number is not just magic,
> and put a descriptrive comment above the macro definition for that
> bit.
Good idea! I will resend the patch.
--- hns--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Eric Dumazet @ 2013-10-04 5:16 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Eric Dumazet, linux-arm-kernel,
linuxppc-dev, linux-s390, netdev
In-Reply-To: <1380859875-31025-1-git-send-email-ast@plumgrid.com>
On Thu, 2013-10-03 at 21:11 -0700, Alexei Starovoitov wrote:
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index a6ac848..5d66cd9 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -25,15 +25,20 @@ struct sk_filter
> {
> atomic_t refcnt;
> unsigned int len; /* Number of filter blocks */
> + struct rcu_head rcu;
> unsigned int (*bpf_func)(const struct sk_buff *skb,
> const struct sock_filter *filter);
> - struct rcu_head rcu;
> + /* insns start right after bpf_func, so that sk_run_filter() fetches
> + * first insn from the same cache line that was used to call into
> + * sk_run_filter()
> + */
> struct sock_filter insns[0];
> };
>
> static inline unsigned int sk_filter_len(const struct sk_filter *fp)
> {
> - return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
> + return max(fp->len * sizeof(struct sock_filter),
> + sizeof(struct work_struct)) + sizeof(*fp);
> }
I would use for include/linux/filter.h this (untested) diff :
(Note the include <linux/workqueue.h>)
I also remove your comment about cache lines, since there is nothing
to align stuff on a cache line boundary.
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..281b05c 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -6,6 +6,7 @@
#include <linux/atomic.h>
#include <linux/compat.h>
+#include <linux/workqueue.h>
#include <uapi/linux/filter.h>
#ifdef CONFIG_COMPAT
@@ -25,15 +26,20 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
- struct sock_filter insns[0];
+ union {
+ struct work_struct work;
+ struct sock_filter insns[0];
+ };
};
-static inline unsigned int sk_filter_len(const struct sk_filter *fp)
+static inline unsigned int sk_filter_size(const struct sk_filter *fp,
+ unsigned int proglen)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(sizeof(*fp),
+ offsetof(struct sk_filter, insns[proglen]));
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
This way, you can use sk_filter_size(fp, fprog->len)
instead of doing the max() games in sk_attach_filter() and
sk_unattached_filter_create()
Other than that, I think your patch is fine.
^ permalink raw reply related
* Re: [PATCH RFC 06/77] PCI/MSI: Factor out pci_get_msi_cap() interface
From: Alexander Gordeev @ 2013-10-04 5:13 UTC (permalink / raw)
To: Ben Hutchings
Cc: linux-kernel, Bjorn Helgaas, Ralf Baechle, Michael Ellerman,
Benjamin Herrenschmidt, Martin Schwidefsky, Ingo Molnar,
Tejun Heo, Dan Williams, Andy King, Jon Mason, Matt Porter,
linux-pci, linux-mips, linuxppc-dev, linux390, linux-s390, x86,
linux-ide, iss_storagedev, linux-nvme, linux-rdma, netdev,
e1000-devel, linux-dr
In-Reply-To: <1380837174.3419.21.camel@bwh-desktop.uk.level5networks.com>
On Thu, Oct 03, 2013 at 10:52:54PM +0100, Ben Hutchings wrote:
> On Wed, 2013-10-02 at 12:48 +0200, Alexander Gordeev wrote:
> > #ifndef CONFIG_PCI_MSI
> > +static inline int pci_get_msi_cap(struct pci_dev *dev)
> > +{
> > + return -1;
> [...]
>
> Shouldn't this also return -EINVAL?
Yep, all inliners here are better to return -EINVAL.
Will do unless someone speaks out against.
> Ben.
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: [PATCH] veth: Showing peer of veth type dev in ip link (kernel side)
From: Eric Dumazet @ 2013-10-04 4:49 UTC (permalink / raw)
To: Masatake YAMATO; +Cc: netdev
In-Reply-To: <1380859529-32351-1-git-send-email-yamato@redhat.com>
On Fri, 2013-10-04 at 13:05 +0900, Masatake YAMATO wrote:
> ip link has ability to show extra information of net work device if
> kernel provides sunh information. With this patch veth driver can
> provide its peer ifindex information to ip command via netlink
> interface.
> +
> + peer_ifindex = peer ? peer->ifindex : 0;
> + if (nla_put_u64(skb, VETH_INFO_PEER, peer_ifindex))
> + return -EMSGSIZE;
> +
ifindex are int, so you should use nla_put_u32()
^ permalink raw reply
* Re: [PATCH] xen-netback: Don't destroy the netdev until the vif is shut
From: David Miller @ 2013-10-04 4:22 UTC (permalink / raw)
To: Paul.Durrant; +Cc: netdev, wei.liu2
Can one of you do -stable backports of this change for v3.11 and any
other active -stable branch where this fix is relevant?
I gave it a shot, and it got outside my realm of expertise.
Thanks!
^ permalink raw reply
* Re: [stable 3.0] add some CVE fixes
From: Ben Hutchings @ 2013-10-04 4:18 UTC (permalink / raw)
To: Jiri Slaby; +Cc: Greg KH, stable, netdev
In-Reply-To: <524DC16D.90805@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]
On Thu, 2013-10-03 at 21:11 +0200, Jiri Slaby wrote:
> On 10/03/2013 08:44 PM, Greg KH wrote:
> > On Thu, Oct 03, 2013 at 11:20:28AM +0200, Jiri Slaby wrote:
> >> Plus the backports that are replied to this mail?
> >
> > I don't see any backports, did you forget to send them?
>
> I don't think so, they were sent and this is a log of one of them:
> OK. Log says:
> Sendmail: /usr/sbin/sendmail -f jslaby@suse.cz -i stable@vger.kernel.org
> jslaby@suse.cz
> From: Jiri Slaby <jslaby@suse.cz>
> To: <stable@vger.kernel.org>
> Cc: jslaby@suse.cz
> Subject: [PATCH 4/4] Tools: hv: verify origin of netlink connector message
> Date: Thu, 3 Oct 2013 11:23:50 +0200
> Message-Id: <1380792230-27255-4-git-send-email-jslaby@suse.cz>
> X-Mailer: git-send-email 1.8.4
> In-Reply-To: <1380792230-27255-1-git-send-email-jslaby@suse.cz>
> References: <524D36DC.5070506@suse.cz>
> <1380792230-27255-1-git-send-email-jslaby@suse.cz>
>
> Result: OK
>
> Could you check your spam folder? Or I can bounce them directly to you?
They didn't hit the list either. If they're applicable to 3.2 as well
then could you send them this way?
Ben.
--
Ben Hutchings
Life is like a sewer:
what you get out of it depends on what you put into it.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 4:11 UTC (permalink / raw)
To: David S. Miller
Cc: Daniel Borkmann, Eric Dumazet, linux-arm-kernel, linuxppc-dev,
linux-s390, netdev
on x86 system with net.core.bpf_jit_enable = 1
sudo tcpdump -i eth1 'tcp port 22'
causes the warning:
[ 56.766097] Possible unsafe locking scenario:
[ 56.766097]
[ 56.780146] CPU0
[ 56.786807] ----
[ 56.793188] lock(&(&vb->lock)->rlock);
[ 56.799593] <Interrupt>
[ 56.805889] lock(&(&vb->lock)->rlock);
[ 56.812266]
[ 56.812266] *** DEADLOCK ***
[ 56.812266]
[ 56.830670] 1 lock held by ksoftirqd/1/13:
[ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
[ 56.849757]
[ 56.849757] stack backtrace:
[ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
[ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
[ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
[ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
[ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
[ 56.923006] Call Trace:
[ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
[ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
[ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
[ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
[ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
[ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
[ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
[ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
[ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
[ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
[ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
[ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
[ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
[ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
[ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
[ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
[ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
[ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
[ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
[ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
[ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
[ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
cannot reuse jited filter memory, since it's readonly,
so use original bpf insns memory to hold work_struct
defer kfree of sk_filter until jit completed freeing
tested on x86_64 and i386
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/arm/net/bpf_jit_32.c | 1 +
arch/powerpc/net/bpf_jit_comp.c | 1 +
arch/s390/net/bpf_jit_comp.c | 4 +++-
arch/sparc/net/bpf_jit_comp.c | 1 +
arch/x86/net/bpf_jit_comp.c | 20 +++++++++++++++-----
include/linux/filter.h | 11 +++++++++--
net/core/filter.c | 11 +++++++----
7 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index f50d223..99b44e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..2345bdb 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 7092392..a5df511 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
struct bpf_binary_header *header = (void *)addr;
if (fp->bpf_func == sk_run_filter)
- return;
+ goto free_filter;
set_memory_rw(addr, header->pages);
module_free(NULL, header);
+free_filter:
+ kfree(fp);
}
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 9c7be59..218b6b2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79c216a..1396a0a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -772,13 +772,23 @@ out:
return;
}
+static void bpf_jit_free_deferred(struct work_struct *work)
+{
+ struct sk_filter *fp = container_of((void *)work, struct sk_filter,
+ insns);
+ unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
+ struct bpf_binary_header *header = (void *)addr;
+
+ set_memory_rw(addr, header->pages);
+ module_free(NULL, header);
+ kfree(fp);
+}
+
void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter) {
- unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
- struct bpf_binary_header *header = (void *)addr;
-
- set_memory_rw(addr, header->pages);
- module_free(NULL, header);
+ struct work_struct *work = (struct work_struct *)fp->insns;
+ INIT_WORK(work, bpf_jit_free_deferred);
+ schedule_work(work);
}
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..5d66cd9 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -25,15 +25,20 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
+ /* insns start right after bpf_func, so that sk_run_filter() fetches
+ * first insn from the same cache line that was used to call into
+ * sk_run_filter()
+ */
struct sock_filter insns[0];
};
static inline unsigned int sk_filter_len(const struct sk_filter *fp)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(fp->len * sizeof(struct sock_filter),
+ sizeof(struct work_struct)) + sizeof(*fp);
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
}
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
#else
+#include <linux/slab.h>
static inline void bpf_jit_compile(struct sk_filter *fp)
{
}
static inline void bpf_jit_free(struct sk_filter *fp)
{
+ kfree(fp);
}
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
#endif
diff --git a/net/core/filter.c b/net/core/filter.c
index 6438f29..ad5eaba 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
bpf_jit_free(fp);
- kfree(fp);
}
EXPORT_SYMBOL(sk_filter_release_rcu);
@@ -677,13 +676,15 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
{
struct sk_filter *fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
/* Make sure new filter is there and in the right amounts. */
if (fprog->filter == NULL)
return -EINVAL;
- fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
+ fp = kmalloc(sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +724,8 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
{
struct sk_filter *fp, *old_fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +735,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
if (fprog->filter == NULL)
return -EINVAL;
- fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
+ fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
- sock_kfree_s(sk, fp, fsize+sizeof(*fp));
+ sock_kfree_s(sk, fp, sk_fsize);
return -EFAULT;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH] ip: Showing peer of veth type dev in ip link (ip cmd side)
From: Masatake YAMATO @ 2013-10-04 4:06 UTC (permalink / raw)
To: netdev; +Cc: yamato
Implement print_opt method to veth to show peer ifindex
as ethtool -S does.
A patch submitted with following subject is needed:
veth: Showing peer of veth type dev in ip link (kernel side)
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
---
ip/link_veth.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/ip/link_veth.c b/ip/link_veth.c
index 7730f39..bd84815 100644
--- a/ip/link_veth.c
+++ b/ip/link_veth.c
@@ -11,6 +11,7 @@
*/
#include <string.h>
+#include <inttypes.h>
#include <net/if.h>
#include <linux/veth.h>
@@ -57,7 +58,22 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
return argc - 1 - err;
}
+static void veth_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
+{
+ if (!tb)
+ return;
+
+ if (tb[VETH_INFO_PEER] &&
+ RTA_PAYLOAD(tb[VETH_INFO_PEER]) < sizeof(__u64))
+ return;
+
+ fprintf(f, "peer_ifindex %"PRIu64,
+ (uint64_t)rta_getattr_u64(tb[VETH_INFO_PEER]));
+}
+
struct link_util veth_link_util = {
.id = "veth",
+ .maxattr = VETH_INFO_MAX,
.parse_opt = veth_parse_opt,
+ .print_opt = veth_print_opt,
};
--
1.8.3.1
^ permalink raw reply related
* [PATCH] veth: Showing peer of veth type dev in ip link (kernel side)
From: Masatake YAMATO @ 2013-10-04 4:05 UTC (permalink / raw)
To: netdev; +Cc: yamato
ip link has ability to show extra information of net work device if
kernel provides sunh information. With this patch veth driver can
provide its peer ifindex information to ip command via netlink
interface.
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
---
drivers/net/veth.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index eee1f19..54187b9 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -434,6 +434,25 @@ static const struct nla_policy veth_policy[VETH_INFO_MAX + 1] = {
[VETH_INFO_PEER] = { .len = sizeof(struct ifinfomsg) },
};
+static size_t veth_get_size(const struct net_device *dev)
+{
+ return nla_total_size(sizeof(u64)) + /* VETH_INFO_PEER */
+ 0;
+}
+
+static int veth_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+ struct veth_priv *priv = netdev_priv(dev);
+ struct net_device *peer = rtnl_dereference(priv->peer);
+ u64 peer_ifindex;
+
+ peer_ifindex = peer ? peer->ifindex : 0;
+ if (nla_put_u64(skb, VETH_INFO_PEER, peer_ifindex))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
static struct rtnl_link_ops veth_link_ops = {
.kind = DRV_NAME,
.priv_size = sizeof(struct veth_priv),
@@ -443,6 +462,8 @@ static struct rtnl_link_ops veth_link_ops = {
.dellink = veth_dellink,
.policy = veth_policy,
.maxtype = VETH_INFO_MAX,
+ .get_size = veth_get_size,
+ .fill_info = veth_fill_info,
};
/*
--
1.8.3.1
^ permalink raw reply related
* RE: tx checksum offload in rtl8168evl disabled in driver
From: hayeswang @ 2013-10-04 3:10 UTC (permalink / raw)
To: 'Francois Romieu', jason.morgan; +Cc: netdev, 'nic_swsd'
In-Reply-To: <20131003230120.GA25047@electric-eye.fr.zoreil.com>
Francois Romieu [mailto:romieu@fr.zoreil.com]
[...]
> > I'm using 2k to 4k frames with a rtl8168evl.
> > I've found this message
> > http://www.spinics.net/lists/netdev/msg216530.html
> [...]
> > However the message thread, above indicates that this is
> not a problem and
> > can be changed to make tx-checksum offload possible.
> >
> > However we are using a newer chip to the on in the message
> thread. I've
> > tried to find other, more recent citations without success.
> >
> > So, why is it still turned off ?
>
> It has been disabled since
> d58d46b5d85139d18eb939aa7279c160bab70484 ("r8169:
> jumbo fixes"). Patch was submitted as a RFC on 2011/07/17 and
> Hayes was
> explicitely requested to comment on the jumbo part if
> necessary. Patch was
> submitted for inclusion on 2011/09/22.
>
> Tx checksumming and jumbo are mutually exclusive in Realtek's
> driver as well.
>
> It seems no recent gigabit chipset reliably supports it.
>
> > What will be the effect of turning it on (changing false to
> true, in the
> > driver line) for our chip ?
Since RTL8111E, the chips support a feature which we call "tx early".
For jumbo frame, the hw starts to send a packet before getting the whole data.
However, the checksum has to be calculated after the whole data are fetched.
Therefore, the jumbo frame and the tx checksum couldn't be enable at the same
time, otherwise a packet with incorrect checksum would be sent.
Best Regards,
Hayes
^ permalink raw reply
* Re: [PATCH v2.41 5/5] datapath: Add basic MPLS support to kernel
From: Pravin Shelar @ 2013-10-04 2:46 UTC (permalink / raw)
To: Simon Horman
Cc: dev@openvswitch.org, netdev, Jesse Gross, Ben Pfaff, Ravi K,
Isaku Yamahata, Joe Stringer
In-Reply-To: <20131003002052.GA13111@verge.net.au>
On Wed, Oct 2, 2013 at 5:20 PM, Simon Horman <horms@verge.net.au> wrote:
> On Wed, Oct 02, 2013 at 11:03:57AM -0700, Pravin Shelar wrote:
>> On Mon, Sep 30, 2013 at 11:47 PM, Simon Horman <horms@verge.net.au> wrote:
>> > Allow datapath to recognize and extract MPLS labels into flow keys
>> > and execute actions which push, pop, and set labels on packets.
>> >
>> > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
>> >
>> > Cc: Ravi K <rkerur@gmail.com>
>> > Cc: Leo Alterman <lalterman@nicira.com>
>> > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
>> > Cc: Joe Stringer <joe@wand.net.nz>
>> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >
>> > ---
>> >
>> > +
>> > + /* this hack needed to get regular skb_gso_segment() */
>> > +#ifdef HAVE___SKB_GSO_SEGMENT
>> > +#undef __skb_gso_segment
>> > + skb_gso = __skb_gso_segment(skb, features, tx_path);
>> > +#else
>> > +#undef skb_gso_segment
>> > + skb_gso = skb_gso_segment(skb, features);
>> > +#endif
>> > +
>>
>> We can get rid of #ifdefs by just using different name for
>> rpl___skb_gso_segment(), something like mpls_vlan_skb_gso_segment().
>> The way it is done for tnl-gso.
>
> Thanks.
>
> The reason that I had the code arranged this way was so that
> calls to __skb_gso_segment() would go via rpl___skb_gso_segment()
> on kernels older than v3.11. In particular calls outside of gso.c.
>
> On closer examination the only such case is in ovs_dp_upcall().
> Currently there should be no need to perform MPLS GSO segmentation in that
> case because MPLS GSO segmentation can only be needed after actions are
> applied.
>
> However, I am concerned that it may be necessary later when
> recirculation is introduced as in that case an upcall may occur
> on a packet which has had actions applied.
good point.
currently we define __skb_gso_segment using skb_gso_segemt(). You have
reversed it. Is there any reason?
if you keep it as it is, it can simplify code a bit.
^ permalink raw reply
* Re: [PATCH v2 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 2:26 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, netdev, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Daniel Borkmann, Paul E. McKenney, Xi Wang, x86,
Eric Dumazet, Heiko Carstens, linux-kernel
In-Reply-To: <CAMEtUuz4Qyg1s7CocNotTYXcn1odcbT5nek1T66dd+dK3ukfTw@mail.gmail.com>
On Thu, Oct 3, 2013 at 4:11 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Thu, Oct 3, 2013 at 4:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Thu, 2013-10-03 at 15:47 -0700, Alexei Starovoitov wrote:
>>
>>> @@ -722,7 +725,8 @@ EXPORT_SYMBOL_GPL(sk_unattached_filter_destroy);
>>> int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>>> {
>>> struct sk_filter *fp, *old_fp;
>>> - unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
>>> + unsigned int fsize = max(sizeof(struct sock_filter) * fprog->len,
>>> + sizeof(struct work_struct));
>>> int err;
>>>
>>> if (sock_flag(sk, SOCK_FILTER_LOCKED))
>>
>> Thats broken, as we might copy more data from user than expected,
>> and eventually trigger EFAULT :
>>
>> if (copy_from_user(fp->insns, fprog->filter, fsize)) {
>
> yes. will fix.
tested on x86_64/i386 only
with tcpdump and netsniff 1-4k filter size.
Thank you for careful review.
^ permalink raw reply
* [PATCH v3 net-next] fix unsafe set_memory_rw from softirq
From: Alexei Starovoitov @ 2013-10-04 2:24 UTC (permalink / raw)
To: David S. Miller
Cc: Benjamin Herrenschmidt, Heiko Carstens, Eric Dumazet,
Paul Mackerras, H. Peter Anvin, sparclinux, Nicolas Dichtel,
Alexei Starovoitov, linux-s390, Russell King, x86, James Morris,
Ingo Molnar, Alexey Kuznetsov, Paul E. McKenney, Xi Wang,
Matt Evans, Thomas Gleixner, linux-arm-kernel, Stelian Nirlu,
Nicolas Schichan, Hideaki YOSHIFUJI, netdev, linux-kernel,
Mircea Gherzan <m
on x86 system with net.core.bpf_jit_enable = 1
sudo tcpdump -i eth1 'tcp port 22'
causes the warning:
[ 56.766097] Possible unsafe locking scenario:
[ 56.766097]
[ 56.780146] CPU0
[ 56.786807] ----
[ 56.793188] lock(&(&vb->lock)->rlock);
[ 56.799593] <Interrupt>
[ 56.805889] lock(&(&vb->lock)->rlock);
[ 56.812266]
[ 56.812266] *** DEADLOCK ***
[ 56.812266]
[ 56.830670] 1 lock held by ksoftirqd/1/13:
[ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380
[ 56.849757]
[ 56.849757] stack backtrace:
[ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45
[ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012
[ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007
[ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001
[ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001
[ 56.923006] Call Trace:
[ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76
[ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208
[ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50
[ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150
[ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0
[ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50
[ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50
[ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90
[ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0
[ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50
[ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380
[ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380
[ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380
[ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460
[ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10
[ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0
[ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0
[ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40
[ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40
[ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30
[ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0
[ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0
[ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70
cannot reuse jited filter memory, since it's readonly,
so use original bpf insns memory to hold work_struct
defer kfree of sk_filter until jit completed freeing
tested on x86_64 and i386
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
arch/arm/net/bpf_jit_32.c | 1 +
arch/powerpc/net/bpf_jit_comp.c | 1 +
arch/s390/net/bpf_jit_comp.c | 4 +++-
arch/sparc/net/bpf_jit_comp.c | 1 +
arch/x86/net/bpf_jit_comp.c | 20 +++++++++++++++-----
include/linux/filter.h | 11 +++++++++--
net/core/filter.c | 11 +++++++----
7 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index f50d223..99b44e0 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index bf56e33..2345bdb 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 7092392..a5df511 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
struct bpf_binary_header *header = (void *)addr;
if (fp->bpf_func == sk_run_filter)
- return;
+ goto free_filter;
set_memory_rw(addr, header->pages);
module_free(NULL, header);
+free_filter:
+ kfree(fp);
}
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 9c7be59..218b6b2 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter)
module_free(NULL, fp->bpf_func);
+ kfree(fp);
}
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79c216a..1396a0a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -772,13 +772,23 @@ out:
return;
}
+static void bpf_jit_free_deferred(struct work_struct *work)
+{
+ struct sk_filter *fp = container_of((void *)work, struct sk_filter,
+ insns);
+ unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
+ struct bpf_binary_header *header = (void *)addr;
+
+ set_memory_rw(addr, header->pages);
+ module_free(NULL, header);
+ kfree(fp);
+}
+
void bpf_jit_free(struct sk_filter *fp)
{
if (fp->bpf_func != sk_run_filter) {
- unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
- struct bpf_binary_header *header = (void *)addr;
-
- set_memory_rw(addr, header->pages);
- module_free(NULL, header);
+ struct work_struct *work = (struct work_struct *)fp->insns;
+ INIT_WORK(work, bpf_jit_free_deferred);
+ schedule_work(work);
}
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index a6ac848..5d66cd9 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -25,15 +25,20 @@ struct sk_filter
{
atomic_t refcnt;
unsigned int len; /* Number of filter blocks */
+ struct rcu_head rcu;
unsigned int (*bpf_func)(const struct sk_buff *skb,
const struct sock_filter *filter);
- struct rcu_head rcu;
+ /* insns start right after bpf_func, so that sk_run_filter() fetches
+ * first insn from the same cache line that was used to call into
+ * sk_run_filter()
+ */
struct sock_filter insns[0];
};
static inline unsigned int sk_filter_len(const struct sk_filter *fp)
{
- return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
+ return max(fp->len * sizeof(struct sock_filter),
+ sizeof(struct work_struct)) + sizeof(*fp);
}
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
}
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
#else
+#include <linux/slab.h>
static inline void bpf_jit_compile(struct sk_filter *fp)
{
}
static inline void bpf_jit_free(struct sk_filter *fp)
{
+ kfree(fp);
}
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
#endif
diff --git a/net/core/filter.c b/net/core/filter.c
index 6438f29..ad5eaba 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
bpf_jit_free(fp);
- kfree(fp);
}
EXPORT_SYMBOL(sk_filter_release_rcu);
@@ -677,13 +676,15 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
{
struct sk_filter *fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
/* Make sure new filter is there and in the right amounts. */
if (fprog->filter == NULL)
return -EINVAL;
- fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
+ fp = kmalloc(sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +724,8 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
{
struct sk_filter *fp, *old_fp;
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
+ unsigned int sk_fsize = max_t(u32, fsize, sizeof(struct work_struct))
+ + sizeof(*fp);
int err;
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +735,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
if (fprog->filter == NULL)
return -EINVAL;
- fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
+ fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
if (!fp)
return -ENOMEM;
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
- sock_kfree_s(sk, fp, fsize+sizeof(*fp));
+ sock_kfree_s(sk, fp, sk_fsize);
return -EFAULT;
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH RFC 01/77] PCI/MSI: Fix return value when populate_msi_sysfs() failed
From: Jon Mason @ 2013-10-04 0:59 UTC (permalink / raw)
To: Ben Hutchings
Cc: Alexander Gordeev, linux-kernel, Bjorn Helgaas, Ralf Baechle,
Michael Ellerman, Benjamin Herrenschmidt, Martin Schwidefsky,
Ingo Molnar, Tejun Heo, Dan Williams, Andy King, Matt Porter,
stable, linux-pci, linux-mips, linuxppc-dev, linux390, linux-s390,
x86, linux-ide, iss_storagedev, linux-nvme, linux-rdma, netdev,
e1000-devel, linux-driver, Solarflare linux maintainers
In-Reply-To: <1380836781.3419.17.camel@bwh-desktop.uk.level5networks.com>
On Thu, Oct 03, 2013 at 10:46:21PM +0100, Ben Hutchings wrote:
> On Wed, 2013-10-02 at 17:39 -0700, Jon Mason wrote:
> > On Wed, Oct 02, 2013 at 12:48:17PM +0200, Alexander Gordeev wrote:
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> >
> > Since you are changing the behavior of the msix_capability_init
> > function on populate_msi_sysfs error, a comment describing why in this
> > commit would be nice.
> [...]
>
> This function was already treating that error as fatal, and freeing the
> MSIs. The change in behaviour is that it now returns the error code in
> this case, rather than 0. This is obviously correct and properly
> described by the one-line summary.
If someone dumb, like me, is looking at this commit and trying to
figure out what is happening, having ANY commit message is good. "Fix
the return value" doesn't tell me anything. Documenting what issue(s)
would've been seen had the error case been encountered and what will
now been seen would be very nice.
>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox