| Message ID | 1475147012-15538-5-git-send-email-shmulik.ladkani@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
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.
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.
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;
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.
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
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...
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().
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
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();
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(-)