On Tue, Mar 31, 2015 at 05:26:19PM -0700, Jesse Gross wrote:
> On Tue, Mar 31, 2015 at 4:21 PM, Ben Pfaff <b...@nicira.com> wrote:
> > On Mon, Mar 30, 2015 at 03:14:46PM -0700, Jesse Gross wrote:
> >> The GRE checksum is a 16 bit field stored in a 32 bit option (the
> >> rest is reserved). The current code treats the checksum as a 32-bit
> >> field and places it in the right place for little endian systems but
> >> not big endian. This fixes the problem by storing the 16 bit field
> >> directly.
> >>
> >> Signed-off-by: Jesse Gross <je...@nicira.com>
> >> ---
> >> lib/netdev-vport.c | 6 ++----
> >> lib/odp-util.c | 8 ++++----
> >> 2 files changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> >> index 1ee68bc..a9639d3 100644
> >> --- a/lib/netdev-vport.c
> >> +++ b/lib/netdev-vport.c
> >> @@ -996,10 +996,8 @@ netdev_gre_push_header__(struct dp_packet *packet,
> >> greh = push_ip_header(packet, header, size, &ip_tot_size);
> >>
> >> if (greh->flags & htons(GRE_CSUM)) {
> >> - ovs_16aligned_be32 *options = (ovs_16aligned_be32 *) (greh + 1);
> >> -
> >> - put_16aligned_be32(options,
> >> - (OVS_FORCE ovs_be32) csum(greh, ip_tot_size -
> >> sizeof (struct ip_header)));
> >> + ovs_be16 *csum_opt = (ovs_be16 *) (greh + 1);
> >> + *csum_opt = csum(greh, ip_tot_size - sizeof (struct ip_header));
> >
> > Does the above zero the other 16 bits?
>
> This should be OK because the entire header was memset to zero when it
> was built at flow creation time.
>
> >> diff --git a/lib/odp-util.c b/lib/odp-util.c
> >> index 827b91c..5990edc 100644
> >> --- a/lib/odp-util.c
> >> +++ b/lib/odp-util.c
> >> @@ -565,7 +565,7 @@ format_odp_tnl_push_header(struct ds *ds, struct
> >> ovs_action_push_tnl *data)
> >> greh->flags, ntohs(greh->protocol));
> >> options = (ovs_16aligned_be32 *)(greh + 1);
> >> if (greh->flags & htons(GRE_CSUM)) {
> >> - ds_put_format(ds, ",csum=0x%"PRIx32,
> >> ntohl(get_16aligned_be32(options)));
> >> + ds_put_format(ds, ",csum=0x%"PRIx16, ntohs(*((ovs_be16
> >> *)options)));
> >> options++;
> >> }
> >> if (greh->flags & htons(GRE_KEY)) {
> >> @@ -913,12 +913,12 @@ ovs_parse_tnl_push(const char *s, struct
> >> ovs_action_push_tnl *data)
> >> ovs_16aligned_be32 *options = (ovs_16aligned_be32 *) (greh + 1);
> >>
> >> if (greh->flags & htons(GRE_CSUM)) {
> >> - uint32_t csum;
> >> + uint16_t csum;
> >>
> >> - if (!ovs_scan_len(s, &n, ",csum=0x%"SCNx32, &csum)) {
> >> + if (!ovs_scan_len(s, &n, ",csum=0x%"SCNx16, &csum)) {
> >> return -EINVAL;
> >> }
> >> - put_16aligned_be32(options, htonl(csum));
> >> + *((ovs_be16 *)options) = htons(csum);
> >> options++;
> >
> > What about here?
>
> This one isn't zeroed so I so added a memset before the checksum is set:
>
> + memset(options, 0, sizeof *options);
Thanks!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
