>-----Original Message-----
>From: Jamal Hadi Salim [mailto:j...@mojatatu.com]
>Sent: Friday, September 23, 2016 1:40 AM
>To: Yotam Gigi <yot...@mellanox.com>; da...@davemloft.net;
>netdev@vger.kernel.org; Roman Mashak <m...@mojatatu.com>
>Subject: Re: [PATCH net] act_ife: Add support for machines with hard_header_len
>!= mac_len
>
>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?
On mlxsw, in order to send a packet there needed to be added small tx header.
In
order to tell the kernel to reserve that space in the allocated skb, we set
that
hard_header_len field to include the tx header in addition to the mac header.
>
>
>> 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.
That's look weird. I will fix it and repost in the next couple of days.
>
>Otherwise the ingress bits look good. Thanks!
>
>Please fix above and resend with:
>Signed-off-by: Jamal Hadi Salim <j...@mojatatu.com>
>
>cheers,
>jamal
