[PATCH v3 net-next 0/4] act_mirred: Ingress actions support

[PATCH v3 net-next 0/4] act_mirred: Ingress actions support

[Shmulik Ladkani]

This patch series implements action mirred '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.

v3:
  in 4/4, addressed non coherency due to reading m->tcfm_eaction multiple
  times, as spotted by Eric Dumazet
v2:
  in 1/4, declare tcfm_mac_header_xmit as bool instead of int

Shmulik Ladkani (4):
  net/sched: act_mirred: Rename tcfm_ok_push to tcfm_mac_header_xmit and
    make it a bool
  net/sched: act_mirred: Refactor detection whether dev needs xmit at
    mac header
  net/sched: tc_mirred: Rename public predicates
    'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror'
  net/sched: act_mirred: Implement ingress actions

 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 +-
 .../net/ethernet/netronome/nfp/nfp_net_offload.c   |  2 +-
 include/net/tc_act/tc_mirred.h                     |  6 +-
 net/sched/act_mirred.c                             | 84 ++++++++++++++++------
 7 files changed, 73 insertions(+), 29 deletions(-)

-- 
2.7.4

[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]

'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

[PATCH v3 net-next 2/4] net/sched: act_mirred: Refactor detection whether dev needs xmit at mac header

[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

[PATCH v3 net-next 3/4] net/sched: tc_mirred: Rename public predicates 'is_tcf_mirred_redirect' and 'is_tcf_mirred_mirror'

[Shmulik Ladkani]

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

[PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[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>
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

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Cong Wang]

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?

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[David Miller]

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.

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Shmulik Ladkani]

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.

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Cong Wang]

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;

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Eric Dumazet]

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.

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Jamal Hadi Salim]

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

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Cong Wang]

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...

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Cong Wang]

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().

Re: [PATCH v3 net-next 4/4] net/sched: act_mirred: Implement ingress actions

[Jamal Hadi Salim]

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