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.
> 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.
> 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?
Do we need backports for the sctp_* functions as well?
> 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?
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?
X-CudaMail-Whitelist-To: dev@openvswitch.org
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
