Re: [PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()

[Simon Horman <horms@kernel.org>]

On Thu, Sep 07, 2023 at 10:03:58PM +0800, Jinjie Ruan wrote:
> Inject fault When select CONFIG_VCAP_KUNIT_TEST, the below memory leak
> occurs. If kzalloc() for duprule succeeds, but the following
> kmemdup() fails, the duprule, ckf and caf memory will be leaked. So kfree
> them in the error path.
> 
> unreferenced object 0xffff122744c50600 (size 192):
>   comm "kunit_try_catch", pid 346, jiffies 4294896122 (age 911.812s)
>   hex dump (first 32 bytes):
>     10 27 00 00 04 00 00 00 1e 00 00 00 2c 01 00 00  .'..........,...
>     00 00 00 00 00 00 00 00 18 06 c5 44 27 12 ff ff  ...........D'...
>   backtrace:
>     [<00000000394b0db8>] __kmem_cache_alloc_node+0x274/0x2f8
>     [<0000000001bedc67>] kmalloc_trace+0x38/0x88
>     [<00000000b0612f98>] vcap_dup_rule+0x50/0x460
>     [<000000005d2d3aca>] vcap_add_rule+0x8cc/0x1038
>     [<00000000eef9d0f8>] test_vcap_xn_rule_creator.constprop.0.isra.0+0x238/0x494
>     [<00000000cbda607b>] vcap_api_rule_remove_in_front_test+0x1ac/0x698
>     [<00000000c8766299>] kunit_try_run_case+0xe0/0x20c
>     [<00000000c4fe9186>] kunit_generic_run_threadfn_adapter+0x50/0x94
>     [<00000000f6864acf>] kthread+0x2e8/0x374
>     [<0000000022e639b3>] ret_from_fork+0x10/0x20
> 
> Fixes: 814e7693207f ("net: microchip: vcap api: Add a storage state to a VCAP rule")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/net/ethernet/microchip/vcap/vcap_api.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> index 300fe1a93dce..ef980e4e5bc2 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> @@ -1021,18 +1021,32 @@ static struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri,
>  	list_for_each_entry(ckf, &ri->data.keyfields, ctrl.list) {
>  		newckf = kmemdup(ckf, sizeof(*newckf), GFP_KERNEL);
>  		if (!newckf)
> -			return ERR_PTR(-ENOMEM);
> +			goto err;
>  		list_add_tail(&newckf->ctrl.list, &duprule->data.keyfields);
>  	}
>  
>  	list_for_each_entry(caf, &ri->data.actionfields, ctrl.list) {
>  		newcaf = kmemdup(caf, sizeof(*newcaf), GFP_KERNEL);
>  		if (!newcaf)
> -			return ERR_PTR(-ENOMEM);
> +			goto err;
>  		list_add_tail(&newcaf->ctrl.list, &duprule->data.actionfields);
>  	}
>  
>  	return duprule;
> +
> +err:

Hi Jinjie Ruan,

I think it would be slightly more idiomatic, and in keeping with the
prevailing style of this file to call the label out_free. But I don't feel
strongly about that. And clearly it would have no effect on the logic.

That not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> +	list_for_each_entry_safe(ckf, newckf, &duprule->data.keyfields, ctrl.list) {
> +		list_del(&ckf->ctrl.list);
> +		kfree(ckf);
> +	}
> +
> +	list_for_each_entry_safe(caf, newcaf, &duprule->data.actionfields, ctrl.list) {
> +		list_del(&caf->ctrl.list);
> +		kfree(caf);
> +	}
> +
> +	kfree(duprule);
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  static void vcap_apply_width(u8 *dst, int width, int bytes)