Messages in this thread | | | Date | Fri, 10 Feb 2023 09:52:27 +0800 | Subject | Re: [ovs-dev] [PATCH v2] net: openvswitch: fix possible memory leak in ovs_meter_cmd_set() | From | Hangyu Hua <> |
| |
On 9/2/2023 21:42, Eelco Chaudron wrote: > > > On 9 Feb 2023, at 11:58, Simon Horman wrote: > >> On Thu, Feb 09, 2023 at 05:32:40PM +0800, Hangyu Hua wrote: >>> old_meter needs to be free after it is detached regardless of whether >>> the new meter is successfully attached. >>> >>> Fixes: c7c4c44c9a95 ("net: openvswitch: expand the meters supported number") >>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >>> --- >>> >>> v2: use goto label and free old_meter outside of ovs lock. >>> >>> net/openvswitch/meter.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c >>> index 6e38f68f88c2..9b680f0894f1 100644 >>> --- a/net/openvswitch/meter.c >>> +++ b/net/openvswitch/meter.c >>> @@ -417,6 +417,7 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info) >>> int err; >>> u32 meter_id; >>> bool failed; >>> + bool locked = true; >>> >>> if (!a[OVS_METER_ATTR_ID]) >>> return -EINVAL; >>> @@ -448,11 +449,13 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info) >>> goto exit_unlock; >>> >>> err = attach_meter(meter_tbl, meter); >>> - if (err) >>> - goto exit_unlock; >>> >>> ovs_unlock(); >>> >>> + if (err) { >>> + locked = false; >>> + goto exit_free_old_meter; >>> + } >>> /* Build response with the meter_id and stats from >>> * the old meter, if any. >>> */ >>> @@ -472,8 +475,11 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info) >>> genlmsg_end(reply, ovs_reply_header); >>> return genlmsg_reply(reply, info); >>> >>> +exit_free_old_meter: >>> + ovs_meter_free(old_meter); >>> exit_unlock: >>> - ovs_unlock(); >>> + if (locked) >>> + ovs_unlock(); >> >> I see where you are going here, but is the complexity of the >> locked variable worth the benefit of freeing old_meter outside >> the lock? > > Looking at the error path, I would agree with Simon, and just add an “exit_free_old_meter” label as suggested in v1 and keep the lock in place to make the error path more straightforward. > > //Eelco
I see. I will send a v3 later.
Thanks, Hangyu
> >>> nlmsg_free(reply); >>> exit_free_meter: >>> kfree(meter); >>> -- >>> 2.34.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >
| |