From: Jack Morgenstein <jackm@dev.mellanox.co.il> To: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Yishai Hadas <yishaih@dev.mellanox.co.il>, Yishai Hadas <yishaih@mellanox.com>, netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, "jackm@mellanox.com" <jackm@mellanox.com>, Majd Dibbiny <majd@mellanox.com> Subject: Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init() Date: Thu, 11 Feb 2016 11:29:43 +0200 [thread overview] Message-ID: <20160211112943.20c0de19@jpm-OptiPlex-GX620> (raw) In-Reply-To: <87twlgtosn.fsf@rasmusvillemoes.dk> On Wed, 10 Feb 2016 19:15:20 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On Wed, Feb 10 2016, Yishai Hadas <yishaih@dev.mellanox.co.il> wrote: > > >> @@ -2429,7 +2429,7 @@ err_thread: > >> flush_workqueue(priv->mfunc.master.comm_wq); > >> destroy_workqueue(priv->mfunc.master.comm_wq); > >> err_slaves: > >> - while (--i) { > >> + while (i--) { > > > > This fix is wrong as it hits the case that i arrived the last value > > then below code will access to a non valid entry in the array. > > > > The expected fix should be: > > while (--i >= 0) > > > > Huh? They're completely equivalent (given that i is necessarily > non-negative before we evaluate the loop condition) No, they are not equivalent. if i == the max value (dev->num_slaves) when entering your proposed while loop, the kfree call index (i) will be out of range! This can happen, for example, if the failure occurs downstream from the "i" for-loop (e.g., if the call to mlx4_init_resource_tracker() fails). Therefore, we DO require the pre-decrement format. Therefore, the one-line fix proposed by Yishai is the correct fix. >. I don't really > care either way, but git grep says that 'while (i--)' is 5 times more > common than 'while (--i >= 0)'. Not relevant, while (i--) is simply not correct, because of the case where the for-loop involving i completes successfully and an error occurs later. FYI, you also had another bug in your solution -- a double-free when kzalloc for port 2 fails. For your code, you should also have reset s_state->vlan_filter[port] to NULL as shown below: for (port = 1; port <= MLX4_MAX_PORTS; port++) { struct mlx4_vport_state *admin_vport; struct mlx4_vport_state *oper_vport; s_state->vlan_filter[port] = kzalloc(sizeof(struct mlx4_vlan_fltr), GFP_KERNEL); if (!s_state->vlan_filter[port]) { if (--port) { kfree(s_state->vlan_filter[port]); ==> You should have added this s_state->vlan_filter[port] = NULL; } goto err_slaves; } However, again, the correct solution is to do what Yishai suggests: while (--i >= 0) so that if i is already zero the while-loop will not be entered. -Jack > > Rasmus > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > in the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Jack Morgenstein <jackm@dev.mellanox.co.il> To: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Yishai Hadas <yishaih@dev.mellanox.co.il>, Yishai Hadas <yishaih@mellanox.com>, netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, "jackm\@mellanox.com" <jackm@mellanox.com>, Majd Dibbiny <majd@mellanox.com> Subject: Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init() Date: Thu, 11 Feb 2016 11:29:43 +0200 [thread overview] Message-ID: <20160211112943.20c0de19@jpm-OptiPlex-GX620> (raw) In-Reply-To: <87twlgtosn.fsf@rasmusvillemoes.dk> On Wed, 10 Feb 2016 19:15:20 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On Wed, Feb 10 2016, Yishai Hadas <yishaih@dev.mellanox.co.il> wrote: > > >> @@ -2429,7 +2429,7 @@ err_thread: > >> flush_workqueue(priv->mfunc.master.comm_wq); > >> destroy_workqueue(priv->mfunc.master.comm_wq); > >> err_slaves: > >> - while (--i) { > >> + while (i--) { > > > > This fix is wrong as it hits the case that i arrived the last value > > then below code will access to a non valid entry in the array. > > > > The expected fix should be: > > while (--i >= 0) > > > > Huh? They're completely equivalent (given that i is necessarily > non-negative before we evaluate the loop condition) No, they are not equivalent. if i == the max value (dev->num_slaves) when entering your proposed while loop, the kfree call index (i) will be out of range! This can happen, for example, if the failure occurs downstream from the "i" for-loop (e.g., if the call to mlx4_init_resource_tracker() fails). Therefore, we DO require the pre-decrement format. Therefore, the one-line fix proposed by Yishai is the correct fix. >. I don't really > care either way, but git grep says that 'while (i--)' is 5 times more > common than 'while (--i >= 0)'. Not relevant, while (i--) is simply not correct, because of the case where the for-loop involving i completes successfully and an error occurs later. FYI, you also had another bug in your solution -- a double-free when kzalloc for port 2 fails. For your code, you should also have reset s_state->vlan_filter[port] to NULL as shown below: for (port = 1; port <= MLX4_MAX_PORTS; port++) { struct mlx4_vport_state *admin_vport; struct mlx4_vport_state *oper_vport; s_state->vlan_filter[port] = kzalloc(sizeof(struct mlx4_vlan_fltr), GFP_KERNEL); if (!s_state->vlan_filter[port]) { if (--port) { kfree(s_state->vlan_filter[port]); ==> You should have added this s_state->vlan_filter[port] = NULL; } goto err_slaves; } However, again, the correct solution is to do what Yishai suggests: while (--i >= 0) so that if i is already zero the while-loop will not be entered. -Jack > > Rasmus > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > in the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2016-02-11 9:29 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-09 20:11 [PATCH 0/5] pre-decrement in error paths considered harmful Rasmus Villemoes 2016-02-09 20:11 ` Rasmus Villemoes 2016-02-09 20:11 ` [PATCH 1/5] drm/gma500: fix error path in gma_intel_setup_gmbus() Rasmus Villemoes 2016-02-10 6:41 ` Andy Shevchenko 2016-02-10 6:41 ` Andy Shevchenko 2016-02-10 7:26 ` Daniel Vetter 2016-02-10 7:26 ` Daniel Vetter 2016-02-09 20:11 ` [PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus() Rasmus Villemoes 2016-02-09 20:11 ` Rasmus Villemoes 2016-02-09 20:27 ` Jani Nikula 2016-02-09 20:27 ` Jani Nikula 2016-02-10 8:56 ` Jani Nikula 2016-02-10 8:56 ` Jani Nikula 2016-02-09 20:11 ` [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init() Rasmus Villemoes 2016-02-10 9:40 ` Yishai Hadas 2016-02-10 18:15 ` Rasmus Villemoes 2016-02-10 18:15 ` Rasmus Villemoes 2016-02-11 9:29 ` Jack Morgenstein [this message] 2016-02-11 9:29 ` Jack Morgenstein 2016-02-11 10:20 ` Jack Morgenstein 2016-02-11 16:02 ` Doug Ledford 2016-02-09 20:11 ` [PATCH 4/5] net: sxgbe: fix error paths in sxgbe_platform_probe() Rasmus Villemoes 2016-03-08 20:44 ` Rasmus Villemoes 2016-03-22 19:47 ` Rasmus Villemoes 2016-03-26 21:24 ` [PATCH] " Rasmus Villemoes 2016-03-27 8:22 ` Francois Romieu 2016-03-27 21:40 ` Rasmus Villemoes 2016-03-28 2:39 ` David Miller 2016-03-28 2:40 ` David Miller 2016-02-09 20:11 ` [PATCH 5/5] mm/backing-dev.c: fix error path in wb_init() Rasmus Villemoes 2016-02-09 20:11 ` Rasmus Villemoes
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=20160211112943.20c0de19@jpm-OptiPlex-GX620 \ --to=jackm@dev.mellanox.co.il \ --cc=jackm@mellanox.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rdma@vger.kernel.org \ --cc=linux@rasmusvillemoes.dk \ --cc=majd@mellanox.com \ --cc=netdev@vger.kernel.org \ --cc=yishaih@dev.mellanox.co.il \ --cc=yishaih@mellanox.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: linkBe 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.