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

On Thu, 2016-10-06 at 10:30 -0700, Cong Wang wrote:

On Thu, Oct 6, 2016 at 6:30 AM, Shmulik Ladkani
[off-list ref] wrote:

quoted

Hi,

On Mon, Oct 3, 2016 at 12:45 PM, Cong Wang [off-list ref] wrote:

quoted

On Thu, Sep 29, 2016 at 4:03 AM, Shmulik Ladkani
[off-list ref] wrote:

quoted

        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.