* [PATCH v3 net-next 1/4] net/sched: act_mirred: Rename tcfm_ok_push to tcfm_mac_header_xmit and make it a bool
2016-09-29 11:03 [PATCH v3 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
@ 2016-09-29 11:03 ` Shmulik Ladkani
2016-09-29 11:03 ` [PATCH v3 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; 14+ messages in thread
From: Shmulik Ladkani @ 2016-09-29 11:03 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
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] 14+ messages in thread* [PATCH v3 net-next 2/4] net/sched: act_mirred: Refactor detection whether dev needs xmit at mac header
2016-09-29 11:03 [PATCH v3 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
2016-09-29 11:03 ` [PATCH v3 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-29 11:03 ` Shmulik Ladkani
2016-09-29 11:03 ` [PATCH v3 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror' Shmulik Ladkani
2016-09-29 11:03 ` [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
3 siblings, 0 replies; 14+ messages in thread
From: Shmulik Ladkani @ 2016-09-29 11:03 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
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] 14+ messages in thread* [PATCH v3 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror'
2016-09-29 11:03 [PATCH v3 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
2016-09-29 11:03 ` [PATCH v3 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-29 11:03 ` [PATCH v3 net-next 2/4] net/sched: act_mirred: Refactor detection whether dev needs xmit at mac header Shmulik Ladkani
@ 2016-09-29 11:03 ` Shmulik Ladkani
2016-09-29 11:03 ` [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
3 siblings, 0 replies; 14+ messages in thread
From: Shmulik Ladkani @ 2016-09-29 11:03 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
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] 14+ messages in thread* [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-29 11:03 [PATCH v3 net-next 0/4] act_mirred: Ingress actions support Shmulik Ladkani
` (2 preceding siblings ...)
2016-09-29 11:03 ` [PATCH v3 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror' Shmulik Ladkani
@ 2016-09-29 11:03 ` Shmulik Ladkani
2016-10-03 19:45 ` Cong Wang
2016-10-04 1:45 ` David Miller
3 siblings, 2 replies; 14+ messages in thread
From: Shmulik Ladkani @ 2016-09-29 11:03 UTC (permalink / raw)
To: David Miller
Cc: Jamal Hadi Salim, WANG Cong, Eric Dumazet, Daniel Borkmann,
netdev, Shmulik Ladkani, Eric Dumazet
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>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
v3: Addressed non coherency due to reading m->tcfm_eaction multiple times,
as spotted by Eric Dumazet
net/sched/act_mirred.c | 51 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 69dcce8c75..22dcfd68e6 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)
@@ -156,15 +177,20 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
struct tcf_mirred *m = to_mirred(a);
+ bool m_mac_header_xmit;
struct net_device *dev;
struct sk_buff *skb2;
- int retval, err;
+ int retval, err = 0;
+ int m_eaction;
+ int mac_len;
u32 at;
tcf_lastuse_update(&m->tcf_tm);
bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
rcu_read_lock();
+ m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
+ m_eaction = READ_ONCE(m->tcfm_eaction);
retval = READ_ONCE(m->tcf_action);
dev = rcu_dereference(m->tcfm_dev);
if (unlikely(!dev)) {
@@ -183,23 +209,36 @@ 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_eaction) && m_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_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_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_eaction))
retval = TC_ACT_SHOT;
}
rcu_read_unlock();
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-29 11:03 ` [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
@ 2016-10-03 19:45 ` Cong Wang
2016-10-06 13:30 ` Shmulik Ladkani
2016-10-04 1:45 ` David Miller
1 sibling, 1 reply; 14+ messages in thread
From: Cong Wang @ 2016-10-03 19:45 UTC (permalink / raw)
To: Shmulik Ladkani
Cc: David Miller, Jamal Hadi Salim, Eric Dumazet, Daniel Borkmann,
Linux Kernel Network Developers, Eric Dumazet
On Thu, Sep 29, 2016 at 4:03 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> skb2->skb_iif = skb->dev->ifindex;
> skb2->dev = dev;
> - err = dev_queue_xmit(skb2);
> + if (tcf_mirred_act_direction(m_eaction) & AT_EGRESS)
> + err = dev_queue_xmit(skb2);
> + else
> + netif_receive_skb(skb2);
Any reason why not check the return value here?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-03 19:45 ` Cong Wang
@ 2016-10-06 13:30 ` Shmulik Ladkani
2016-10-06 17:30 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Shmulik Ladkani @ 2016-10-06 13:30 UTC (permalink / raw)
To: Cong Wang, David Miller
Cc: Jamal Hadi Salim, Eric Dumazet, Daniel Borkmann,
Linux Kernel Network Developers, Eric Dumazet
Hi,
On Mon, Oct 3, 2016 at 12:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Sep 29, 2016 at 4:03 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
>> skb2->skb_iif = skb->dev->ifindex;
>> skb2->dev = dev;
>> - err = dev_queue_xmit(skb2);
>> + if (tcf_mirred_act_direction(m_eaction) & AT_EGRESS)
>> + err = dev_queue_xmit(skb2);
>> + else
>> + netif_receive_skb(skb2);
>
> Any reason why not check the return value here?
Rationale: netif_receive_skb returns err if there was no protocol
handler to deliver the skb to.
If skb is not caught by any protocol handler, this should not be
considered an "ingress redirect" error. The redirect action should be
considered successful.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-06 13:30 ` Shmulik Ladkani
@ 2016-10-06 17:30 ` Cong Wang
2016-10-06 19:38 ` Eric Dumazet
2016-10-07 0:17 ` Jamal Hadi Salim
0 siblings, 2 replies; 14+ messages in thread
From: Cong Wang @ 2016-10-06 17:30 UTC (permalink / raw)
To: Shmulik Ladkani
Cc: David Miller, Jamal Hadi Salim, Eric Dumazet, Daniel Borkmann,
Linux Kernel Network Developers, Eric Dumazet
On Thu, Oct 6, 2016 at 6:30 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> Hi,
>
> On Mon, Oct 3, 2016 at 12:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Sep 29, 2016 at 4:03 AM, Shmulik Ladkani
>> <shmulik.ladkani@gmail.com> wrote:
>>> skb2->skb_iif = skb->dev->ifindex;
>>> skb2->dev = dev;
>>> - err = dev_queue_xmit(skb2);
>>> + if (tcf_mirred_act_direction(m_eaction) & AT_EGRESS)
>>> + err = dev_queue_xmit(skb2);
>>> + else
>>> + netif_receive_skb(skb2);
>>
>> Any reason why not check the return value here?
>
> Rationale: netif_receive_skb returns err if there was no protocol
> handler to deliver the skb to.
> If skb is not caught by any protocol handler, this should not be
> considered an "ingress redirect" error. The redirect action should be
> considered successful.
A quick grep shows there are many places returning NET_RX_DROP:
E.g.
net/ipv4/arp.c: return NET_RX_DROP;
net/ipv4/arp.c: return NET_RX_DROP;
net/ipv4/gre_demux.c: return NET_RX_DROP;
net/ipv4/ip_forward.c: return NET_RX_DROP;
net/ipv4/ip_input.c: return NET_RX_DROP;
net/ipv4/ip_input.c: return NET_RX_DROP;
net/ipv4/ipconfig.c: return NET_RX_DROP;
net/ipv4/ipconfig.c: return NET_RX_DROP;
net/ipv4/raw.c: return NET_RX_DROP;
net/ipv4/raw.c: return NET_RX_DROP;
net/ipv4/xfrm4_input.c: return NET_RX_DROP;
net/ipv6/ip6_input.c: return NET_RX_DROP;
net/ipv6/ip6_input.c: return NET_RX_DROP;
net/ipv6/ip6_input.c: return NET_RX_DROP;
net/ipv6/raw.c: return NET_RX_DROP;
net/ipv6/raw.c: return NET_RX_DROP;
net/ipv6/raw.c: return NET_RX_DROP;
net/ipv6/raw.c: return NET_RX_DROP;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-06 17:30 ` Cong Wang
@ 2016-10-06 19:38 ` Eric Dumazet
2016-10-07 0:44 ` Cong Wang
2016-10-07 0:17 ` Jamal Hadi Salim
1 sibling, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2016-10-06 19:38 UTC (permalink / raw)
To: Cong Wang
Cc: Shmulik Ladkani, David Miller, Jamal Hadi Salim, Eric Dumazet,
Daniel Borkmann, Linux Kernel Network Developers
On Thu, 2016-10-06 at 10:30 -0700, Cong Wang wrote:
> On Thu, Oct 6, 2016 at 6:30 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > Hi,
> >
> > On Mon, Oct 3, 2016 at 12:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> On Thu, Sep 29, 2016 at 4:03 AM, Shmulik Ladkani
> >> <shmulik.ladkani@gmail.com> wrote:
> >>> skb2->skb_iif = skb->dev->ifindex;
> >>> skb2->dev = dev;
> >>> - err = dev_queue_xmit(skb2);
> >>> + if (tcf_mirred_act_direction(m_eaction) & AT_EGRESS)
> >>> + err = dev_queue_xmit(skb2);
> >>> + else
> >>> + netif_receive_skb(skb2);
> >>
> >> Any reason why not check the return value here?
> >
> > Rationale: netif_receive_skb returns err if there was no protocol
> > handler to deliver the skb to.
> > If skb is not caught by any protocol handler, this should not be
> > considered an "ingress redirect" error. The redirect action should be
> > considered successful.
>
> A quick grep shows there are many places returning NET_RX_DROP:
> E.g.
And another quick grep shows that out of 142 drivers, only one [1] of
them (incorrectly) checks netif_receive_skb() return value.
Real question is more like : what is the impact of propagating an error
at this point ?
[1] drivers/net/caif/caif_virtio.c
This is incorrect because at the driver layer, the packet was received
and the rx_packets/rx_bytes counters _should_ be incremented regardless
of packet being dropped or not by upper layers.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-06 19:38 ` Eric Dumazet
@ 2016-10-07 0:44 ` Cong Wang
0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2016-10-07 0:44 UTC (permalink / raw)
To: Eric Dumazet
Cc: Shmulik Ladkani, David Miller, Jamal Hadi Salim, Eric Dumazet,
Daniel Borkmann, Linux Kernel Network Developers
On Thu, Oct 6, 2016 at 12:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> And another quick grep shows that out of 142 drivers, only one [1] of
> them (incorrectly) checks netif_receive_skb() return value.
>
act_mirred is not a driver, apparently.
> Real question is more like : what is the impact of propagating an error
> at this point ?
_If_ we are going to just propagate the error like egress, then
the difference is m->tcf_action (PIPE or STOLEN) vs TC_ACT_SHOT.
And this error code is propagated from tcf_action_exec() up to
qdisc layer...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-06 17:30 ` Cong Wang
2016-10-06 19:38 ` Eric Dumazet
@ 2016-10-07 0:17 ` Jamal Hadi Salim
2016-10-07 0:49 ` Cong Wang
1 sibling, 1 reply; 14+ messages in thread
From: Jamal Hadi Salim @ 2016-10-07 0:17 UTC (permalink / raw)
To: Cong Wang, Shmulik Ladkani
Cc: David Miller, Eric Dumazet, Daniel Borkmann,
Linux Kernel Network Developers, Eric Dumazet
On 16-10-06 01:30 PM, Cong Wang wrote:
> On Thu, Oct 6, 2016 at 6:30 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
>> Hi,
>>
>> On Mon, Oct 3, 2016 at 12:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Thu, Sep 29, 2016 at 4:03 AM, Shmulik Ladkani
>>> <shmulik.ladkani@gmail.com> wrote:
>>>> skb2->skb_iif = skb->dev->ifindex;
>>>> skb2->dev = dev;
>>>> - err = dev_queue_xmit(skb2);
>>>> + if (tcf_mirred_act_direction(m_eaction) & AT_EGRESS)
>>>> + err = dev_queue_xmit(skb2);
>>>> + else
>>>> + netif_receive_skb(skb2);
>>>
>>> Any reason why not check the return value here?
>>
>> Rationale: netif_receive_skb returns err if there was no protocol
>> handler to deliver the skb to.
>> If skb is not caught by any protocol handler, this should not be
>> considered an "ingress redirect" error. The redirect action should be
>> considered successful.
>
I dont believe we need to bother with the return code in this case.
The core netif_receive_skb() code already increments any necessary
stats.
cheers,
jamal
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-07 0:17 ` Jamal Hadi Salim
@ 2016-10-07 0:49 ` Cong Wang
2016-10-08 7:01 ` Jamal Hadi Salim
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2016-10-07 0:49 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Shmulik Ladkani, David Miller, Eric Dumazet, Daniel Borkmann,
Linux Kernel Network Developers, Eric Dumazet
On Thu, Oct 6, 2016 at 5:17 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> I dont believe we need to bother with the return code in this case.
Why?
For a quick example, STOLEN vs. SHOT:
result = tc_classify(skb, filter, &res, false);
if (result >= 0) {
#ifdef CONFIG_NET_CLS_ACT
switch (result) {
case TC_ACT_STOLEN:
case TC_ACT_QUEUED:
*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
case TC_ACT_SHOT:
return 0;
}
#endif
Note, *qerr is the return value to ->enqueue().
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-10-07 0:49 ` Cong Wang
@ 2016-10-08 7:01 ` Jamal Hadi Salim
0 siblings, 0 replies; 14+ messages in thread
From: Jamal Hadi Salim @ 2016-10-08 7:01 UTC (permalink / raw)
To: Cong Wang
Cc: Shmulik Ladkani, David Miller, Eric Dumazet, Daniel Borkmann,
Linux Kernel Network Developers, Eric Dumazet
On 16-10-06 08:49 PM, Cong Wang wrote:
> On Thu, Oct 6, 2016 at 5:17 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> I dont believe we need to bother with the return code in this case.
>
> Why?
>
> For a quick example, STOLEN vs. SHOT:
>
> result = tc_classify(skb, filter, &res, false);
> if (result >= 0) {
> #ifdef CONFIG_NET_CLS_ACT
> switch (result) {
> case TC_ACT_STOLEN:
> case TC_ACT_QUEUED:
> *qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
> case TC_ACT_SHOT:
> return 0;
> }
> #endif
>
> Note, *qerr is the return value to ->enqueue().
>
You are right. I take back what i said.
We at minimal need consistency; so whether going to ingress or egress
we should at least increment the overlimit stats in case of non-success
code. Shmulik please fix up with checks on return code.
cheers,
jamal
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions
2016-09-29 11:03 ` [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions Shmulik Ladkani
2016-10-03 19:45 ` Cong Wang
@ 2016-10-04 1:45 ` David Miller
1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2016-10-04 1:45 UTC (permalink / raw)
To: shmulik.ladkani
Cc: jhs, xiyou.wangcong, edumazet, daniel, netdev, eric.dumazet
From: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Date: Thu, 29 Sep 2016 14:03:32 +0300
> skb2->skb_iif = skb->dev->ifindex;
> skb2->dev = dev;
> - err = dev_queue_xmit(skb2);
> + if (tcf_mirred_act_direction(m_eaction) & AT_EGRESS)
> + err = dev_queue_xmit(skb2);
> + else
> + netif_receive_skb(skb2);
Can you address the feedback about lack of checking the return
value of netif_receive_skb()? It seems like a legitimate issue,
thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread