On Wed, Jul 3, 2013 at 7:50 AM, Jesse Gross <je...@nicira.com> wrote:
> On Tue, Jun 11, 2013 at 10:35 PM, Joe Stringer <j...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
