[ovs-dev] [PATCHv3 3/5] datapath: Add SCTP support

[Joe Stringer]

On Wed, Jul 3, 2013 at 7:50 AM, Jesse Gross <jesse at nicira.com> wrote:
> On Tue, Jun 11, 2013 at 10:35 PM, Joe Stringer <joe at wand.net.nz> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 0dac658..d4fdd65 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static int set_sctp(struct sk_buff *skb,
>> +                    const struct ovs_key_sctp *sctp_port_key)
>> +{
>> +       struct sctphdr *sh;
>> +       int err;
>> +
>> +       err = make_writable(skb, skb_transport_offset(skb) +
>> +                                sizeof(struct sctphdr));
>> +       if (unlikely(err))
>> +               return err;
>> +
>> +       sh = sctp_hdr(skb);
>> +       if (sctp_port_key->sctp_src != sh->source ||
>> +           sctp_port_key->sctp_dst != sh->dest) {
>> +               __le32 old_correct_csum, new_csum, old_csum;
>> +
>> +               old_csum = sh->checksum;
>> +               old_correct_csum = compute_sctp_csum(skb);
>> +
>> +               sh->source = sctp_port_key->sctp_src;
>> +               sh->dest = sctp_port_key->sctp_dst;
>> +
>> +                new_csum = compute_sctp_csum(skb);
>> +
>> +               /* Carry any checksum errors through. */
>> +               sh->checksum = old_csum ^ old_correct_csum ^ new_csum;
>> +
>> +               skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> I don't think that it's a good idea to set CHECKSUM_UNNCESSARY here:
> if this packet came in with a bad checksum and it goes to the local
> stack then it will be accepted even if we set an intentionally bad
> checksum value.

OK, I'll remove it.

>> diff --git a/datapath/checksum.c b/datapath/checksum.c
>> index 5146c65..bfa75a7 100644
>> --- a/datapath/checksum.c
>> +++ b/datapath/checksum.c
>> @@ -59,6 +59,9 @@ static int vswitch_skb_checksum_setup(struct sk_buff *skb)
>>         case IPPROTO_UDP:
>>                 csum_offset = offsetof(struct udphdr, check);
>>                 break;
>> +       case IPPROTO_SCTP:
>> +               csum_offset = offsetof(struct sctphdr, check);
>> +               break;
>
> This is compatibility code from an old version of Xen. We should never
> get SCTP packets requiring checksum offloading there, so it's probably
> better to just leave this case out and let it hit the warning since
> something has to be wrong.

OK.

>> diff --git a/datapath/checksum.h b/datapath/checksum.h
>> index a440c59..a97d47b 100644
>> --- a/datapath/checksum.h
>> +++ b/datapath/checksum.h
>> +static inline __le32 compute_sctp_csum(const struct sk_buff *skb)
>> +{
>> +       const struct sk_buff *iter;
>> +       __u32 crc;
>> +       __u16 tp_len = skb_headlen(skb) - skb_transport_offset(skb);
>> +
>> +       crc = sctp_start_cksum((__u8 *)sctp_hdr(skb), tp_len);
>> +       skb_walk_frags(skb, iter)
>> +               crc = sctp_update_cksum((u8 *) iter->data, skb_headlen(iter),
>> +                                       crc);
>> +
>> +       return sctp_end_cksum(crc);
>> +}
>
> This file is mostly compatibility code for older kernels, which this
> function isn't really. I see that this appears several places in the
> upstream kernel, so maybe we can refactor and put it in the
> appropriate place in the compat directory?

OK, that can be done. I'll send a separate patch for the refactor and
move this code into compat/.

> Do we need backports for the sctp_* functions as well?

It seems that include/net/sctp/checksum.h header was introduced with
2.6.25. Prior to that, the functions were in include/net/sctp/sctp.h.
I'll fix this up with the compat adjustments above.

>> diff --git a/datapath/linux/compat/include/linux/sctp.h b/datapath/linux/compat/include/linux/sctp.h
>> new file mode 100644
>> index 0000000..e6b9174
>> --- /dev/null
>> +++ b/datapath/linux/compat/include/linux/sctp.h
>> +#ifndef NEXTHDR_SCTP
>> +#define NEXTHDR_SCTP    132 /* Stream Control Transport Protocol */
>> +#endif
>
> I believe this doesn't exist yet in the upstream kernel but if it did
> then it would live in include/net/ipv6.h, right? If so, then can we
> put the backport there?

OK.

> Would you mind including the changes to include/linux/openvswitch.h in
> this patch instead of the userspace one so that when I send it
> upstream I can just use the original commit instead of piecing a few
> together?

OK.