Date:   Sun, 25 Sep 2016 15:55:49 +0000
From:   Yotam Gigi <yotamg@...lanox.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Yotam Gigi <yotam.gi@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Jiri Pirko <jiri@...lanox.com>, Elad Raz <eladr@...lanox.com>,
        mlxsw <mlxsw@...lanox.com>
Subject: RE: [PATCH net v2 0/2] Fix tc-ife bugs

>-----Original Message-----
>From: Jamal Hadi Salim [mailto:jhs@...atatu.com]
>Sent: Sunday, September 25, 2016 4:46 PM
>To: Yotam Gigi <yotam.gi@...il.com>; davem@...emloft.net;
>netdev@...r.kernel.org; Yotam Gigi <yotamg@...lanox.com>
>Subject: Re: [PATCH net v2 0/2] Fix tc-ife bugs
>
>On 16-09-25 08:31 AM, Yotam Gigi wrote:
>> This patch-set contains two bugfixes in the tc-ife action, one fixing some
>> random behaviour in encode side, and one fixing the decode side packet
>> parsing logic.
>>
>> Yotam Gigi (2):
>>   act_ife: Fix external mac header on encode
>>   act_ife: Fix false parsing on decode side
>>
>>  net/sched/act_ife.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>
>Yotam, did you run the test i proposed? I am worried about the TLV one.
>Note:
>Encoder includes the length of the TLV header length in the L part.
>If you are reaching a conclusion that you need this in the decoding:
>+              tlvdata += alen + sizeof(struct meta_tlvhdr);
>
>then very likely whoever is sending that packet is not encoding
>correctly.

The one encoding the packets I get is the ife action. You can look at the code:

  int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen, const void *dval)
  {
        u32 *tlv = (u32 *)(skbdata);
        u16 totlen = nla_total_size(dlen);      /*alignment + hdr */
        char *dptr = (char *)tlv + NLA_HDRLEN;
        u32 htlv = attrtype << 16 | dlen;

        *tlv = htonl(htlv);
        memset(dptr, 0, totlen - NLA_HDRLEN);
        memcpy(dptr, dval, dlen);
        
        return totlen;
  }

As you can see, the data that is actually written into the packet is the htlv
var, which has the 'dlen' in it, and not the totlen which corresponds the tlv
header size. You can see that in the following example:

I ran the tc command you proposed:
  $TC filter add dev $ETH parent 1: protocol ip prio 10 \
        u32 match ip protocol 1 0xff flowid 1:2 \
        action skbedit prio 33 \
        action ife encode \
        type 0xDEAD \
        use mark 12 \
        allow prio \
        dst 02:15:15:15:15:15

And this is the packet I got:
  0x0000:  0215 1515 1515 da23 d209 8644 dead 0012
  0x0010:  0001 0004 0000 000c 0003 0004 0000 0033
  0x0020:  fa30 7418 593a da23 d209 8644 0800 4500
  0x0030:  0054 da3c 4000 4001 486a 0c00 0001 0c00
  0x0040:  0002 0800 9fa2 1562 0008 aeec e757 0000
  0x0050:  0000 ecdb 0100 0000 0000 1011 1213 1415
  0x0060:  1617 1819 1a1b 1c1d 1e1f 2021 2223 2425
  0x0070:  2627 2829 2a2b 2c2d 2e2f 3031 3233 3435
  0x0080:  3637

You can see that there are two tlvs there, one for mark (with value 0xc=12) and
one for prio (with value 0x33). In the packets, you can see the on offsets 0x12
and 0x1a, that the size in the tlv is 4 which is the datalen and not 8 which is
the total tlv size.

When I ran the decode without the fix, my kernel went into infinite loop which
was caused by:
 - The loop stopping condition was checking that an unsigned variable is greater
   than zero.
 - The parsing assumes that in the tlv header, the size refers to the whole tlv
   size, but it refers to the size of the data only.

To fix those problems, I fixed the decode side to assume that the tlv->size
refers to the data len and not the whole tlv, and changed the variable to be
signed.

Do you prefer that I will fix the encode side to encode the whole tlv header
size instead of fixing the decode side?

Thanks :)

>
>cheers,
>jamal

Powered by blists - more mailing lists