Re: [PATCH net] act_ife: Add support for machines with hard_header_len != mac_len

[Jamal Hadi Salim]

On 16-09-21 08:54 AM, Yotam Gigi wrote:

Without that fix, the following could occur:
 - On encode ingress, the total amount of skb_pushes (in lines 751 and
   753) was more than specified in cow.
 - On machines with hard_header_len > mac_len, the packet format was not

Just curious: What hardware would this be?

Fixes: ef6980b6becb ("net sched: introduce IFE action")
Signed-off-by: Yotam Gigi <yot...@mellanox.com>
---
 net/sched/act_ife.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index e87cd81..27b19ca 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -708,11 +708,13 @@ static int tcf_ife_encode(struct sk_buff *skb, const 
struct tc_action *a,
           where ORIGDATA = original ethernet header ...
         */
        u16 metalen = ife_get_sz(skb, ife);
-       int hdrm = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
-       unsigned int skboff = skb->dev->hard_header_len;
        u32 at = G_TC_AT(skb->tc_verd);
-       int new_len = skb->len + hdrm;
        bool exceed_mtu = false;
+       unsigned int skboff;
+       int total_push;
+       int reserve;
+       int new_len;
+       int hdrm;
        int err;

        if (at & AT_EGRESS) {
@@ -724,6 +726,22 @@ static int tcf_ife_encode(struct sk_buff *skb, const 
struct tc_action *a,
        bstats_update(&ife->tcf_bstats, skb);
        tcf_lastuse_update(&ife->tcf_tm);

+       if (at & AT_EGRESS) {
+               /* on egress, reserve space for hard_header_len instead of
+                * mac_len
+                */
+               skb_reset_mac_len(skb);

The skb_reset_mac_len() above is unneeded.

+               hdrm = metalen + skb->mac_len + IFE_METAHDRLEN;

Can you move this line outside of the if? It appears on the else
so factoring it out is useful.

+               total_push = hdrm;
+               reserve = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
+       } else {
+               /* on ingress, push mac_len as it already get parsed from tc */
+               hdrm = metalen + skb->mac_len + IFE_METAHDRLEN;
+               total_push = hdrm + skb->mac_len;
+               reserve = total_push;
+       }
+       new_len =  skb->len + hdrm;
+
        if (!metalen) {         /* no metadata to send */
                /* abuse overlimits to count when we allow packet
                 * with no metadata
@@ -742,19 +760,17 @@ static int tcf_ife_encode(struct sk_buff *skb, const 
struct tc_action *a,

        iethh = eth_hdr(skb);

-       err = skb_cow_head(skb, hdrm);
+       err = skb_cow_head(skb, reserve);
        if (unlikely(err)) {
                ife->tcf_qstats.drops++;
                spin_unlock(&ife->tcf_lock);
                return TC_ACT_SHOT;
        }

-       if (!(at & AT_EGRESS))
-               skb_push(skb, skb->dev->hard_header_len);
-
-       __skb_push(skb, hdrm);
+       __skb_push(skb, total_push);
        memcpy(skb->data, iethh, skb->mac_len);
        skb_reset_mac_header(skb);
+       skboff += skb->mac_len;

Above looks dangerous. Did the compiler not warn?
Maybe init skboff to skb->mac_len at the top.

Otherwise the ingress bits look good. Thanks!

Please fix above and resend with:
Signed-off-by: Jamal Hadi Salim <j...@mojatatu.com>

cheers,
jamal