* [PATCH v2 net-next 1/4] net/sched: act_mirred: Rename tcfm_ok_push to tcfm_mac_header_xmit and make it a bool
2016-09-27 20:59 [PATCH v2 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
@ 2016-09-27 20:59 ` Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 2/4] net/sched: act_mirred: Refactor detection whether dev needs xmit at mac header Shmulik Ladkani
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Shmulik Ladkani @ 2016-09-27 20:59 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
Florian Westphal, netdev, Shmulik Ladkani
'tcfm_ok_push' specifies whether a mac_len sized push is needed upon
egress to the target device (if action is performed at ingress).
Rename it to 'tcfm_mac_header_xmit' as this is actually an attribute of
the target device (and use a bool instead of int).
This allows to decouple the attribute from the action to be taken.
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
v2: declare tcfm_mac_header_xmit as bool instead of int
include/net/tc_act/tc_mirred.h | 2 +-
net/sched/act_mirred.c | 11 ++++++-----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index 62770add15..95431092c4 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -8,7 +8,7 @@ struct tcf_mirred {
struct tc_action common;
int tcfm_eaction;
int tcfm_ifindex;
- int tcfm_ok_push;
+ bool tcfm_mac_header_xmit;
struct net_device __rcu *tcfm_dev;
struct list_head tcfm_list;
};
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 667dc382df..16e17a887b 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -60,11 +60,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
{
struct tc_action_net *tn = net_generic(net, mirred_net_id);
struct nlattr *tb[TCA_MIRRED_MAX + 1];
+ bool mac_header_xmit = false;
struct tc_mirred *parm;
struct tcf_mirred *m;
struct net_device *dev;
- int ret, ok_push = 0;
bool exists = false;
+ int ret;
if (nla == NULL)
return -EINVAL;
@@ -102,10 +103,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
case ARPHRD_IPGRE:
case ARPHRD_VOID:
case ARPHRD_NONE:
- ok_push = 0;
+ mac_header_xmit = false;
break;
default:
- ok_push = 1;
+ mac_header_xmit = true;
break;
}
} else {
@@ -136,7 +137,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
dev_hold(dev);
rcu_assign_pointer(m->tcfm_dev, dev);
- m->tcfm_ok_push = ok_push;
+ m->tcfm_mac_header_xmit = mac_header_xmit;
}
if (ret == ACT_P_CREATED) {
@@ -181,7 +182,7 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
goto out;
if (!(at & AT_EGRESS)) {
- if (m->tcfm_ok_push)
+ if (m->tcfm_mac_header_xmit)
skb_push_rcsum(skb2, skb->mac_len);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 net-next 2/4] net/sched: act_mirred: Refactor detection whether dev needs xmit at mac header
2016-09-27 20:59 [PATCH v2 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 1/4] net/sched: act_mirred: Rename tcfm_ok_push to tcfm_mac_header_xmit and make it a bool Shmulik Ladkani
@ 2016-09-27 20:59 ` Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror' Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
3 siblings, 0 replies; 9+ messages in thread
From: Shmulik Ladkani @ 2016-09-27 20:59 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
Florian Westphal, netdev, Shmulik Ladkani
Move detection logic that tests whether device expects skb data to point
at mac_header upon xmit into a function.
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
net/sched/act_mirred.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 16e17a887b..69dcce8c75 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -54,6 +54,20 @@ static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
static int mirred_net_id;
static struct tc_action_ops act_mirred_ops;
+static bool dev_is_mac_header_xmit(const struct net_device *dev)
+{
+ switch (dev->type) {
+ case ARPHRD_TUNNEL:
+ case ARPHRD_TUNNEL6:
+ case ARPHRD_SIT:
+ case ARPHRD_IPGRE:
+ case ARPHRD_VOID:
+ case ARPHRD_NONE:
+ return false;
+ }
+ return true;
+}
+
static int tcf_mirred_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
int bind)
@@ -96,19 +110,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
tcf_hash_release(*a, bind);
return -ENODEV;
}
- switch (dev->type) {
- case ARPHRD_TUNNEL:
- case ARPHRD_TUNNEL6:
- case ARPHRD_SIT:
- case ARPHRD_IPGRE:
- case ARPHRD_VOID:
- case ARPHRD_NONE:
- mac_header_xmit = false;
- break;
- default:
- mac_header_xmit = true;
- break;
- }
+ mac_header_xmit = dev_is_mac_header_xmit(dev);
} else {
dev = NULL;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror'
2016-09-27 20:59 [PATCH v2 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 1/4] net/sched: act_mirred: Rename tcfm_ok_push to tcfm_mac_header_xmit and make it a bool Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 2/4] net/sched: act_mirred: Refactor detection whether dev needs xmit at mac header Shmulik Ladkani
@ 2016-09-27 20:59 ` Shmulik Ladkani
2016-09-27 20:59 ` [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
3 siblings, 0 replies; 9+ messages in thread
From: Shmulik Ladkani @ 2016-09-27 20:59 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
Florian Westphal, netdev, Shmulik Ladkani, Hariprasad S,
Jeff Kirsher, Saeed Mahameed, Jiri Pirko, Ido Schimmel,
Jakub Kicinski
These accessors are used in various drivers that support tc offloading,
to detect properties of a given 'tc_action'.
'is_tcf_mirred_redirect' tests that the action is TCA_EGRESS_REDIR.
'is_tcf_mirred_mirror' tests that the action is TCA_EGRESS_MIRROR.
As a prep towards supporting INGRESS redir/mirror, rename these
predicates to reflect their true meaning:
s/is_tcf_mirred_redirect/is_tcf_mirred_egress_redirect/
s/is_tcf_mirred_mirror/is_tcf_mirred_egress_mirror/
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Cc: Hariprasad S <hariprasad@chelsio.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Ido Schimmel <idosch@mellanox.com>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +-
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 4 +++-
drivers/net/ethernet/netronome/nfp/nfp_net_offload.c | 2 +-
include/net/tc_act/tc_mirred.h | 4 ++--
6 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
index 49d2debb33..52af62e0ec 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
@@ -113,7 +113,7 @@ static int fill_action_fields(struct adapter *adap,
}
/* Re-direct to specified port in hardware. */
- if (is_tcf_mirred_redirect(a)) {
+ if (is_tcf_mirred_egress_redirect(a)) {
struct net_device *n_dev;
unsigned int i, index;
bool found = false;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a244d9a672..784b0b98ab 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8410,7 +8410,7 @@ static int parse_tc_actions(struct ixgbe_adapter *adapter,
}
/* Redirect to a VF or a offloaded macvlan */
- if (is_tcf_mirred_redirect(a)) {
+ if (is_tcf_mirred_egress_redirect(a)) {
int ifindex = tcf_mirred_ifindex(a);
err = handle_redirect_action(adapter, ifindex, queue,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index a350b7171e..957a464489 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -404,7 +404,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv, struct tcf_exts *exts,
continue;
}
- if (is_tcf_mirred_redirect(a)) {
+ if (is_tcf_mirred_egress_redirect(a)) {
int ifindex = tcf_mirred_ifindex(a);
struct net_device *out_dev;
struct mlx5e_priv *out_priv;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index fd74d1064f..6a4f9c4664 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -1237,8 +1237,10 @@ static int mlxsw_sp_port_add_cls_matchall(struct mlxsw_sp_port *mlxsw_sp_port,
tcf_exts_to_list(cls->exts, &actions);
list_for_each_entry(a, &actions, list) {
- if (!is_tcf_mirred_mirror(a) || protocol != htons(ETH_P_ALL))
+ if (!is_tcf_mirred_egress_mirror(a) ||
+ protocol != htons(ETH_P_ALL)) {
return -ENOTSUPP;
+ }
err = mlxsw_sp_port_add_cls_matchall_mirror(mlxsw_sp_port, cls,
a, ingress);
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_offload.c b/drivers/net/ethernet/netronome/nfp/nfp_net_offload.c
index 8acfb631a0..cfed40c0e3 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_offload.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_offload.c
@@ -128,7 +128,7 @@ nfp_net_bpf_get_act(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
if (is_tcf_gact_shot(a))
return NN_ACT_TC_DROP;
- if (is_tcf_mirred_redirect(a) &&
+ if (is_tcf_mirred_egress_redirect(a) &&
tcf_mirred_ifindex(a) == nn->netdev->ifindex)
return NN_ACT_TC_REDIR;
}
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index 95431092c4..604bc31e23 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -14,7 +14,7 @@ struct tcf_mirred {
};
#define to_mirred(a) ((struct tcf_mirred *)a)
-static inline bool is_tcf_mirred_redirect(const struct tc_action *a)
+static inline bool is_tcf_mirred_egress_redirect(const struct tc_action *a)
{
#ifdef CONFIG_NET_CLS_ACT
if (a->ops && a->ops->type == TCA_ACT_MIRRED)
@@ -23,7 +23,7 @@ static inline bool is_tcf_mirred_redirect(const struct tc_action *a)
return false;
}
-static inline bool is_tcf_mirred_mirror(const struct tc_action *a)
+static inline bool is_tcf_mirred_egress_mirror(const struct tc_action *a)
{
#ifdef CONFIG_NET_CLS_ACT
if (a->ops && a->ops->type == TCA_ACT_MIRRED)
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-27 20:59 [PATCH v2 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
` (2 preceding siblings ...)
2016-09-27 20:59 ` [PATCH v2 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror' Shmulik Ladkani
@ 2016-09-27 20:59 ` Shmulik Ladkani
2016-09-27 21:27 ` Eric Dumazet
3 siblings, 1 reply; 9+ messages in thread
From: Shmulik Ladkani @ 2016-09-27 20:59 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
Florian Westphal, netdev, Shmulik Ladkani
Up until now, 'action mirred' supported only egress actions (either
TCA_EGRESS_REDIR or TCA_EGRESS_MIRROR).
This patch implements the corresponding ingress actions
TCA_INGRESS_REDIR and TCA_INGRESS_MIRROR.
This allows attaching filters whose target is to hand matching skbs into
the rx processing of a specified device.
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_mirred.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 69dcce8c75..21f0f5f868 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -33,6 +33,25 @@
static LIST_HEAD(mirred_list);
static DEFINE_SPINLOCK(mirred_list_lock);
+static bool tcf_mirred_is_act_redirect(int action)
+{
+ return action == TCA_EGRESS_REDIR || action == TCA_INGRESS_REDIR;
+}
+
+static u32 tcf_mirred_act_direction(int action)
+{
+ switch (action) {
+ case TCA_EGRESS_REDIR:
+ case TCA_EGRESS_MIRROR:
+ return AT_EGRESS;
+ case TCA_INGRESS_REDIR:
+ case TCA_INGRESS_MIRROR:
+ return AT_INGRESS;
+ default:
+ BUG();
+ }
+}
+
static void tcf_mirred_release(struct tc_action *a, int bind)
{
struct tcf_mirred *m = to_mirred(a);
@@ -97,6 +116,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
switch (parm->eaction) {
case TCA_EGRESS_MIRROR:
case TCA_EGRESS_REDIR:
+ case TCA_INGRESS_REDIR:
+ case TCA_INGRESS_MIRROR:
break;
default:
if (exists)
@@ -158,7 +179,8 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
struct tcf_mirred *m = to_mirred(a);
struct net_device *dev;
struct sk_buff *skb2;
- int retval, err;
+ int retval, err = 0;
+ int mac_len;
u32 at;
tcf_lastuse_update(&m->tcf_tm);
@@ -183,23 +205,37 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
if (!skb2)
goto out;
- if (!(at & AT_EGRESS)) {
- if (m->tcfm_mac_header_xmit)
+ /* If action's target direction differs than filter's direction,
+ * and devices expect a mac header on xmit, then mac push/pull is
+ * needed.
+ */
+ if (at != tcf_mirred_act_direction(m->tcfm_eaction) &&
+ m->tcfm_mac_header_xmit) {
+ if (at & AT_EGRESS) {
+ /* caught at egress, act ingress: pull mac */
+ mac_len = skb_network_header(skb) - skb_mac_header(skb);
+ skb_pull_rcsum(skb2, mac_len);
+ } else {
+ /* caught at ingress, act egress: push mac */
skb_push_rcsum(skb2, skb->mac_len);
+ }
}
/* mirror is always swallowed */
- if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
+ if (tcf_mirred_is_act_redirect(m->tcfm_eaction))
skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);
skb2->skb_iif = skb->dev->ifindex;
skb2->dev = dev;
- err = dev_queue_xmit(skb2);
+ if (tcf_mirred_act_direction(m->tcfm_eaction) & AT_EGRESS)
+ err = dev_queue_xmit(skb2);
+ else
+ netif_receive_skb(skb2);
if (err) {
out:
qstats_overlimit_inc(this_cpu_ptr(m->common.cpu_qstats));
- if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
+ if (tcf_mirred_is_act_redirect(m->tcfm_eaction))
retval = TC_ACT_SHOT;
}
rcu_read_unlock();
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-27 20:59 ` [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
@ 2016-09-27 21:27 ` Eric Dumazet
2016-09-27 21:42 ` Shmulik Ladkani
2016-09-29 10:35 ` Shmulik Ladkani
0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2016-09-27 21:27 UTC (permalink / raw)
To: Shmulik Ladkani
Cc: David Miller, Jamal Hadi Salim, WANG Cong, Eric Dumazet,
Daniel Borkmann, Florian Westphal, netdev
On Tue, 2016-09-27 at 23:59 +0300, Shmulik Ladkani wrote:
> Up until now, 'action mirred' supported only egress actions (either
> TCA_EGRESS_REDIR or TCA_EGRESS_MIRROR).
>
> This patch implements the corresponding ingress actions
> TCA_INGRESS_REDIR and TCA_INGRESS_MIRROR.
> - if (m->tcfm_mac_header_xmit)
> + /* If action's target direction differs than filter's direction,
> + * and devices expect a mac header on xmit, then mac push/pull is
> + * needed.
> + */
> + if (at != tcf_mirred_act_direction(m->tcfm_eaction) &&
Note that m->tcfm_eaction is read here.
> + m->tcfm_mac_header_xmit) {
> + if (at & AT_EGRESS) {
> + /* caught at egress, act ingress: pull mac */
> + mac_len = skb_network_header(skb) - skb_mac_header(skb);
> + skb_pull_rcsum(skb2, mac_len);
> + } else {
> + /* caught at ingress, act egress: push mac */
> skb_push_rcsum(skb2, skb->mac_len);
> + }
> }
>
> /* mirror is always swallowed */
> - if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
> + if (tcf_mirred_is_act_redirect(m->tcfm_eaction))
> skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);
>
> skb2->skb_iif = skb->dev->ifindex;
> skb2->dev = dev;
> - err = dev_queue_xmit(skb2);
Note that m->tcfm_eaction is read another time here.
> + if (tcf_mirred_act_direction(m->tcfm_eaction) & AT_EGRESS)
> + err = dev_queue_xmit(skb2);
> + else
> + netif_receive_skb(skb2);
>
Since this runs lockless, another cpu might change m->tcfm_eaction in
the middle, and you could call dev_queue_xmit(skb2) while the skb2 was
prepared for the opposite action.
I guess some drivers could crash, because they expect to find a MAC
header.
If not, a comment would be nice.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-27 21:27 ` Eric Dumazet
@ 2016-09-27 21:42 ` Shmulik Ladkani
2016-09-27 22:32 ` Eric Dumazet
2016-09-29 10:35 ` Shmulik Ladkani
1 sibling, 1 reply; 9+ messages in thread
From: Shmulik Ladkani @ 2016-09-27 21:42 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Jamal Hadi Salim, WANG Cong, Eric Dumazet,
Daniel Borkmann, Florian Westphal, netdev
Hi,
On Tue, 27 Sep 2016 14:27:13 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-09-27 at 23:59 +0300, Shmulik Ladkani wrote:
> > Up until now, 'action mirred' supported only egress actions (either
> > TCA_EGRESS_REDIR or TCA_EGRESS_MIRROR).
> >
> > This patch implements the corresponding ingress actions
> > TCA_INGRESS_REDIR and TCA_INGRESS_MIRROR.
>
>
> > - if (m->tcfm_mac_header_xmit)
> > + /* If action's target direction differs than filter's direction,
> > + * and devices expect a mac header on xmit, then mac push/pull is
> > + * needed.
> > + */
> > + if (at != tcf_mirred_act_direction(m->tcfm_eaction) &&
>
> Note that m->tcfm_eaction is read here.
>
> > + m->tcfm_mac_header_xmit) {
> > + if (at & AT_EGRESS) {
> > + /* caught at egress, act ingress: pull mac */
> > + mac_len = skb_network_header(skb) - skb_mac_header(skb);
> > + skb_pull_rcsum(skb2, mac_len);
> > + } else {
> > + /* caught at ingress, act egress: push mac */
> > skb_push_rcsum(skb2, skb->mac_len);
> > + }
> > }
> >
> > /* mirror is always swallowed */
> > - if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
> > + if (tcf_mirred_is_act_redirect(m->tcfm_eaction))
> > skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);
> >
> > skb2->skb_iif = skb->dev->ifindex;
> > skb2->dev = dev;
> > - err = dev_queue_xmit(skb2);
>
> Note that m->tcfm_eaction is read another time here.
>
> > + if (tcf_mirred_act_direction(m->tcfm_eaction) & AT_EGRESS)
> > + err = dev_queue_xmit(skb2);
> > + else
> > + netif_receive_skb(skb2);
> >
>
> Since this runs lockless, another cpu might change m->tcfm_eaction in
> the middle, and you could call dev_queue_xmit(skb2) while the skb2 was
> prepared for the opposite action.
Thanks Eric.
I assume adding a READ_ONCE(m->tcfm_eaction) at beggining of section,
and using the read value, will solve this specific inconsistency?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-27 21:42 ` Shmulik Ladkani
@ 2016-09-27 22:32 ` Eric Dumazet
0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2016-09-27 22:32 UTC (permalink / raw)
To: Shmulik Ladkani
Cc: David Miller, Jamal Hadi Salim, WANG Cong, Eric Dumazet,
Daniel Borkmann, Florian Westphal, netdev
On Wed, 2016-09-28 at 00:42 +0300, Shmulik Ladkani wrote:
> Thanks Eric.
>
> I assume adding a READ_ONCE(m->tcfm_eaction) at beggining of section,
> and using the read value, will solve this specific inconsistency?
Sure, adding a READ_ONCE() might work, if done properly ;)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-27 21:27 ` Eric Dumazet
2016-09-27 21:42 ` Shmulik Ladkani
@ 2016-09-29 10:35 ` Shmulik Ladkani
1 sibling, 0 replies; 9+ messages in thread
From: Shmulik Ladkani @ 2016-09-29 10:35 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Jamal Hadi Salim, WANG Cong, Eric Dumazet,
Daniel Borkmann, Florian Westphal, netdev
Hi Eric,
On Tue, 27 Sep 2016 14:27:13 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Since this runs lockless, another cpu might change m->tcfm_eaction in
> the middle, and you could call dev_queue_xmit(skb2) while the skb2 was
> prepared for the opposite action.
Well, seem members of 'struct tcf_mirred' are out of sync wrt to each
other, even in existing code, regadless this patch:
- 'tcfm_dev' may be assigned, but 'tcfm_ok_push' not yet updated,
may result in skb_push_rcsum being called/not called
- 'tcfm_eaction' is changed, in between "mirror is always swallowed" to
the final 'out:' label,
may result in wrong tc_verd assigned (or lack of assignment)
Seems the whole "params" need be rcu_dereferenced, like in
tunnel_key_act, or like your suggestion in
https://patchwork.ozlabs.org/patch/667680/.
I'm gonna fix the new problem you pointed out, by reading-once
'tcfm_eaction' early (right when tcfm_dev is dereferenced) knowing this
is just "keeping things as is wrt running lockless", without introducing
any new non-coherent code.
Thanks,
Shmulik
^ permalink raw reply [flat|nested] 9+ messages in thread