| Message ID | 3d7e0a1ba73c4a415b3ceb8d5ef80b25ff84e12f.1762950453.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Architectural refactoring of hardware offload infrastructure | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Looking at this patch, I have a few questions about the nullable_memcpy fix in queue_netdev_flow_put. commit: 3d7e0a1ba73c4a415b3ceb8d5ef80b25ff84e12f Author: Eelco Chaudron <echaudro@redhat.com> Subject: dpif_netdev: Fix nullable memcpy in queue_netdev_flow_put(). This patch addresses undefined behavior when memcpy is called with NULL actions parameter in the flow offload probe functions, which install flows without any actions. > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 484eefccb..586b589a3 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -3090,7 +3090,7 @@ queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd, > flow_offload = &item->data->flow; > flow_offload->match = *match; > flow_offload->actions = xmalloc(actions_len); > - memcpy(flow_offload->actions, actions, actions_len); > + nullable_memcpy(flow_offload->actions, actions, actions_len); > flow_offload->actions_len = actions_len; > flow_offload->orig_in_port = flow->orig_in_port; Could this code be allocating memory unnecessarily when actions is NULL? The xmalloc call occurs regardless of whether actions is NULL, and when actions_len is 0, xmalloc still allocates a small amount of memory that may never be used. Would it make more sense to check if actions_len is 0 and set flow_offload->actions to NULL instead of allocating memory?
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 484eefccb..586b589a3 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3090,7 +3090,7 @@ queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd, flow_offload = &item->data->flow; flow_offload->match = *match; flow_offload->actions = xmalloc(actions_len); - memcpy(flow_offload->actions, actions, actions_len); + nullable_memcpy(flow_offload->actions, actions, actions_len); flow_offload->actions_len = actions_len; flow_offload->orig_in_port = flow->orig_in_port;
The probe functions try to install a flow without any actions. For some reason, this only triggers an undefined behavior error after the next patch in this series. Fixes: 02bb2824e51d ("dpif-netdev: do hw flow offload in a thread") Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/dpif-netdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)