| Message ID | 20200104001645.7038-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-upcall: Fix using uninitialized upcall hash. | expand |
On Sat, Jan 4, 2020 at 8:16 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > upcalls are allocated on stack and 'hash' field must be initialized > regardless of attribute existence because it will be used later. > > Conditional jump or move depends on uninitialised value(s) > at 0xFA74A7: dpif_netlink_encode_execute (dpif-netlink.c:1828) > by 0xFA6DE8: dpif_netlink_operate__ (dpif-netlink.c:1906) > by 0xFA612F: dpif_netlink_operate_chunks (dpif-netlink.c:2219) > by 0xFA0E36: dpif_netlink_operate (dpif-netlink.c:2275) > by 0xE5AFAC: dpif_operate (dpif.c:1376) > by 0xDF3922: handle_upcalls (ofproto-dpif-upcall.c:1615) > by 0xDF269B: recv_upcalls (ofproto-dpif-upcall.c:857) > by 0xDF1C49: udpif_upcall_handler (ofproto-dpif-upcall.c:759) > by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383) > by 0x565F6DA: start_thread (pthread_create.c:463) > by 0x615988E: clone (clone.S:95) > Uninitialised value was created by a stack allocation > at 0xDF2258: recv_upcalls (ofproto-dpif-upcall.c:773) > > Fixes: 0442bfb11d6c ("ofproto-dpif-upcall: Echo HASH attribute back to datapath.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ofproto/ofproto-dpif-upcall.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index aac0554af..409286ab1 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -786,6 +786,7 @@ recv_upcalls(struct handler *handler) > struct upcall *upcall = &upcalls[n_upcalls]; > struct flow *flow = &flows[n_upcalls]; > unsigned int mru = 0; > + uint64_t hash = 0; > int error; > > ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls], > @@ -806,7 +807,7 @@ recv_upcalls(struct handler *handler) > } > > if (dupcall->hash) { > - upcall->hash = nl_attr_get_u64(dupcall->hash); > + hash = nl_attr_get_u64(dupcall->hash); > } > > error = upcall_receive(upcall, udpif->backer, &dupcall->packet, > @@ -830,6 +831,7 @@ recv_upcalls(struct handler *handler) > upcall->key = dupcall->key; > upcall->key_len = dupcall->key_len; > upcall->ufid = &dupcall->ufid; > + upcall->hash = hash; > > upcall->out_tun_key = dupcall->out_tun_key; > upcall->actions = dupcall->actions; > -- > 2.17.1 > Acked-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
On Sat, Jan 04, 2020 at 11:16:27AM +0800, Tonghao Zhang wrote: > On Sat, Jan 4, 2020 at 8:16 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > > > upcalls are allocated on stack and 'hash' field must be initialized > > regardless of attribute existence because it will be used later. > > > > Conditional jump or move depends on uninitialised value(s) > > at 0xFA74A7: dpif_netlink_encode_execute (dpif-netlink.c:1828) > > by 0xFA6DE8: dpif_netlink_operate__ (dpif-netlink.c:1906) > > by 0xFA612F: dpif_netlink_operate_chunks (dpif-netlink.c:2219) > > by 0xFA0E36: dpif_netlink_operate (dpif-netlink.c:2275) > > by 0xE5AFAC: dpif_operate (dpif.c:1376) > > by 0xDF3922: handle_upcalls (ofproto-dpif-upcall.c:1615) > > by 0xDF269B: recv_upcalls (ofproto-dpif-upcall.c:857) > > by 0xDF1C49: udpif_upcall_handler (ofproto-dpif-upcall.c:759) > > by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383) > > by 0x565F6DA: start_thread (pthread_create.c:463) > > by 0x615988E: clone (clone.S:95) > > Uninitialised value was created by a stack allocation > > at 0xDF2258: recv_upcalls (ofproto-dpif-upcall.c:773) > > > > Fixes: 0442bfb11d6c ("ofproto-dpif-upcall: Echo HASH attribute back to datapath.") > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > --- > > ofproto/ofproto-dpif-upcall.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > > index aac0554af..409286ab1 100644 > > --- a/ofproto/ofproto-dpif-upcall.c > > +++ b/ofproto/ofproto-dpif-upcall.c > > @@ -786,6 +786,7 @@ recv_upcalls(struct handler *handler) > > struct upcall *upcall = &upcalls[n_upcalls]; > > struct flow *flow = &flows[n_upcalls]; > > unsigned int mru = 0; > > + uint64_t hash = 0; > > int error; > > > > ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls], > > @@ -806,7 +807,7 @@ recv_upcalls(struct handler *handler) > > } > > > > if (dupcall->hash) { > > - upcall->hash = nl_attr_get_u64(dupcall->hash); > > + hash = nl_attr_get_u64(dupcall->hash); > > } > > > > error = upcall_receive(upcall, udpif->backer, &dupcall->packet, > > @@ -830,6 +831,7 @@ recv_upcalls(struct handler *handler) > > upcall->key = dupcall->key; > > upcall->key_len = dupcall->key_len; > > upcall->ufid = &dupcall->ufid; > > + upcall->hash = hash; > > > > upcall->out_tun_key = dupcall->out_tun_key; > > upcall->actions = dupcall->actions; > > -- > > 2.17.1 > > > Acked-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> Looks good to me. Acked-by: William Tu <u9012063@gmail.com> I assume you're running valgrind and saw this. I'm curious which test cases triggers this issue? Thanks William
On 06.01.2020 21:27, William Tu wrote: > On Sat, Jan 04, 2020 at 11:16:27AM +0800, Tonghao Zhang wrote: >> On Sat, Jan 4, 2020 at 8:16 AM Ilya Maximets <i.maximets@ovn.org> wrote: >>> >>> upcalls are allocated on stack and 'hash' field must be initialized >>> regardless of attribute existence because it will be used later. >>> >>> Conditional jump or move depends on uninitialised value(s) >>> at 0xFA74A7: dpif_netlink_encode_execute (dpif-netlink.c:1828) >>> by 0xFA6DE8: dpif_netlink_operate__ (dpif-netlink.c:1906) >>> by 0xFA612F: dpif_netlink_operate_chunks (dpif-netlink.c:2219) >>> by 0xFA0E36: dpif_netlink_operate (dpif-netlink.c:2275) >>> by 0xE5AFAC: dpif_operate (dpif.c:1376) >>> by 0xDF3922: handle_upcalls (ofproto-dpif-upcall.c:1615) >>> by 0xDF269B: recv_upcalls (ofproto-dpif-upcall.c:857) >>> by 0xDF1C49: udpif_upcall_handler (ofproto-dpif-upcall.c:759) >>> by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383) >>> by 0x565F6DA: start_thread (pthread_create.c:463) >>> by 0x615988E: clone (clone.S:95) >>> Uninitialised value was created by a stack allocation >>> at 0xDF2258: recv_upcalls (ofproto-dpif-upcall.c:773) >>> >>> Fixes: 0442bfb11d6c ("ofproto-dpif-upcall: Echo HASH attribute back to datapath.") >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> ofproto/ofproto-dpif-upcall.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >>> index aac0554af..409286ab1 100644 >>> --- a/ofproto/ofproto-dpif-upcall.c >>> +++ b/ofproto/ofproto-dpif-upcall.c >>> @@ -786,6 +786,7 @@ recv_upcalls(struct handler *handler) >>> struct upcall *upcall = &upcalls[n_upcalls]; >>> struct flow *flow = &flows[n_upcalls]; >>> unsigned int mru = 0; >>> + uint64_t hash = 0; >>> int error; >>> >>> ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls], >>> @@ -806,7 +807,7 @@ recv_upcalls(struct handler *handler) >>> } >>> >>> if (dupcall->hash) { >>> - upcall->hash = nl_attr_get_u64(dupcall->hash); >>> + hash = nl_attr_get_u64(dupcall->hash); >>> } >>> >>> error = upcall_receive(upcall, udpif->backer, &dupcall->packet, >>> @@ -830,6 +831,7 @@ recv_upcalls(struct handler *handler) >>> upcall->key = dupcall->key; >>> upcall->key_len = dupcall->key_len; >>> upcall->ufid = &dupcall->ufid; >>> + upcall->hash = hash; >>> >>> upcall->out_tun_key = dupcall->out_tun_key; >>> upcall->actions = dupcall->actions; >>> -- >>> 2.17.1 >>> >> Acked-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Looks good to me. > Acked-by: William Tu <u9012063@gmail.com> > > I assume you're running valgrind and saw this. > I'm curious which test cases triggers this issue? I tried to fix broken check-offloads and used valgrind to find issues. Here is the patch that I used: https://patchwork.ozlabs.org/patch/1218096/ Should be reproducible with usual kmod system testsuite too. Best regards, Ilya Maximets.
On 06.01.2020 21:27, William Tu wrote: > On Sat, Jan 04, 2020 at 11:16:27AM +0800, Tonghao Zhang wrote: >> On Sat, Jan 4, 2020 at 8:16 AM Ilya Maximets <i.maximets@ovn.org> wrote: >>> >>> upcalls are allocated on stack and 'hash' field must be initialized >>> regardless of attribute existence because it will be used later. >>> >>> Conditional jump or move depends on uninitialised value(s) >>> at 0xFA74A7: dpif_netlink_encode_execute (dpif-netlink.c:1828) >>> by 0xFA6DE8: dpif_netlink_operate__ (dpif-netlink.c:1906) >>> by 0xFA612F: dpif_netlink_operate_chunks (dpif-netlink.c:2219) >>> by 0xFA0E36: dpif_netlink_operate (dpif-netlink.c:2275) >>> by 0xE5AFAC: dpif_operate (dpif.c:1376) >>> by 0xDF3922: handle_upcalls (ofproto-dpif-upcall.c:1615) >>> by 0xDF269B: recv_upcalls (ofproto-dpif-upcall.c:857) >>> by 0xDF1C49: udpif_upcall_handler (ofproto-dpif-upcall.c:759) >>> by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383) >>> by 0x565F6DA: start_thread (pthread_create.c:463) >>> by 0x615988E: clone (clone.S:95) >>> Uninitialised value was created by a stack allocation >>> at 0xDF2258: recv_upcalls (ofproto-dpif-upcall.c:773) >>> >>> Fixes: 0442bfb11d6c ("ofproto-dpif-upcall: Echo HASH attribute back to datapath.") >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> ofproto/ofproto-dpif-upcall.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >>> index aac0554af..409286ab1 100644 >>> --- a/ofproto/ofproto-dpif-upcall.c >>> +++ b/ofproto/ofproto-dpif-upcall.c >>> @@ -786,6 +786,7 @@ recv_upcalls(struct handler *handler) >>> struct upcall *upcall = &upcalls[n_upcalls]; >>> struct flow *flow = &flows[n_upcalls]; >>> unsigned int mru = 0; >>> + uint64_t hash = 0; >>> int error; >>> >>> ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls], >>> @@ -806,7 +807,7 @@ recv_upcalls(struct handler *handler) >>> } >>> >>> if (dupcall->hash) { >>> - upcall->hash = nl_attr_get_u64(dupcall->hash); >>> + hash = nl_attr_get_u64(dupcall->hash); >>> } >>> >>> error = upcall_receive(upcall, udpif->backer, &dupcall->packet, >>> @@ -830,6 +831,7 @@ recv_upcalls(struct handler *handler) >>> upcall->key = dupcall->key; >>> upcall->key_len = dupcall->key_len; >>> upcall->ufid = &dupcall->ufid; >>> + upcall->hash = hash; >>> >>> upcall->out_tun_key = dupcall->out_tun_key; >>> upcall->actions = dupcall->actions; >>> -- >>> 2.17.1 >>> >> Acked-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Looks good to me. > Acked-by: William Tu <u9012063@gmail.com> Thanks! Applied to master. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index aac0554af..409286ab1 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -786,6 +786,7 @@ recv_upcalls(struct handler *handler) struct upcall *upcall = &upcalls[n_upcalls]; struct flow *flow = &flows[n_upcalls]; unsigned int mru = 0; + uint64_t hash = 0; int error; ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls], @@ -806,7 +807,7 @@ recv_upcalls(struct handler *handler) } if (dupcall->hash) { - upcall->hash = nl_attr_get_u64(dupcall->hash); + hash = nl_attr_get_u64(dupcall->hash); } error = upcall_receive(upcall, udpif->backer, &dupcall->packet, @@ -830,6 +831,7 @@ recv_upcalls(struct handler *handler) upcall->key = dupcall->key; upcall->key_len = dupcall->key_len; upcall->ufid = &dupcall->ufid; + upcall->hash = hash; upcall->out_tun_key = dupcall->out_tun_key; upcall->actions = dupcall->actions;
upcalls are allocated on stack and 'hash' field must be initialized regardless of attribute existence because it will be used later. Conditional jump or move depends on uninitialised value(s) at 0xFA74A7: dpif_netlink_encode_execute (dpif-netlink.c:1828) by 0xFA6DE8: dpif_netlink_operate__ (dpif-netlink.c:1906) by 0xFA612F: dpif_netlink_operate_chunks (dpif-netlink.c:2219) by 0xFA0E36: dpif_netlink_operate (dpif-netlink.c:2275) by 0xE5AFAC: dpif_operate (dpif.c:1376) by 0xDF3922: handle_upcalls (ofproto-dpif-upcall.c:1615) by 0xDF269B: recv_upcalls (ofproto-dpif-upcall.c:857) by 0xDF1C49: udpif_upcall_handler (ofproto-dpif-upcall.c:759) by 0xF3A3FE: ovsthread_wrapper (ovs-thread.c:383) by 0x565F6DA: start_thread (pthread_create.c:463) by 0x615988E: clone (clone.S:95) Uninitialised value was created by a stack allocation at 0xDF2258: recv_upcalls (ofproto-dpif-upcall.c:773) Fixes: 0442bfb11d6c ("ofproto-dpif-upcall: Echo HASH attribute back to datapath.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ofproto/ofproto-dpif-upcall.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)