| Message ID | CAJBeyFHrv+xMV0qivRC=AAzrzQGks6DOx0KZJrHzaofDPtaM1g@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kevin Traynor |
| Headers | show |
| Series | [ovs-dev] netdev-dpdk: Fix possible memory leak when configuring rx-steering | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | fail | cirrus build: failed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 4/3/26 1:07 PM, Mikhail Dmitrichenko wrote: > From 81def5ff65881ce6cb4ccdd23a34da8a2870568f Mon Sep 17 00:00:00 2001 > From: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com> > Date: Fri, 3 Apr 2026 14:19:13 +0300 > Subject: [PATCH] netdev-dpdk: Fix possible memory leak when configuring > rx-steering > > VLOG_WARN_BUF() allocates a new string with xasprintf() every time it is > called and overwrites *errp without freeing the previous value. This can > lead to a memory leak if multiple warnings are emitted or if a later > hard error in netdev_dpdk_set_config() also writes > to errp. > > The three cases in dpdk_set_rx_steer_config() are not fatal errors: > - unsupported rx-steering value > - rss+lacp on non-ethernet port > - rss+lacp together with hw-offload > > In all cases program simply log a warning and fall back to default RSS > steering. Configuration continues normally (err remains 0 and execution > flow do not goto out). > > Therefore change them to plain VLOG_WARN(). As a result the 'errp' > parameter becomes unused and is removed from the function signature and > the call site in netdev_dpdk_set_config(). > > This makes the code cleaner and consistent with the rest of netdev-dpdk. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > Hi Mikhail, Thank you for the fix. Indeed, we don't need to set errp for these cases and it could cause a potential leak in the event of multiple config issues. It looks like there is some wrapping in the email on the @@'s which confused patchwork [0] and I wasn't able to apply the patch as sent. Would you be able to send a v2 without wrapping ? (Also, if you are re-sending there is a couple of minor style items below) If you have problems sending in correct format, let me know and I can fix it up locally. thanks, Kevin. [0] https://patchwork.ozlabs.org/project/openvswitch/patch/CAJBeyFHrv+xMV0qivRC=AAzrzQGks6DOx0KZJrHzaofDPtaM1g@mail.gmail.com/ > Fixes: fc06ea ("netdev-dpdk: Add custom rx-steering configuration.") We typically use longer hash fc06ea9a1883 ("netdev-dpdk: Add custom rx-steering configuration.") > Signed-off-by: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com> > --- > lib/netdev-dpdk.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 54959ff0d..9fbb204ef 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2251,7 +2251,7 @@ dpdk_process_queue_size(struct netdev *netdev, > const struct smap *args, > ^^^ e.g. line wrapping here and break is confusing patchwork/git am. > static void > dpdk_set_rx_steer_config(struct netdev *netdev, struct netdev_dpdk *dev, > - const struct smap *args, char **errp) > + const struct smap *args) > { > const char *arg = smap_get_def(args, "rx-steering", "rss"); > uint64_t flags = 0; > @@ -2259,22 +2259,22 @@ dpdk_set_rx_steer_config(struct netdev > *netdev, struct netdev_dpdk *dev, > if (!strcmp(arg, "rss+lacp")) { > flags = DPDK_RX_STEER_LACP; > } else if (strcmp(arg, "rss")) { > - VLOG_WARN_BUF(errp, "%s: options:rx-steering " > - "unsupported parameter value '%s'", > - netdev_get_name(netdev), arg); > + VLOG_WARN("%s: options:rx-steering " > + "unsupported parameter value '%s'", We don't need to wrap the string anymore > + netdev_get_name(netdev), arg); > } > > if (flags && dev->type != DPDK_DEV_ETH) { > - VLOG_WARN_BUF(errp, "%s: options:rx-steering " > - "is only supported on ethernet ports", > - netdev_get_name(netdev)); > + VLOG_WARN("%s: options:rx-steering " > + "is only supported on ethernet ports", > + netdev_get_name(netdev)); > flags = 0; > } > > if (flags && dpif_offload_enabled()) { > - VLOG_WARN_BUF(errp, "%s: options:rx-steering " > - "is incompatible with hw-offload", > - netdev_get_name(netdev)); > + VLOG_WARN("%s: options:rx-steering " > + "is incompatible with hw-offload", We don't need to wrap the string anymore > + netdev_get_name(netdev)); > flags = 0; > } > > @@ -2304,7 +2304,7 @@ netdev_dpdk_set_config(struct netdev *netdev, > const struct smap *args, > ovs_mutex_lock(&dpdk_mutex); > ovs_mutex_lock(&dev->mutex); > > - dpdk_set_rx_steer_config(netdev, dev, args, errp); > + dpdk_set_rx_steer_config(netdev, dev, args); > > dpdk_set_rxq_config(dev, args); >
Hi Kevin! Thank you very much for the review! I sent an updated version of patch but unfortunately it created a new thread https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431643.html in mail list.. Previously, I sent my patch simply from gmail web client and maybe it caused problems with weird wrapping, sorry for that, I will never do it again. Now I configured sending via git send-email, so I hope this time everything will be better. Sorry again for such a mess. Best regards, Mikhail Dmitrichenko On Fri, Apr 10, 2026 at 2:04 PM Kevin Traynor <ktraynor@redhat.com> wrote: > > On 4/3/26 1:07 PM, Mikhail Dmitrichenko wrote: > > From 81def5ff65881ce6cb4ccdd23a34da8a2870568f Mon Sep 17 00:00:00 2001 > > From: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com> > > Date: Fri, 3 Apr 2026 14:19:13 +0300 > > Subject: [PATCH] netdev-dpdk: Fix possible memory leak when configuring > > rx-steering > > > > VLOG_WARN_BUF() allocates a new string with xasprintf() every time it is > > called and overwrites *errp without freeing the previous value. This can > > lead to a memory leak if multiple warnings are emitted or if a later > > hard error in netdev_dpdk_set_config() also writes > > to errp. > > > > The three cases in dpdk_set_rx_steer_config() are not fatal errors: > > - unsupported rx-steering value > > - rss+lacp on non-ethernet port > > - rss+lacp together with hw-offload > > > > In all cases program simply log a warning and fall back to default RSS > > steering. Configuration continues normally (err remains 0 and execution > > flow do not goto out). > > > > Therefore change them to plain VLOG_WARN(). As a result the 'errp' > > parameter becomes unused and is removed from the function signature and > > the call site in netdev_dpdk_set_config(). > > > > This makes the code cleaner and consistent with the rest of netdev-dpdk. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Hi Mikhail, > > Thank you for the fix. Indeed, we don't need to set errp for these cases > and it could cause a potential leak in the event of multiple config issues. > > It looks like there is some wrapping in the email on the @@'s which > confused patchwork [0] and I wasn't able to apply the patch as sent. > > Would you be able to send a v2 without wrapping ? (Also, if you are > re-sending there is a couple of minor style items below) > > If you have problems sending in correct format, let me know and I can > fix it up locally. > > thanks, > Kevin. > > [0] > https://patchwork.ozlabs.org/project/openvswitch/patch/CAJBeyFHrv+xMV0qivRC=AAzrzQGks6DOx0KZJrHzaofDPtaM1g@mail.gmail.com/ > > > Fixes: fc06ea ("netdev-dpdk: Add custom rx-steering configuration.") > > We typically use longer hash > fc06ea9a1883 ("netdev-dpdk: Add custom rx-steering configuration.") > > > Signed-off-by: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com> > > --- > > lib/netdev-dpdk.c | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index 54959ff0d..9fbb204ef 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -2251,7 +2251,7 @@ dpdk_process_queue_size(struct netdev *netdev, > > const struct smap *args, > > > > ^^^ e.g. line wrapping here and break is confusing patchwork/git am. > > > static void > > dpdk_set_rx_steer_config(struct netdev *netdev, struct netdev_dpdk *dev, > > - const struct smap *args, char **errp) > > + const struct smap *args) > > { > > const char *arg = smap_get_def(args, "rx-steering", "rss"); > > uint64_t flags = 0; > > @@ -2259,22 +2259,22 @@ dpdk_set_rx_steer_config(struct netdev > > *netdev, struct netdev_dpdk *dev, > > if (!strcmp(arg, "rss+lacp")) { > > flags = DPDK_RX_STEER_LACP; > > } else if (strcmp(arg, "rss")) { > > - VLOG_WARN_BUF(errp, "%s: options:rx-steering " > > - "unsupported parameter value '%s'", > > - netdev_get_name(netdev), arg); > > + VLOG_WARN("%s: options:rx-steering " > > + "unsupported parameter value '%s'", > > We don't need to wrap the string anymore > > > + netdev_get_name(netdev), arg); > > } > > > > if (flags && dev->type != DPDK_DEV_ETH) { > > - VLOG_WARN_BUF(errp, "%s: options:rx-steering " > > - "is only supported on ethernet ports", > > - netdev_get_name(netdev)); > > + VLOG_WARN("%s: options:rx-steering " > > + "is only supported on ethernet ports", > > + netdev_get_name(netdev)); > > flags = 0; > > } > > > > if (flags && dpif_offload_enabled()) { > > - VLOG_WARN_BUF(errp, "%s: options:rx-steering " > > - "is incompatible with hw-offload", > > - netdev_get_name(netdev)); > > + VLOG_WARN("%s: options:rx-steering " > > + "is incompatible with hw-offload", > > We don't need to wrap the string anymore > > > + netdev_get_name(netdev)); > > flags = 0; > > } > > > > @@ -2304,7 +2304,7 @@ netdev_dpdk_set_config(struct netdev *netdev, > > const struct smap *args, > > ovs_mutex_lock(&dpdk_mutex); > > ovs_mutex_lock(&dev->mutex); > > > > - dpdk_set_rx_steer_config(netdev, dev, args, errp); > > + dpdk_set_rx_steer_config(netdev, dev, args); > > > > dpdk_set_rxq_config(dev, args); > > >
On 4/10/26 3:25 PM, Mikhail Dmitrichenko wrote: > Hi Kevin! > > Thank you very much for the review! I sent an updated version of patch > but unfortunately it created a new thread > https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431643.html New thread is actually the custom for ovs dev mailing list so you aligned with it, even if it was not deliberate :-) > in mail list.. Previously, I sent my patch simply from gmail web client > and maybe it caused problems with weird wrapping, sorry for that, I > will never do it again. Now I configured sending via git send-email, > so I hope this time everything will be better. Sorry again for such a > mess. > No problem at all. Yes, gmail may cause that. Thanks for the new version. > Best regards, > Mikhail Dmitrichenko > > > On Fri, Apr 10, 2026 at 2:04 PM Kevin Traynor <ktraynor@redhat.com> wrote: >> >> On 4/3/26 1:07 PM, Mikhail Dmitrichenko wrote: >>> From 81def5ff65881ce6cb4ccdd23a34da8a2870568f Mon Sep 17 00:00:00 2001 >>> From: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com> >>> Date: Fri, 3 Apr 2026 14:19:13 +0300 >>> Subject: [PATCH] netdev-dpdk: Fix possible memory leak when configuring >>> rx-steering >>> >>> VLOG_WARN_BUF() allocates a new string with xasprintf() every time it is >>> called and overwrites *errp without freeing the previous value. This can >>> lead to a memory leak if multiple warnings are emitted or if a later >>> hard error in netdev_dpdk_set_config() also writes >>> to errp. >>> >>> The three cases in dpdk_set_rx_steer_config() are not fatal errors: >>> - unsupported rx-steering value >>> - rss+lacp on non-ethernet port >>> - rss+lacp together with hw-offload >>> >>> In all cases program simply log a warning and fall back to default RSS >>> steering. Configuration continues normally (err remains 0 and execution >>> flow do not goto out). >>> >>> Therefore change them to plain VLOG_WARN(). As a result the 'errp' >>> parameter becomes unused and is removed from the function signature and >>> the call site in netdev_dpdk_set_config(). >>> >>> This makes the code cleaner and consistent with the rest of netdev-dpdk. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >> >> Hi Mikhail, >> >> Thank you for the fix. Indeed, we don't need to set errp for these cases >> and it could cause a potential leak in the event of multiple config issues. >> >> It looks like there is some wrapping in the email on the @@'s which >> confused patchwork [0] and I wasn't able to apply the patch as sent. >> >> Would you be able to send a v2 without wrapping ? (Also, if you are >> re-sending there is a couple of minor style items below) >> >> If you have problems sending in correct format, let me know and I can >> fix it up locally. >> >> thanks, >> Kevin. >> >> [0] >> https://patchwork.ozlabs.org/project/openvswitch/patch/CAJBeyFHrv+xMV0qivRC=AAzrzQGks6DOx0KZJrHzaofDPtaM1g@mail.gmail.com/ >> >>> Fixes: fc06ea ("netdev-dpdk: Add custom rx-steering configuration.") >> >> We typically use longer hash >> fc06ea9a1883 ("netdev-dpdk: Add custom rx-steering configuration.") >> >>> Signed-off-by: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com> >>> --- >>> lib/netdev-dpdk.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 54959ff0d..9fbb204ef 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -2251,7 +2251,7 @@ dpdk_process_queue_size(struct netdev *netdev, >>> const struct smap *args, >>> >> >> ^^^ e.g. line wrapping here and break is confusing patchwork/git am. >> >>> static void >>> dpdk_set_rx_steer_config(struct netdev *netdev, struct netdev_dpdk *dev, >>> - const struct smap *args, char **errp) >>> + const struct smap *args) >>> { >>> const char *arg = smap_get_def(args, "rx-steering", "rss"); >>> uint64_t flags = 0; >>> @@ -2259,22 +2259,22 @@ dpdk_set_rx_steer_config(struct netdev >>> *netdev, struct netdev_dpdk *dev, >>> if (!strcmp(arg, "rss+lacp")) { >>> flags = DPDK_RX_STEER_LACP; >>> } else if (strcmp(arg, "rss")) { >>> - VLOG_WARN_BUF(errp, "%s: options:rx-steering " >>> - "unsupported parameter value '%s'", >>> - netdev_get_name(netdev), arg); >>> + VLOG_WARN("%s: options:rx-steering " >>> + "unsupported parameter value '%s'", >> >> We don't need to wrap the string anymore >> >>> + netdev_get_name(netdev), arg); >>> } >>> >>> if (flags && dev->type != DPDK_DEV_ETH) { >>> - VLOG_WARN_BUF(errp, "%s: options:rx-steering " >>> - "is only supported on ethernet ports", >>> - netdev_get_name(netdev)); >>> + VLOG_WARN("%s: options:rx-steering " >>> + "is only supported on ethernet ports", >>> + netdev_get_name(netdev)); >>> flags = 0; >>> } >>> >>> if (flags && dpif_offload_enabled()) { >>> - VLOG_WARN_BUF(errp, "%s: options:rx-steering " >>> - "is incompatible with hw-offload", >>> - netdev_get_name(netdev)); >>> + VLOG_WARN("%s: options:rx-steering " >>> + "is incompatible with hw-offload", >> >> We don't need to wrap the string anymore >> >>> + netdev_get_name(netdev)); >>> flags = 0; >>> } >>> >>> @@ -2304,7 +2304,7 @@ netdev_dpdk_set_config(struct netdev *netdev, >>> const struct smap *args, >>> ovs_mutex_lock(&dpdk_mutex); >>> ovs_mutex_lock(&dev->mutex); >>> >>> - dpdk_set_rx_steer_config(netdev, dev, args, errp); >>> + dpdk_set_rx_steer_config(netdev, dev, args); >>> >>> dpdk_set_rxq_config(dev, args); >>> >> >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 54959ff0d..9fbb204ef 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2251,7 +2251,7 @@ dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, static void dpdk_set_rx_steer_config(struct netdev *netdev, struct netdev_dpdk *dev, - const struct smap *args, char **errp) + const struct smap *args) { const char *arg = smap_get_def(args, "rx-steering", "rss");