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

2016-09-23 Thread Yotam Gigi 

>-Original Message-
>From: David Miller [mailto:da...@davemloft.net]
>Sent: Friday, September 23, 2016 2:06 PM
>To: Yotam Gigi 
>Cc: j...@mojatatu.com; netdev@vger.kernel.org
>Subject: Re: [PATCH net] act_ife: Add support for machines with hard_header_len
>!= mac_len
>
>From: Yotam Gigi 
>Date: Wed, 21 Sep 2016 15:54:13 +0300
>
>> Without that fix, the following could occur:
> ...
>
>I don't think what you are doing in mlxsw is valid.
>
>You can't set hard_header_len arbitrarily, it's the MAC length.
>
>If you need to prepend special headers or whatever, set
>->needed_headroom which is designed for this purpose.

Ok, we will fix that.

Thanks for the comment!

>
>Thanks.

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

2016-09-23 Thread David Miller 
From: Yotam Gigi 
Date: Wed, 21 Sep 2016 15:54:13 +0300

> Without that fix, the following could occur:
 ...

I don't think what you are doing in mlxsw is valid.

You can't set hard_header_len arbitrarily, it's the MAC length.

If you need to prepend special headers or whatever, set
->needed_headroom which is designed for this purpose.

Thanks.

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

2016-09-22 Thread Yotam Gigi 
>-Original Message-
>From: Jamal Hadi Salim [mailto:j...@mojatatu.com]
>Sent: Friday, September 23, 2016 1:40 AM
>To: Yotam Gigi ; da...@davemloft.net;
>netdev@vger.kernel.org; Roman Mashak 
>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 
>> ---
>>  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 
>
>cheers,
>jamal

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

2016-09-22 Thread 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 
---
 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 

cheers,
jamal