| Message ID | 20211221110322.14345.33610.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix UndefinedBehaviorSanitizer reported issues and enable it in CI. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Dumitru Ceara <dceara@redhat.com> writes: > UB Sanitizer report: > lib/dp-packet.h:297:39: runtime error: applying zero offset to null pointer > #0 0x7946f5 in dp_packet_tail /root/ovs/./lib/dp-packet.h:297:39 > #1 0x794331 in dp_packet_tailroom /root/ovs/./lib/dp-packet.h:325:49 > #2 0x7942a0 in dp_packet_prealloc_tailroom /root/ovs/lib/dp-packet.c:297:16 > #3 0xc347cf in eth_compose /root/ovs/lib/packets.c:1061:5 > [...] > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > lib/netdev-dummy.c | 6 +++--- > ofproto/bond.c | 3 ++- > ofproto/ofproto-dpif-trace.c | 4 ++-- > ofproto/ofproto-dpif-xlate.c | 4 ++-- > ofproto/ofproto-dpif.c | 6 +++--- > utilities/ovs-ofctl.c | 6 ++---- > 6 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 1f386b81bbaf..2bb6c38bd5ca 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -188,7 +188,7 @@ dummy_packet_stream_init(struct dummy_packet_stream *s, struct stream *stream) > { > int rxbuf_size = stream ? 2048 : 0; > s->stream = stream; > - dp_packet_init(&s->rxbuf, rxbuf_size); > + dp_packet_init(&s->rxbuf, MAX(rxbuf_size, ETH_HEADER_LEN)); > ovs_list_init(&s->txq); > } > > @@ -1149,7 +1149,7 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED, > if (flow.dl_type == htons(ETH_TYPE_ARP) > && flow.nw_proto == ARP_OP_REQUEST > && flow.nw_dst == dev->address.s_addr) { > - struct dp_packet *reply = dp_packet_new(0); > + struct dp_packet *reply = dp_packet_new(ETH_HEADER_LEN); > compose_arp(reply, ARP_OP_REPLY, dev->hwaddr, flow.dl_src, > false, flow.nw_dst, flow.nw_src); > netdev_dummy_queue_packet(dev, reply, NULL, 0); > @@ -1647,7 +1647,7 @@ eth_from_flow_str(const char *s, size_t packet_size, > return NULL; > } > > - packet = dp_packet_new(0); > + packet = dp_packet_new(MAX(packet_size, ETH_HEADER_LEN)); > if (packet_size) { > flow_compose(packet, flow, NULL, 0); > if (dp_packet_size(packet) < packet_size) { > diff --git a/ofproto/bond.c b/ofproto/bond.c > index cdfdf0b9d8c0..8ab27ed09c3a 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -809,7 +809,8 @@ bond_compose_learning_packet(struct bond *bond, const struct eth_addr eth_src, > flow.dl_src = eth_src; > member = choose_output_member(bond, &flow, NULL, vlan); > > - packet = dp_packet_new(0); > + packet = dp_packet_new(2 + ETH_HEADER_LEN + VLAN_HEADER_LEN > + + ARP_ETH_HEADER_LEN); > compose_rarp(packet, eth_src); > if (vlan) { > eth_push_vlan(packet, htons(ETH_TYPE_VLAN), htons(vlan)); > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c > index 78a54c715dc7..b4a9974689d1 100644 > --- a/ofproto/ofproto-dpif-trace.c > +++ b/ofproto/ofproto-dpif-trace.c > @@ -245,7 +245,7 @@ parse_flow_and_packet(int argc, const char *argv[], > > struct dp_packet payload; > memset(&payload, 0, sizeof payload); > - dp_packet_init(&payload, 0); > + dp_packet_init(&payload, ETH_HEADER_LEN); > if (dp_packet_put_hex(&payload, argv[++i], NULL)[0] != '\0') { > dp_packet_uninit(&payload); > error = xstrdup("Trailing garbage in packet data"); > @@ -436,7 +436,7 @@ parse_flow_and_packet(int argc, const char *argv[], > > if (generate_packet) { > /* Generate a packet, as requested. */ > - packet = dp_packet_new(0); > + packet = dp_packet_new(ETH_HEADER_LEN); > flow_compose(packet, flow, l7, l7_len); > } else if (packet) { > /* Use the metadata from the flow and the packet argument to > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 991e22e6b64b..c1fd4c585d56 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3490,7 +3490,7 @@ tnl_send_nd_request(struct xlate_ctx *ctx, const struct xport *out_dev, > { > struct dp_packet packet; > > - dp_packet_init(&packet, 0); > + dp_packet_init(&packet, ETH_HEADER_LEN); > compose_nd_ns(&packet, eth_src, ipv6_src, ipv6_dst); > compose_table_xlate(ctx, out_dev, &packet); > dp_packet_uninit(&packet); > @@ -3503,7 +3503,7 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev, > { > struct dp_packet packet; > > - dp_packet_init(&packet, 0); > + dp_packet_init(&packet, ETH_HEADER_LEN); > compose_arp(&packet, ARP_OP_REQUEST, > eth_src, eth_addr_zero, true, ip_src, ip_dst); > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index bc3df8ea1510..e44e365dd405 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -1268,7 +1268,7 @@ check_ct_eventmask(struct dpif_backer *backer) > nl_msg_end_nested(&actions, ct_start); > > /* Compose a dummy UDP packet. */ > - dp_packet_init(&packet, 0); > + dp_packet_init(&packet, ETH_HEADER_LEN); > flow_compose(&packet, &flow, NULL, 64); > > /* Execute the actions. On older datapaths this fails with EINVAL, on > @@ -1361,7 +1361,7 @@ check_ct_timeout_policy(struct dpif_backer *backer) > nl_msg_end_nested(&actions, ct_start); > > /* Compose a dummy UDP packet. */ > - dp_packet_init(&packet, 0); > + dp_packet_init(&packet, ETH_HEADER_LEN); > flow_compose(&packet, &flow, NULL, 64); > > /* Execute the actions. On older datapaths this fails with EINVAL, on > @@ -3501,7 +3501,7 @@ send_pdu_cb(void *port_, const void *pdu, size_t pdu_size) > struct dp_packet packet; > void *packet_pdu; > > - dp_packet_init(&packet, 0); > + dp_packet_init(&packet, ETH_HEADER_LEN); > packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP, > pdu_size); > memcpy(packet_pdu, pdu, pdu_size); > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > index ede7f1e61ab8..55b5e1a4c95a 100644 > --- a/utilities/ovs-ofctl.c > +++ b/utilities/ovs-ofctl.c > @@ -4902,15 +4902,13 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx) > } > > struct dp_packet p; > - memset(&p, 0, sizeof p); > - dp_packet_init(&p, 0); > + dp_packet_init(&p, ETH_HEADER_LEN); > > void *l7 = NULL; > size_t l7_len = 64; > if (ctx->argc > 2) { > struct dp_packet payload; > - memset(&payload, 0, sizeof payload); > - dp_packet_init(&payload, 0); > + dp_packet_init(&payload, ETH_HEADER_LEN); > if (dp_packet_put_hex(&payload, ctx->argv[2], NULL)[0] != '\0') { > ovs_fatal(0, "%s: trailing garbage in packet data", ctx->argv[2]); > } If the issue is just in dp_packet_prealloc_tailroom() can we just do a size check there? - if (size > dp_packet_tailroom(b)) { + /* check against pkt_size to start, because it is more likely to fail */ + if (dp_packet_size(b) < size || size > dp_packet_tailroom(b)) { Otherwise, I think there will be multiple reallocations with all of these changes. Maybe I missed a different ubsan flake - otherwise, we can also assert that size must be non-zero when allocating a dp_packet, but that's a bigger change.
On 12/21/21 16:44, Aaron Conole wrote: > Dumitru Ceara <dceara@redhat.com> writes: > >> UB Sanitizer report: >> lib/dp-packet.h:297:39: runtime error: applying zero offset to null pointer >> #0 0x7946f5 in dp_packet_tail /root/ovs/./lib/dp-packet.h:297:39 >> #1 0x794331 in dp_packet_tailroom /root/ovs/./lib/dp-packet.h:325:49 >> #2 0x7942a0 in dp_packet_prealloc_tailroom /root/ovs/lib/dp-packet.c:297:16 >> #3 0xc347cf in eth_compose /root/ovs/lib/packets.c:1061:5 >> [...] >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> lib/netdev-dummy.c | 6 +++--- >> ofproto/bond.c | 3 ++- >> ofproto/ofproto-dpif-trace.c | 4 ++-- >> ofproto/ofproto-dpif-xlate.c | 4 ++-- >> ofproto/ofproto-dpif.c | 6 +++--- >> utilities/ovs-ofctl.c | 6 ++---- >> 6 files changed, 14 insertions(+), 15 deletions(-) >> >> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c >> index 1f386b81bbaf..2bb6c38bd5ca 100644 >> --- a/lib/netdev-dummy.c >> +++ b/lib/netdev-dummy.c >> @@ -188,7 +188,7 @@ dummy_packet_stream_init(struct dummy_packet_stream *s, struct stream *stream) >> { >> int rxbuf_size = stream ? 2048 : 0; >> s->stream = stream; >> - dp_packet_init(&s->rxbuf, rxbuf_size); >> + dp_packet_init(&s->rxbuf, MAX(rxbuf_size, ETH_HEADER_LEN)); >> ovs_list_init(&s->txq); >> } >> >> @@ -1149,7 +1149,7 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED, >> if (flow.dl_type == htons(ETH_TYPE_ARP) >> && flow.nw_proto == ARP_OP_REQUEST >> && flow.nw_dst == dev->address.s_addr) { >> - struct dp_packet *reply = dp_packet_new(0); >> + struct dp_packet *reply = dp_packet_new(ETH_HEADER_LEN); >> compose_arp(reply, ARP_OP_REPLY, dev->hwaddr, flow.dl_src, >> false, flow.nw_dst, flow.nw_src); >> netdev_dummy_queue_packet(dev, reply, NULL, 0); >> @@ -1647,7 +1647,7 @@ eth_from_flow_str(const char *s, size_t packet_size, >> return NULL; >> } >> >> - packet = dp_packet_new(0); >> + packet = dp_packet_new(MAX(packet_size, ETH_HEADER_LEN)); >> if (packet_size) { >> flow_compose(packet, flow, NULL, 0); >> if (dp_packet_size(packet) < packet_size) { >> diff --git a/ofproto/bond.c b/ofproto/bond.c >> index cdfdf0b9d8c0..8ab27ed09c3a 100644 >> --- a/ofproto/bond.c >> +++ b/ofproto/bond.c >> @@ -809,7 +809,8 @@ bond_compose_learning_packet(struct bond *bond, const struct eth_addr eth_src, >> flow.dl_src = eth_src; >> member = choose_output_member(bond, &flow, NULL, vlan); >> >> - packet = dp_packet_new(0); >> + packet = dp_packet_new(2 + ETH_HEADER_LEN + VLAN_HEADER_LEN >> + + ARP_ETH_HEADER_LEN); >> compose_rarp(packet, eth_src); >> if (vlan) { >> eth_push_vlan(packet, htons(ETH_TYPE_VLAN), htons(vlan)); >> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c >> index 78a54c715dc7..b4a9974689d1 100644 >> --- a/ofproto/ofproto-dpif-trace.c >> +++ b/ofproto/ofproto-dpif-trace.c >> @@ -245,7 +245,7 @@ parse_flow_and_packet(int argc, const char *argv[], >> >> struct dp_packet payload; >> memset(&payload, 0, sizeof payload); >> - dp_packet_init(&payload, 0); >> + dp_packet_init(&payload, ETH_HEADER_LEN); >> if (dp_packet_put_hex(&payload, argv[++i], NULL)[0] != '\0') { >> dp_packet_uninit(&payload); >> error = xstrdup("Trailing garbage in packet data"); >> @@ -436,7 +436,7 @@ parse_flow_and_packet(int argc, const char *argv[], >> >> if (generate_packet) { >> /* Generate a packet, as requested. */ >> - packet = dp_packet_new(0); >> + packet = dp_packet_new(ETH_HEADER_LEN); >> flow_compose(packet, flow, l7, l7_len); >> } else if (packet) { >> /* Use the metadata from the flow and the packet argument to >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 991e22e6b64b..c1fd4c585d56 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -3490,7 +3490,7 @@ tnl_send_nd_request(struct xlate_ctx *ctx, const struct xport *out_dev, >> { >> struct dp_packet packet; >> >> - dp_packet_init(&packet, 0); >> + dp_packet_init(&packet, ETH_HEADER_LEN); >> compose_nd_ns(&packet, eth_src, ipv6_src, ipv6_dst); >> compose_table_xlate(ctx, out_dev, &packet); >> dp_packet_uninit(&packet); >> @@ -3503,7 +3503,7 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev, >> { >> struct dp_packet packet; >> >> - dp_packet_init(&packet, 0); >> + dp_packet_init(&packet, ETH_HEADER_LEN); >> compose_arp(&packet, ARP_OP_REQUEST, >> eth_src, eth_addr_zero, true, ip_src, ip_dst); >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index bc3df8ea1510..e44e365dd405 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -1268,7 +1268,7 @@ check_ct_eventmask(struct dpif_backer *backer) >> nl_msg_end_nested(&actions, ct_start); >> >> /* Compose a dummy UDP packet. */ >> - dp_packet_init(&packet, 0); >> + dp_packet_init(&packet, ETH_HEADER_LEN); >> flow_compose(&packet, &flow, NULL, 64); >> >> /* Execute the actions. On older datapaths this fails with EINVAL, on >> @@ -1361,7 +1361,7 @@ check_ct_timeout_policy(struct dpif_backer *backer) >> nl_msg_end_nested(&actions, ct_start); >> >> /* Compose a dummy UDP packet. */ >> - dp_packet_init(&packet, 0); >> + dp_packet_init(&packet, ETH_HEADER_LEN); >> flow_compose(&packet, &flow, NULL, 64); >> >> /* Execute the actions. On older datapaths this fails with EINVAL, on >> @@ -3501,7 +3501,7 @@ send_pdu_cb(void *port_, const void *pdu, size_t pdu_size) >> struct dp_packet packet; >> void *packet_pdu; >> >> - dp_packet_init(&packet, 0); >> + dp_packet_init(&packet, ETH_HEADER_LEN); >> packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP, >> pdu_size); >> memcpy(packet_pdu, pdu, pdu_size); >> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c >> index ede7f1e61ab8..55b5e1a4c95a 100644 >> --- a/utilities/ovs-ofctl.c >> +++ b/utilities/ovs-ofctl.c >> @@ -4902,15 +4902,13 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx) >> } >> >> struct dp_packet p; >> - memset(&p, 0, sizeof p); >> - dp_packet_init(&p, 0); >> + dp_packet_init(&p, ETH_HEADER_LEN); >> >> void *l7 = NULL; >> size_t l7_len = 64; >> if (ctx->argc > 2) { >> struct dp_packet payload; >> - memset(&payload, 0, sizeof payload); >> - dp_packet_init(&payload, 0); >> + dp_packet_init(&payload, ETH_HEADER_LEN); >> if (dp_packet_put_hex(&payload, ctx->argv[2], NULL)[0] != '\0') { >> ovs_fatal(0, "%s: trailing garbage in packet data", ctx->argv[2]); >> } > > If the issue is just in dp_packet_prealloc_tailroom() can we just do a > size check there? > > - if (size > dp_packet_tailroom(b)) { > + /* check against pkt_size to start, because it is more likely to fail */ > + if (dp_packet_size(b) < size || size > dp_packet_tailroom(b)) { This creates issues. I think due to the fact that the packet tailroom might shrink in some cases now. > > Otherwise, I think there will be multiple reallocations with all of > these changes. They're mainly in test/init code in which case it doesn't matter too much. The only exceptions are for bond learn, lacpdu and tunnel arp/nd request packet composition which shouldn't happen too often I guess. In any case, we can also do something similar to what you suggested, with a small change: diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 72f6d09ac7f3..8ae528ba6a6e 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -294,7 +294,7 @@ dp_packet_resize(struct dp_packet *b, size_t new_headroom, size_t new_tailroom) void dp_packet_prealloc_tailroom(struct dp_packet *b, size_t size) { - if (size > dp_packet_tailroom(b)) { + if ((!dp_packet_base(b) && size) || (size > dp_packet_tailroom(b))) { dp_packet_resize(b, dp_packet_headroom(b), MAX(size, 64)); } } This passes tests. What do you think? > > Maybe I missed a different ubsan flake - otherwise, we can also assert > that size must be non-zero when allocating a dp_packet, but that's a > bigger change. > Thanks, Dumitru
Dumitru Ceara <dceara@redhat.com> writes: > On 12/21/21 16:44, Aaron Conole wrote: >> Dumitru Ceara <dceara@redhat.com> writes: >> >>> UB Sanitizer report: >>> lib/dp-packet.h:297:39: runtime error: applying zero offset to null pointer >>> #0 0x7946f5 in dp_packet_tail /root/ovs/./lib/dp-packet.h:297:39 >>> #1 0x794331 in dp_packet_tailroom /root/ovs/./lib/dp-packet.h:325:49 >>> #2 0x7942a0 in dp_packet_prealloc_tailroom /root/ovs/lib/dp-packet.c:297:16 >>> #3 0xc347cf in eth_compose /root/ovs/lib/packets.c:1061:5 >>> [...] >>> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> lib/netdev-dummy.c | 6 +++--- >>> ofproto/bond.c | 3 ++- >>> ofproto/ofproto-dpif-trace.c | 4 ++-- >>> ofproto/ofproto-dpif-xlate.c | 4 ++-- >>> ofproto/ofproto-dpif.c | 6 +++--- >>> utilities/ovs-ofctl.c | 6 ++---- >>> 6 files changed, 14 insertions(+), 15 deletions(-) >>> >>> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c >>> index 1f386b81bbaf..2bb6c38bd5ca 100644 >>> --- a/lib/netdev-dummy.c >>> +++ b/lib/netdev-dummy.c >>> @@ -188,7 +188,7 @@ dummy_packet_stream_init(struct dummy_packet_stream *s, struct stream *stream) >>> { >>> int rxbuf_size = stream ? 2048 : 0; >>> s->stream = stream; >>> - dp_packet_init(&s->rxbuf, rxbuf_size); >>> + dp_packet_init(&s->rxbuf, MAX(rxbuf_size, ETH_HEADER_LEN)); >>> ovs_list_init(&s->txq); >>> } >>> >>> @@ -1149,7 +1149,7 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED, >>> if (flow.dl_type == htons(ETH_TYPE_ARP) >>> && flow.nw_proto == ARP_OP_REQUEST >>> && flow.nw_dst == dev->address.s_addr) { >>> - struct dp_packet *reply = dp_packet_new(0); >>> + struct dp_packet *reply = dp_packet_new(ETH_HEADER_LEN); >>> compose_arp(reply, ARP_OP_REPLY, dev->hwaddr, flow.dl_src, >>> false, flow.nw_dst, flow.nw_src); >>> netdev_dummy_queue_packet(dev, reply, NULL, 0); >>> @@ -1647,7 +1647,7 @@ eth_from_flow_str(const char *s, size_t packet_size, >>> return NULL; >>> } >>> >>> - packet = dp_packet_new(0); >>> + packet = dp_packet_new(MAX(packet_size, ETH_HEADER_LEN)); >>> if (packet_size) { >>> flow_compose(packet, flow, NULL, 0); >>> if (dp_packet_size(packet) < packet_size) { >>> diff --git a/ofproto/bond.c b/ofproto/bond.c >>> index cdfdf0b9d8c0..8ab27ed09c3a 100644 >>> --- a/ofproto/bond.c >>> +++ b/ofproto/bond.c >>> @@ -809,7 +809,8 @@ bond_compose_learning_packet(struct bond *bond, const struct eth_addr eth_src, >>> flow.dl_src = eth_src; >>> member = choose_output_member(bond, &flow, NULL, vlan); >>> >>> - packet = dp_packet_new(0); >>> + packet = dp_packet_new(2 + ETH_HEADER_LEN + VLAN_HEADER_LEN >>> + + ARP_ETH_HEADER_LEN); >>> compose_rarp(packet, eth_src); >>> if (vlan) { >>> eth_push_vlan(packet, htons(ETH_TYPE_VLAN), htons(vlan)); >>> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c >>> index 78a54c715dc7..b4a9974689d1 100644 >>> --- a/ofproto/ofproto-dpif-trace.c >>> +++ b/ofproto/ofproto-dpif-trace.c >>> @@ -245,7 +245,7 @@ parse_flow_and_packet(int argc, const char *argv[], >>> >>> struct dp_packet payload; >>> memset(&payload, 0, sizeof payload); >>> - dp_packet_init(&payload, 0); >>> + dp_packet_init(&payload, ETH_HEADER_LEN); >>> if (dp_packet_put_hex(&payload, argv[++i], NULL)[0] != '\0') { >>> dp_packet_uninit(&payload); >>> error = xstrdup("Trailing garbage in packet data"); >>> @@ -436,7 +436,7 @@ parse_flow_and_packet(int argc, const char *argv[], >>> >>> if (generate_packet) { >>> /* Generate a packet, as requested. */ >>> - packet = dp_packet_new(0); >>> + packet = dp_packet_new(ETH_HEADER_LEN); >>> flow_compose(packet, flow, l7, l7_len); >>> } else if (packet) { >>> /* Use the metadata from the flow and the packet argument to >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index 991e22e6b64b..c1fd4c585d56 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -3490,7 +3490,7 @@ tnl_send_nd_request(struct xlate_ctx *ctx, const struct xport *out_dev, >>> { >>> struct dp_packet packet; >>> >>> - dp_packet_init(&packet, 0); >>> + dp_packet_init(&packet, ETH_HEADER_LEN); >>> compose_nd_ns(&packet, eth_src, ipv6_src, ipv6_dst); >>> compose_table_xlate(ctx, out_dev, &packet); >>> dp_packet_uninit(&packet); >>> @@ -3503,7 +3503,7 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev, >>> { >>> struct dp_packet packet; >>> >>> - dp_packet_init(&packet, 0); >>> + dp_packet_init(&packet, ETH_HEADER_LEN); >>> compose_arp(&packet, ARP_OP_REQUEST, >>> eth_src, eth_addr_zero, true, ip_src, ip_dst); >>> >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index bc3df8ea1510..e44e365dd405 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -1268,7 +1268,7 @@ check_ct_eventmask(struct dpif_backer *backer) >>> nl_msg_end_nested(&actions, ct_start); >>> >>> /* Compose a dummy UDP packet. */ >>> - dp_packet_init(&packet, 0); >>> + dp_packet_init(&packet, ETH_HEADER_LEN); >>> flow_compose(&packet, &flow, NULL, 64); >>> >>> /* Execute the actions. On older datapaths this fails with EINVAL, on >>> @@ -1361,7 +1361,7 @@ check_ct_timeout_policy(struct dpif_backer *backer) >>> nl_msg_end_nested(&actions, ct_start); >>> >>> /* Compose a dummy UDP packet. */ >>> - dp_packet_init(&packet, 0); >>> + dp_packet_init(&packet, ETH_HEADER_LEN); >>> flow_compose(&packet, &flow, NULL, 64); >>> >>> /* Execute the actions. On older datapaths this fails with EINVAL, on >>> @@ -3501,7 +3501,7 @@ send_pdu_cb(void *port_, const void *pdu, size_t pdu_size) >>> struct dp_packet packet; >>> void *packet_pdu; >>> >>> - dp_packet_init(&packet, 0); >>> + dp_packet_init(&packet, ETH_HEADER_LEN); >>> packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP, >>> pdu_size); >>> memcpy(packet_pdu, pdu, pdu_size); >>> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c >>> index ede7f1e61ab8..55b5e1a4c95a 100644 >>> --- a/utilities/ovs-ofctl.c >>> +++ b/utilities/ovs-ofctl.c >>> @@ -4902,15 +4902,13 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx) >>> } >>> >>> struct dp_packet p; >>> - memset(&p, 0, sizeof p); >>> - dp_packet_init(&p, 0); >>> + dp_packet_init(&p, ETH_HEADER_LEN); >>> >>> void *l7 = NULL; >>> size_t l7_len = 64; >>> if (ctx->argc > 2) { >>> struct dp_packet payload; >>> - memset(&payload, 0, sizeof payload); >>> - dp_packet_init(&payload, 0); >>> + dp_packet_init(&payload, ETH_HEADER_LEN); >>> if (dp_packet_put_hex(&payload, ctx->argv[2], NULL)[0] != '\0') { >>> ovs_fatal(0, "%s: trailing garbage in packet data", ctx->argv[2]); >>> } >> >> If the issue is just in dp_packet_prealloc_tailroom() can we just do a >> size check there? >> >> - if (size > dp_packet_tailroom(b)) { >> + /* check against pkt_size to start, because it is more likely to fail */ >> + if (dp_packet_size(b) < size || size > dp_packet_tailroom(b)) { > > This creates issues. I think due to the fact that the packet tailroom > might shrink in some cases now. > >> >> Otherwise, I think there will be multiple reallocations with all of >> these changes. > > They're mainly in test/init code in which case it doesn't matter too > much. The only exceptions are for bond learn, lacpdu and tunnel arp/nd > request packet composition which shouldn't happen too often I guess. > > In any case, we can also do something similar to what you suggested, > with a small change: > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index 72f6d09ac7f3..8ae528ba6a6e 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -294,7 +294,7 @@ dp_packet_resize(struct dp_packet *b, size_t new_headroom, size_t new_tailroom) > void > dp_packet_prealloc_tailroom(struct dp_packet *b, size_t size) > { > - if (size > dp_packet_tailroom(b)) { > + if ((!dp_packet_base(b) && size) || (size > dp_packet_tailroom(b))) { > dp_packet_resize(b, dp_packet_headroom(b), MAX(size, 64)); > } > } > > This passes tests. What do you think? Looks good to me :) Thanks for following up. >> >> Maybe I missed a different ubsan flake - otherwise, we can also assert >> that size must be non-zero when allocating a dp_packet, but that's a >> bigger change. >> > Thanks, > Dumitru
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 1f386b81bbaf..2bb6c38bd5ca 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -188,7 +188,7 @@ dummy_packet_stream_init(struct dummy_packet_stream *s, struct stream *stream) { int rxbuf_size = stream ? 2048 : 0; s->stream = stream; - dp_packet_init(&s->rxbuf, rxbuf_size); + dp_packet_init(&s->rxbuf, MAX(rxbuf_size, ETH_HEADER_LEN)); ovs_list_init(&s->txq); } @@ -1149,7 +1149,7 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED, if (flow.dl_type == htons(ETH_TYPE_ARP) && flow.nw_proto == ARP_OP_REQUEST && flow.nw_dst == dev->address.s_addr) { - struct dp_packet *reply = dp_packet_new(0); + struct dp_packet *reply = dp_packet_new(ETH_HEADER_LEN); compose_arp(reply, ARP_OP_REPLY, dev->hwaddr, flow.dl_src, false, flow.nw_dst, flow.nw_src); netdev_dummy_queue_packet(dev, reply, NULL, 0); @@ -1647,7 +1647,7 @@ eth_from_flow_str(const char *s, size_t packet_size, return NULL; } - packet = dp_packet_new(0); + packet = dp_packet_new(MAX(packet_size, ETH_HEADER_LEN)); if (packet_size) { flow_compose(packet, flow, NULL, 0); if (dp_packet_size(packet) < packet_size) { diff --git a/ofproto/bond.c b/ofproto/bond.c index cdfdf0b9d8c0..8ab27ed09c3a 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -809,7 +809,8 @@ bond_compose_learning_packet(struct bond *bond, const struct eth_addr eth_src, flow.dl_src = eth_src; member = choose_output_member(bond, &flow, NULL, vlan); - packet = dp_packet_new(0); + packet = dp_packet_new(2 + ETH_HEADER_LEN + VLAN_HEADER_LEN + + ARP_ETH_HEADER_LEN); compose_rarp(packet, eth_src); if (vlan) { eth_push_vlan(packet, htons(ETH_TYPE_VLAN), htons(vlan)); diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c index 78a54c715dc7..b4a9974689d1 100644 --- a/ofproto/ofproto-dpif-trace.c +++ b/ofproto/ofproto-dpif-trace.c @@ -245,7 +245,7 @@ parse_flow_and_packet(int argc, const char *argv[], struct dp_packet payload; memset(&payload, 0, sizeof payload); - dp_packet_init(&payload, 0); + dp_packet_init(&payload, ETH_HEADER_LEN); if (dp_packet_put_hex(&payload, argv[++i], NULL)[0] != '\0') { dp_packet_uninit(&payload); error = xstrdup("Trailing garbage in packet data"); @@ -436,7 +436,7 @@ parse_flow_and_packet(int argc, const char *argv[], if (generate_packet) { /* Generate a packet, as requested. */ - packet = dp_packet_new(0); + packet = dp_packet_new(ETH_HEADER_LEN); flow_compose(packet, flow, l7, l7_len); } else if (packet) { /* Use the metadata from the flow and the packet argument to diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 991e22e6b64b..c1fd4c585d56 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3490,7 +3490,7 @@ tnl_send_nd_request(struct xlate_ctx *ctx, const struct xport *out_dev, { struct dp_packet packet; - dp_packet_init(&packet, 0); + dp_packet_init(&packet, ETH_HEADER_LEN); compose_nd_ns(&packet, eth_src, ipv6_src, ipv6_dst); compose_table_xlate(ctx, out_dev, &packet); dp_packet_uninit(&packet); @@ -3503,7 +3503,7 @@ tnl_send_arp_request(struct xlate_ctx *ctx, const struct xport *out_dev, { struct dp_packet packet; - dp_packet_init(&packet, 0); + dp_packet_init(&packet, ETH_HEADER_LEN); compose_arp(&packet, ARP_OP_REQUEST, eth_src, eth_addr_zero, true, ip_src, ip_dst); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index bc3df8ea1510..e44e365dd405 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1268,7 +1268,7 @@ check_ct_eventmask(struct dpif_backer *backer) nl_msg_end_nested(&actions, ct_start); /* Compose a dummy UDP packet. */ - dp_packet_init(&packet, 0); + dp_packet_init(&packet, ETH_HEADER_LEN); flow_compose(&packet, &flow, NULL, 64); /* Execute the actions. On older datapaths this fails with EINVAL, on @@ -1361,7 +1361,7 @@ check_ct_timeout_policy(struct dpif_backer *backer) nl_msg_end_nested(&actions, ct_start); /* Compose a dummy UDP packet. */ - dp_packet_init(&packet, 0); + dp_packet_init(&packet, ETH_HEADER_LEN); flow_compose(&packet, &flow, NULL, 64); /* Execute the actions. On older datapaths this fails with EINVAL, on @@ -3501,7 +3501,7 @@ send_pdu_cb(void *port_, const void *pdu, size_t pdu_size) struct dp_packet packet; void *packet_pdu; - dp_packet_init(&packet, 0); + dp_packet_init(&packet, ETH_HEADER_LEN); packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP, pdu_size); memcpy(packet_pdu, pdu, pdu_size); diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index ede7f1e61ab8..55b5e1a4c95a 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -4902,15 +4902,13 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx) } struct dp_packet p; - memset(&p, 0, sizeof p); - dp_packet_init(&p, 0); + dp_packet_init(&p, ETH_HEADER_LEN); void *l7 = NULL; size_t l7_len = 64; if (ctx->argc > 2) { struct dp_packet payload; - memset(&payload, 0, sizeof payload); - dp_packet_init(&payload, 0); + dp_packet_init(&payload, ETH_HEADER_LEN); if (dp_packet_put_hex(&payload, ctx->argv[2], NULL)[0] != '\0') { ovs_fatal(0, "%s: trailing garbage in packet data", ctx->argv[2]); }
UB Sanitizer report: lib/dp-packet.h:297:39: runtime error: applying zero offset to null pointer #0 0x7946f5 in dp_packet_tail /root/ovs/./lib/dp-packet.h:297:39 #1 0x794331 in dp_packet_tailroom /root/ovs/./lib/dp-packet.h:325:49 #2 0x7942a0 in dp_packet_prealloc_tailroom /root/ovs/lib/dp-packet.c:297:16 #3 0xc347cf in eth_compose /root/ovs/lib/packets.c:1061:5 [...] Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/netdev-dummy.c | 6 +++--- ofproto/bond.c | 3 ++- ofproto/ofproto-dpif-trace.c | 4 ++-- ofproto/ofproto-dpif-xlate.c | 4 ++-- ofproto/ofproto-dpif.c | 6 +++--- utilities/ovs-ofctl.c | 6 ++---- 6 files changed, 14 insertions(+), 15 deletions(-)