netdev - Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to handle packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANvJab3_ZMiMtCns2TtVxjM8GjVtqtk=+WYzsoj3t2b2958AVg@mail.gmail.com>
Date:	Thu, 21 Feb 2013 23:19:05 -0800
From:	pravin <pravin.shelar@...il.com>
To:	Dmitry Kravkov <dmitry@...adcom.com>
Cc:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to
 handle packets

On Thu, Feb 21, 2013 at 3:46 PM, pravin <pravin.shelar@...il.com> wrote:
> On Thu, Feb 21, 2013 at 2:26 PM, Dmitry Kravkov <dmitry@...adcom.com> wrote:
>> On Tue, 2013-02-19 at 15:03 -0800, pravin wrote:
>>> On Tue, Feb 19, 2013 at 11:20 AM, Dmitry Kravkov <dmitry@...adcom.com> wrote:
>>> >
>>> >> -----Original Message-----
>>> >> From: pravin [mailto:pravin.shelar@...il.com]
>>> >> Sent: Tuesday, February 19, 2013 8:28 PM
>>> >> To: Dmitry Kravkov
>>> >> Cc: davem@...emloft.net; netdev@...r.kernel.org
>>> >> Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to
>>> >> handle packets
>>> >>
>>> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@...adcom.com>
>>> >> wrote:
>>> >> > If device is not able to handle checksumming it will
>>> >> > be handled in dev_xmit
>>> >> >
>>> >> > Signed-off-by: Dmitry Kravkov <dmitry@...adcom.com>
>>> >> > ---
>>> >> > Changes from v1: fixed email address
>>> >> >
>>> >> >  net/ipv4/ip_gre.c |    7 ++-----
>>> >> >  1 files changed, 2 insertions(+), 5 deletions(-)
>>> >> >
>>> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>>> >> > index a56f118..cdc31ac 100644
>>> >> > --- a/net/ipv4/ip_gre.c
>>> >> > +++ b/net/ipv4/ip_gre.c
>>> >> > @@ -745,12 +745,9 @@ static struct sk_buff *handle_offloads(struct sk_buff
>>> >> *skb)
>>> >> >                         goto error;
>>> >> >                 skb_shinfo(skb)->gso_type |= SKB_GSO_GRE;
>>> >> >                 return skb;
>>> >> > -       } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>> >> > -               err = skb_checksum_help(skb);
>>> >> > -               if (unlikely(err))
>>> >> > -                       goto error;
>>> >> >         }
>>> >> > -       skb->ip_summed = CHECKSUM_NONE;
>>> >> > +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>>> >> > +               skb->ip_summed = CHECKSUM_NONE;
>>> >> >
>>> >> >         return skb;
>>> >> >
>>> >> > --
>>> >> > 1.7.7.2
>>> >> >
>>> >> >
>>> >>
>>> >> This patch breaks GRE tunnel with GRE_CSUM. since GRE_CSUM need
>>> >> complete IP packet to checksum entire GRE payload.
>>> >
>>> > Testing for o_flags&GRE_CSUM does not look too hurt here, since it will be used in ipgre_tunnel_xmit() later on
>>> > This is the only problematic case, right?
>>> >
>>>
>>> It does not work for me. I have GRE device with csum on. Ping works
>>> fine but Netperf is not working.
>>> Looking at code, I am not sure how tcp will work if inner packet TCP
>>> checksum is calculated after GRE_CSUM calculation.
>>
>>
>> Fixes handling when CSUM or SEQ flags are set - via skb_checksum_help()
>> ---
>> Tested with each mode (separately)
>>
>>  net/ipv4/ip_gre.c |   14 +++++++++++---
>>  1 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> index 5ef4da7..0de54cd 100644
>> --- a/net/ipv4/ip_gre.c
>> +++ b/net/ipv4/ip_gre.c
>> @@ -735,7 +735,8 @@ drop:
>>         return 0;
>>  }
>>
>> -static struct sk_buff *handle_offloads(struct sk_buff *skb)
>> +static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel,
>> +                                      struct sk_buff *skb)
>>  {
>>         int err;
>>
>> @@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct
>> sk_buff *skb)
>>                         goto error;
>>                 skb_shinfo(skb)->gso_type |= SKB_GSO_GRE;
>>                 return skb;
>> +       } else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) &&
>> +                  skb->ip_summed == CHECKSUM_PARTIAL) {
>> +               err = skb_checksum_help(skb);
>> +               if (unlikely(err))
>> +                       goto error;
>>         }

one more thing, there is no need to do csum for GRE_SEQ case.

>> -       if (skb->ip_summed != CHECKSUM_PARTIAL)
>> +
>> +       if (skb->ip_summed != CHECKSUM_PARTIAL ||
>> +           tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM))
>>                 skb->ip_summed = CHECKSUM_NONE;
>>
> We can simplify this by dropping check of o_flags.
>
>>         return skb;
>> @@ -776,7 +784,7 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff
>> *skb, struct net_device *dev
>>         int    err;
>>         int    pkt_len;
>>
>> -       skb = handle_offloads(skb);
>> +       skb = handle_offloads(tunnel, skb);
>>         if (IS_ERR(skb)) {
>>                 dev->stats.tx_dropped++;
>>                 return NETDEV_TX_OK;
>> --
>> 1.7.7.2
>>
>>
>>> Thanks,
>>> Pravin.
>>>
>>
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html