All of lore.kernel.org
 help / color / mirror / Atom feed
From: Simon Horman <horms@kernel.org>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
	Lars Povlsen <lars.povlsen@microchip.com>,
	Steen Hegelund <Steen.Hegelund@microchip.com>,
	Daniel Machon <daniel.machon@microchip.com>,
	UNGLinuxDriver@microchip.com,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Horatiu Vultur <horatiu.vultur@microchip.com>
Subject: Re: [PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()
Date: Sun, 10 Sep 2023 15:21:30 +0200	[thread overview]
Message-ID: <20230910132130.GA775887@kernel.org> (raw)
In-Reply-To: <20230907140359.2399646-1-ruanjinjie@huawei.com>

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)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Simon Horman <horms@kernel.org>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
	Lars Povlsen <lars.povlsen@microchip.com>,
	Steen Hegelund <Steen.Hegelund@microchip.com>,
	Daniel Machon <daniel.machon@microchip.com>,
	UNGLinuxDriver@microchip.com,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Horatiu Vultur <horatiu.vultur@microchip.com>
Subject: Re: [PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()
Date: Sun, 10 Sep 2023 15:21:30 +0200	[thread overview]
Message-ID: <20230910132130.GA775887@kernel.org> (raw)
In-Reply-To: <20230907140359.2399646-1-ruanjinjie@huawei.com>

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)

  reply	other threads:[~2023-09-10 13:22 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-07 14:03 [PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule() Jinjie Ruan
2023-09-07 14:03 ` Jinjie Ruan
2023-09-10 13:21 ` Simon Horman [this message]
2023-09-10 13:21   ` Simon Horman
2023-09-10 18:00 ` patchwork-bot+netdevbpf
2023-09-10 18:00   ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230910132130.GA775887@kernel.org \
    --to=horms@kernel.org \
    --cc=Steen.Hegelund@microchip.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=daniel.machon@microchip.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=horatiu.vultur@microchip.com \
    --cc=kuba@kernel.org \
    --cc=lars.povlsen@microchip.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=ruanjinjie@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.