| Message ID | 20240416132149.1698611-1-roid@nvidia.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,1/1] netdev-dpdk: Fix possible memory leak configuring VF MAC address. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, Apr 16, 2024 at 04:21:48PM +0300, Roi Dayan via dev wrote: > VLOG_WARN_BUF() is allocating memory for the error string and should > e used if the configuration cannot continue and error is being returned > so the caller has indication of releasing the pointer. > Change to VLOG_WARN() to keep the logic that error is not being > returned. > > Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.") > Signed-off-by: Roi Dayan <roid@nvidia.com> > Acked-by: Gaetan Rivet <gaetanr@nvidia.com> > Acked-by: Eli Britstein <elibr@nvidia.com> > --- > lib/netdev-dpdk.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 2111f776810b..9249b9e9c646 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > struct eth_addr mac; > > if (!dpdk_port_is_representor(dev)) { > - VLOG_WARN_BUF(errp, "'%s' is trying to set the VF MAC '%s' " > - "but 'options:dpdk-vf-mac' is only supported for " > - "VF representors.", > - netdev_get_name(netdev), vf_mac); > + VLOG_WARN("'%s' is trying to set the VF MAC '%s' " > + "but 'options:dpdk-vf-mac' is only supported for " > + "VF representors.", > + netdev_get_name(netdev), vf_mac); > } else if (!eth_addr_from_string(vf_mac, &mac)) { > - VLOG_WARN_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.", > - netdev_get_name(netdev), vf_mac); > + VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.", > + netdev_get_name(netdev), vf_mac); > } else if (eth_addr_is_multicast(mac)) { > - VLOG_WARN_BUF(errp, > - "interface '%s': cannot set VF MAC to multicast " > - "address '%s'.", netdev_get_name(netdev), vf_mac); > + VLOG_WARN("interface '%s': cannot set VF MAC to multicast " > + "address '%s'.", netdev_get_name(netdev), vf_mac); > } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) { > dev->requested_hwaddr = mac; > netdev_request_reconfigure(netdev); Thanks Roi, I agree that this change makes sense as the allocated value will typically be discarded. And I think if VLOG_WARN_BUF() is called again in the same invocation of netdev_dpdk_set_config() a memory leak occurs. Acked-by: Simon Horman <horms@ovn.org> After reviewing your patch I am now wondering if, conversely, the following change _also_ makes sense: diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 2111f776810b..32d4193d24af 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, } err = 0; /* Not fatal. */ } else { - VLOG_WARN("%s: Cannot get flow control parameters: %s", - netdev_get_name(netdev), rte_strerror(err)); + VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s", + netdev_get_name(netdev), rte_strerror(err)); } goto out; }
On 16/04/2024 18:48, Simon Horman wrote: > On Tue, Apr 16, 2024 at 04:21:48PM +0300, Roi Dayan via dev wrote: >> VLOG_WARN_BUF() is allocating memory for the error string and should >> e used if the configuration cannot continue and error is being returned >> so the caller has indication of releasing the pointer. >> Change to VLOG_WARN() to keep the logic that error is not being >> returned. >> >> Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.") >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> Acked-by: Gaetan Rivet <gaetanr@nvidia.com> >> Acked-by: Eli Britstein <elibr@nvidia.com> >> --- >> lib/netdev-dpdk.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 2111f776810b..9249b9e9c646 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, >> struct eth_addr mac; >> >> if (!dpdk_port_is_representor(dev)) { >> - VLOG_WARN_BUF(errp, "'%s' is trying to set the VF MAC '%s' " >> - "but 'options:dpdk-vf-mac' is only supported for " >> - "VF representors.", >> - netdev_get_name(netdev), vf_mac); >> + VLOG_WARN("'%s' is trying to set the VF MAC '%s' " >> + "but 'options:dpdk-vf-mac' is only supported for " >> + "VF representors.", >> + netdev_get_name(netdev), vf_mac); >> } else if (!eth_addr_from_string(vf_mac, &mac)) { >> - VLOG_WARN_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.", >> - netdev_get_name(netdev), vf_mac); >> + VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.", >> + netdev_get_name(netdev), vf_mac); >> } else if (eth_addr_is_multicast(mac)) { >> - VLOG_WARN_BUF(errp, >> - "interface '%s': cannot set VF MAC to multicast " >> - "address '%s'.", netdev_get_name(netdev), vf_mac); >> + VLOG_WARN("interface '%s': cannot set VF MAC to multicast " >> + "address '%s'.", netdev_get_name(netdev), vf_mac); >> } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) { >> dev->requested_hwaddr = mac; >> netdev_request_reconfigure(netdev); > > Thanks Roi, > > I agree that this change makes sense as the allocated > value will typically be discarded. And I think if > VLOG_WARN_BUF() is called again in the same invocation of > netdev_dpdk_set_config() a memory leak occurs. > > Acked-by: Simon Horman <horms@ovn.org> > > After reviewing your patch I am now wondering if, conversely, > the following change _also_ makes sense: > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 2111f776810b..32d4193d24af 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > } > err = 0; /* Not fatal. */ > } else { > - VLOG_WARN("%s: Cannot get flow control parameters: %s", > - netdev_get_name(netdev), rte_strerror(err)); > + VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s", > + netdev_get_name(netdev), rte_strerror(err)); > } > goto out; > } Hi, thanks for the review. yes it can improve the error to the user in ovs-vsctl show. error: "enp8s0f1: could not set configuration (Invalid argument)" vs error: "enp8s0f1: Cannot get flow control parameters: Invalid argument" I'll do it in a different commit as its not related to the memleak fix. Thanks, Roi
On Wed, Apr 17, 2024 at 10:43:11AM +0300, Roi Dayan wrote: > > > On 16/04/2024 18:48, Simon Horman wrote: > > On Tue, Apr 16, 2024 at 04:21:48PM +0300, Roi Dayan via dev wrote: > >> VLOG_WARN_BUF() is allocating memory for the error string and should > >> e used if the configuration cannot continue and error is being returned > >> so the caller has indication of releasing the pointer. > >> Change to VLOG_WARN() to keep the logic that error is not being > >> returned. > >> > >> Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.") > >> Signed-off-by: Roi Dayan <roid@nvidia.com> > >> Acked-by: Gaetan Rivet <gaetanr@nvidia.com> > >> Acked-by: Eli Britstein <elibr@nvidia.com> > >> --- > >> lib/netdev-dpdk.c | 17 ++++++++--------- > >> 1 file changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index 2111f776810b..9249b9e9c646 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > >> struct eth_addr mac; > >> > >> if (!dpdk_port_is_representor(dev)) { > >> - VLOG_WARN_BUF(errp, "'%s' is trying to set the VF MAC '%s' " > >> - "but 'options:dpdk-vf-mac' is only supported for " > >> - "VF representors.", > >> - netdev_get_name(netdev), vf_mac); > >> + VLOG_WARN("'%s' is trying to set the VF MAC '%s' " > >> + "but 'options:dpdk-vf-mac' is only supported for " > >> + "VF representors.", > >> + netdev_get_name(netdev), vf_mac); > >> } else if (!eth_addr_from_string(vf_mac, &mac)) { > >> - VLOG_WARN_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.", > >> - netdev_get_name(netdev), vf_mac); > >> + VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.", > >> + netdev_get_name(netdev), vf_mac); > >> } else if (eth_addr_is_multicast(mac)) { > >> - VLOG_WARN_BUF(errp, > >> - "interface '%s': cannot set VF MAC to multicast " > >> - "address '%s'.", netdev_get_name(netdev), vf_mac); > >> + VLOG_WARN("interface '%s': cannot set VF MAC to multicast " > >> + "address '%s'.", netdev_get_name(netdev), vf_mac); > >> } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) { > >> dev->requested_hwaddr = mac; > >> netdev_request_reconfigure(netdev); > > > > Thanks Roi, > > > > I agree that this change makes sense as the allocated > > value will typically be discarded. And I think if > > VLOG_WARN_BUF() is called again in the same invocation of > > netdev_dpdk_set_config() a memory leak occurs. > > > > Acked-by: Simon Horman <horms@ovn.org> > > > > After reviewing your patch I am now wondering if, conversely, > > the following change _also_ makes sense: > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > index 2111f776810b..32d4193d24af 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > > } > > err = 0; /* Not fatal. */ > > } else { > > - VLOG_WARN("%s: Cannot get flow control parameters: %s", > > - netdev_get_name(netdev), rte_strerror(err)); > > + VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s", > > + netdev_get_name(netdev), rte_strerror(err)); > > } > > goto out; > > } > > Hi, > > thanks for the review. yes it can improve the error to the user in ovs-vsctl show. > > error: "enp8s0f1: could not set configuration (Invalid argument)" > vs > > error: "enp8s0f1: Cannot get flow control parameters: Invalid argument" > > > I'll do it in a different commit as its not related to the memleak fix. Thanks, I have gone ahead and applied this patch to main. - netdev-dpdk: Fix possible memory leak configuring VF MAC address. https://github.com/openvswitch/ovs/commit/4f29804f249b As a follow-up I will prepare backports back to v2.17.
On Tue, Apr 23, 2024 at 11:33:15AM +0100, Simon Horman wrote: > On Wed, Apr 17, 2024 at 10:43:11AM +0300, Roi Dayan wrote: > > > > > > On 16/04/2024 18:48, Simon Horman wrote: > > > On Tue, Apr 16, 2024 at 04:21:48PM +0300, Roi Dayan via dev wrote: > > >> VLOG_WARN_BUF() is allocating memory for the error string and should > > >> e used if the configuration cannot continue and error is being returned > > >> so the caller has indication of releasing the pointer. > > >> Change to VLOG_WARN() to keep the logic that error is not being > > >> returned. > > >> > > >> Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.") > > >> Signed-off-by: Roi Dayan <roid@nvidia.com> > > >> Acked-by: Gaetan Rivet <gaetanr@nvidia.com> > > >> Acked-by: Eli Britstein <elibr@nvidia.com> > > >> --- > > >> lib/netdev-dpdk.c | 17 ++++++++--------- > > >> 1 file changed, 8 insertions(+), 9 deletions(-) > > >> > > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > >> index 2111f776810b..9249b9e9c646 100644 > > >> --- a/lib/netdev-dpdk.c > > >> +++ b/lib/netdev-dpdk.c > > >> @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > > >> struct eth_addr mac; > > >> > > >> if (!dpdk_port_is_representor(dev)) { > > >> - VLOG_WARN_BUF(errp, "'%s' is trying to set the VF MAC '%s' " > > >> - "but 'options:dpdk-vf-mac' is only supported for " > > >> - "VF representors.", > > >> - netdev_get_name(netdev), vf_mac); > > >> + VLOG_WARN("'%s' is trying to set the VF MAC '%s' " > > >> + "but 'options:dpdk-vf-mac' is only supported for " > > >> + "VF representors.", > > >> + netdev_get_name(netdev), vf_mac); > > >> } else if (!eth_addr_from_string(vf_mac, &mac)) { > > >> - VLOG_WARN_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.", > > >> - netdev_get_name(netdev), vf_mac); > > >> + VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.", > > >> + netdev_get_name(netdev), vf_mac); > > >> } else if (eth_addr_is_multicast(mac)) { > > >> - VLOG_WARN_BUF(errp, > > >> - "interface '%s': cannot set VF MAC to multicast " > > >> - "address '%s'.", netdev_get_name(netdev), vf_mac); > > >> + VLOG_WARN("interface '%s': cannot set VF MAC to multicast " > > >> + "address '%s'.", netdev_get_name(netdev), vf_mac); > > >> } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) { > > >> dev->requested_hwaddr = mac; > > >> netdev_request_reconfigure(netdev); > > > > > > Thanks Roi, > > > > > > I agree that this change makes sense as the allocated > > > value will typically be discarded. And I think if > > > VLOG_WARN_BUF() is called again in the same invocation of > > > netdev_dpdk_set_config() a memory leak occurs. > > > > > > Acked-by: Simon Horman <horms@ovn.org> > > > > > > After reviewing your patch I am now wondering if, conversely, > > > the following change _also_ makes sense: > > > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > > > index 2111f776810b..32d4193d24af 100644 > > > --- a/lib/netdev-dpdk.c > > > +++ b/lib/netdev-dpdk.c > > > @@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > > > } > > > err = 0; /* Not fatal. */ > > > } else { > > > - VLOG_WARN("%s: Cannot get flow control parameters: %s", > > > - netdev_get_name(netdev), rte_strerror(err)); > > > + VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s", > > > + netdev_get_name(netdev), rte_strerror(err)); > > > } > > > goto out; > > > } > > > > Hi, > > > > thanks for the review. yes it can improve the error to the user in ovs-vsctl show. > > > > error: "enp8s0f1: could not set configuration (Invalid argument)" > > vs > > > > error: "enp8s0f1: Cannot get flow control parameters: Invalid argument" > > > > > > I'll do it in a different commit as its not related to the memleak fix. > > Thanks, > > I have gone ahead and applied this patch to main. > > - netdev-dpdk: Fix possible memory leak configuring VF MAC address. > https://github.com/openvswitch/ovs/commit/4f29804f249b > > As a follow-up I will prepare backports back to v2.17. I now have applied the following backports. Backport applied to branch-3.3: - netdev-dpdk: Fix possible memory leak configuring VF MAC address. https://github.com/openvswitch/ovs/commit/a6c3b5202c2f Backport applied to branch-3.2: - netdev-dpdk: Fix possible memory leak configuring VF MAC address. https://github.com/openvswitch/ovs/commit/4e4463ca5539 Backport applied to branch-3.1: - netdev-dpdk: Fix possible memory leak configuring VF MAC address. https://github.com/openvswitch/ovs/commit/6ce0130f1239 Backport applied to branch-3.0: - netdev-dpdk: Fix possible memory leak configuring VF MAC address. https://github.com/openvswitch/ovs/commit/e5f237e9a95d Backport applied to branch-2.17: - netdev-dpdk: Fix possible memory leak configuring VF MAC address. https://github.com/openvswitch/ovs/commit/b2e19110394e
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 2111f776810b..9249b9e9c646 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, struct eth_addr mac; if (!dpdk_port_is_representor(dev)) { - VLOG_WARN_BUF(errp, "'%s' is trying to set the VF MAC '%s' " - "but 'options:dpdk-vf-mac' is only supported for " - "VF representors.", - netdev_get_name(netdev), vf_mac); + VLOG_WARN("'%s' is trying to set the VF MAC '%s' " + "but 'options:dpdk-vf-mac' is only supported for " + "VF representors.", + netdev_get_name(netdev), vf_mac); } else if (!eth_addr_from_string(vf_mac, &mac)) { - VLOG_WARN_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.", - netdev_get_name(netdev), vf_mac); + VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.", + netdev_get_name(netdev), vf_mac); } else if (eth_addr_is_multicast(mac)) { - VLOG_WARN_BUF(errp, - "interface '%s': cannot set VF MAC to multicast " - "address '%s'.", netdev_get_name(netdev), vf_mac); + VLOG_WARN("interface '%s': cannot set VF MAC to multicast " + "address '%s'.", netdev_get_name(netdev), vf_mac); } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) { dev->requested_hwaddr = mac; netdev_request_reconfigure(netdev);
VLOG_WARN_BUF() is allocating memory for the error string and should e used if the configuration cannot continue and error is being returned so the caller has indication of releasing the pointer. Change to VLOG_WARN() to keep the logic that error is not being returned. Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.") Signed-off-by: Roi Dayan <roid@nvidia.com> Acked-by: Gaetan Rivet <gaetanr@nvidia.com> Acked-by: Eli Britstein <elibr@nvidia.com> --- lib/netdev-dpdk.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)