| Message ID | 20170527042413.26448-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
Acked-By: Miguel Angel Ajo <majopela@redhat.com> $ cat test.c #include <stdio.h> #include <stdint.h> void main(void) { printf("uint16_t<<24=%Lx\n", ((uint16_t)0xf123)<<24); printf("uint64_t<<24=%Lx\n", ((uint64_t)0xf123)<<24); } linux: vagrant@gw1 in ~/ovs on $ ./test uint16_t<<24=23000000 uint64_t<<24=f123000000 I tested it, and it seems (that at least for my gcc version 4.8.5 20150623 (Red Hat 4.8.5-11) (GCC) on x64), a 16 bit shifted, will result in 32bit output. a 64bit shifted will result in 64bit output. That could explain why this could have been working and not seen with <256 ports. On Sat, May 27, 2017 at 6:24 AM, Ben Pfaff <blp@ovn.org> wrote: > put_encapsulation() is meant to load the logical output port into bits > 24 to 40 of the tunnel ID metadata field, but 'outport << 24' did not > have that effect because outport has type uint16_t. This fixes the > problem. > > Found by Coverity. > > Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/ > fileInstanceId=14763078&defectInstanceId=4304791&mergedDefectId=180391 > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ovn/controller/physical.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index 457fc45414bd..532c7252e1ea 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -128,8 +128,8 @@ put_encapsulation(enum mf_field_id mff_ovn_geneve, > put_load(outport, mff_ovn_geneve, 0, 32, ofpacts); > put_move(MFF_LOG_INPORT, 0, mff_ovn_geneve, 16, 15, ofpacts); > } else if (tun->type == STT) { > - put_load(datapath->tunnel_key | (outport << 24), MFF_TUN_ID, 0, > 64, > - ofpacts); > + put_load(datapath->tunnel_key | ((uint64_t) outport << 24), > + MFF_TUN_ID, 0, 64, ofpacts); > put_move(MFF_LOG_INPORT, 0, MFF_TUN_ID, 40, 15, ofpacts); > } else if (tun->type == VXLAN) { > put_load(datapath->tunnel_key, MFF_TUN_ID, 0, 24, ofpacts); > -- > 2.10.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks for the review! I applied this to master, branch-2.7, and branch-2.6. On Mon, May 29, 2017 at 12:47:07PM +0200, Miguel Angel Ajo Pelayo wrote: > Acked-By: Miguel Angel Ajo <majopela@redhat.com> > > $ cat test.c > #include <stdio.h> > #include <stdint.h> > > void main(void) { > > printf("uint16_t<<24=%Lx\n", ((uint16_t)0xf123)<<24); > printf("uint64_t<<24=%Lx\n", ((uint64_t)0xf123)<<24); > } > > linux: vagrant@gw1 in ~/ovs on > $ ./test > uint16_t<<24=23000000 > uint64_t<<24=f123000000 > > I tested it, and it seems (that at least for my gcc version 4.8.5 20150623 > (Red Hat 4.8.5-11) (GCC) on x64), > > a 16 bit shifted, will result in 32bit output. > a 64bit shifted will result in 64bit output. > > > That could explain why this could have been working and not seen with <256 > ports. > > > On Sat, May 27, 2017 at 6:24 AM, Ben Pfaff <blp@ovn.org> wrote: > > > put_encapsulation() is meant to load the logical output port into bits > > 24 to 40 of the tunnel ID metadata field, but 'outport << 24' did not > > have that effect because outport has type uint16_t. This fixes the > > problem. > > > > Found by Coverity. > > > > Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/ > > fileInstanceId=14763078&defectInstanceId=4304791&mergedDefectId=180391 > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > ovn/controller/physical.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > index 457fc45414bd..532c7252e1ea 100644 > > --- a/ovn/controller/physical.c > > +++ b/ovn/controller/physical.c > > @@ -128,8 +128,8 @@ put_encapsulation(enum mf_field_id mff_ovn_geneve, > > put_load(outport, mff_ovn_geneve, 0, 32, ofpacts); > > put_move(MFF_LOG_INPORT, 0, mff_ovn_geneve, 16, 15, ofpacts); > > } else if (tun->type == STT) { > > - put_load(datapath->tunnel_key | (outport << 24), MFF_TUN_ID, 0, > > 64, > > - ofpacts); > > + put_load(datapath->tunnel_key | ((uint64_t) outport << 24), > > + MFF_TUN_ID, 0, 64, ofpacts); > > put_move(MFF_LOG_INPORT, 0, MFF_TUN_ID, 40, 15, ofpacts); > > } else if (tun->type == VXLAN) { > > put_load(datapath->tunnel_key, MFF_TUN_ID, 0, 24, ofpacts); > > -- > > 2.10.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 457fc45414bd..532c7252e1ea 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -128,8 +128,8 @@ put_encapsulation(enum mf_field_id mff_ovn_geneve, put_load(outport, mff_ovn_geneve, 0, 32, ofpacts); put_move(MFF_LOG_INPORT, 0, mff_ovn_geneve, 16, 15, ofpacts); } else if (tun->type == STT) { - put_load(datapath->tunnel_key | (outport << 24), MFF_TUN_ID, 0, 64, - ofpacts); + put_load(datapath->tunnel_key | ((uint64_t) outport << 24), + MFF_TUN_ID, 0, 64, ofpacts); put_move(MFF_LOG_INPORT, 0, MFF_TUN_ID, 40, 15, ofpacts); } else if (tun->type == VXLAN) { put_load(datapath->tunnel_key, MFF_TUN_ID, 0, 24, ofpacts);
put_encapsulation() is meant to load the logical output port into bits 24 to 40 of the tunnel ID metadata field, but 'outport << 24' did not have that effect because outport has type uint16_t. This fixes the problem. Found by Coverity. Reported-at: https://scan3.coverity.com/reports.htm#v16889/p10449/fileInstanceId=14763078&defectInstanceId=4304791&mergedDefectId=180391 Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/controller/physical.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)