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

[Shmulik Ladkani]

Hi Eric,

On Tue, 27 Sep 2016 14:27:13 -0700 Eric Dumazet  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

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

[Eric Dumazet]

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 ;)

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

[Shmulik Ladkani]

Hi,

On Tue, 27 Sep 2016 14:27:13 -0700 Eric Dumazet  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?

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

[Eric Dumazet]

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.

[PATCH v2 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 
Cc: Jamal Hadi Salim 
---
 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(>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