| Message ID | 20230209093240.14685-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: openvswitch: fix possible memory leak in ovs_meter_cmd_set() | expand |
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? > 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 >
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 >> 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 >>
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 >>> >
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(); nlmsg_free(reply); exit_free_meter: kfree(meter);
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(-)