Re: [PATCH] net: openvswitch: fix possible memory leak in ovs_meter_cmd_set()

[Hangyu Hua]

On 9/2/2023 16:26, Eelco Chaudron wrote:
> On 8 Feb 2023, at 8:16, 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@xxxxxxxxx>
> > ---
> >   net/openvswitch/meter.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
> > index 6e38f68f88c2..e84082e209e9 100644
> > --- a/net/openvswitch/meter.c
> > +++ b/net/openvswitch/meter.c
> > @@ -448,8 +448,10 @@ 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)
> > +	if (err) {
> > +		ovs_meter_free(old_meter);
> >   		goto exit_unlock;
>
> It would be nicer to add another goto label like exit_free_old_meter.
>
> +	if (err)
> +     	goto exit_free_old_meter:
>
> exit_free_old_meter:
>      ovs_meter_free(old_meter);
> exit_unlock:
> 	ovs_unlock();
> 	nlmsg_free(reply);
> exit_free_meter:
>
>
> Or maybe it would be even nicer to free the old_meter outside of the global lock?
>
> > +	}
> >
> >   	ovs_unlock();

I get it. I will send a v2.

Thanks,
Hangyu
> > --
> > 2.34.1