Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470820 > unrolled thread

[PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann <arnd@arndb.de> - 2016-08-26 17:40 +0200
    [PATCH 3/5] rxrpc: fix last_call processing Arnd Bergmann <arnd@arndb.de> - 2016-08-26 17:40 +0200
      Re: [PATCH 3/5] rxrpc: fix last_call processing David Howells <dhowells@redhat.com> - 2016-08-27 09:10 +0200
      Re: [PATCH 3/5] rxrpc: fix last_call processing David Howells <dhowells@redhat.com> - 2016-08-28 10:50 +0200
        Re: [PATCH 3/5] rxrpc: fix last_call processing Arnd Bergmann <arnd@arndb.de> - 2016-08-31 14:00 +0200
          Re: [PATCH 3/5] rxrpc: fix last_call processing David Howells <dhowells@redhat.com> - 2016-08-31 23:00 +0200
    [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann <arnd@arndb.de> - 2016-08-26 17:40 +0200
      Re: [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Phil Reid <preid@electromag.com.au> - 2016-08-26 17:50 +0200
      Re: [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Linus Walleij <linus.walleij@linaro.org> - 2016-09-07 16:20 +0200
    [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower Arnd Bergmann <arnd@arndb.de> - 2016-08-26 17:40 +0200
      Re: [PATCH 4/5] net_sched: fix use of uninitialized ethertype  variable in cls_flower David Miller <davem@davemloft.net> - 2016-08-29 06:40 +0200

#1470820 — [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next

In 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally"),
Linus wrote:

    Looking at the warnings produced, every single one I looked at was a
    false positive, and the warnings are frequent enough (and big enough)
    that they can easily hide real problems that you don't notice in
    the noise generated by -Wmaybe-uninitialized.

Today, I tried reverting the patch on linux-next and built an ARM
allmodconfig kernel on ARM along with some randconfig kernels,
and got a handful of warnings, all of which appear to be reasonable
and point to actual mistakes in the code. The difference to what
Linus saw must be that previously the useful warnings were more
likely to get fixed before making it into the kernel, while now
we have to find them the hard way.

These five patches address all new warnings. In some cases this
may not be the correct fix, so please review carefully before applying,
or suggest a better fix. No need to keep them as a series, I
just group them here for the sake of discussion. Please pick up
whatever looks right to you.

Obviously, this kind of warnings always produces some false positives
(see https://gcc.gnu.org/wiki/Better_Uninitialized_Warnings), but I still
hope to get a better balance with enabling them sometimes where
people want them, as the current approach of always enabling them
for "make W=1" but never by default seems suboptimal: We had previously
identified a number of options (CONFIG_CC_OPTIMIZE_FOR_SIZE,
CONFIG_PROFILE_ALL_BRANCHES, CONFIG_UBSAN_ALIGNMENT, and
CONFIG_GCOV_PROFILE_ALL) that cause tons of false positives,
but without those options (and avoiding gcc-4.8 or lower),
we typically get mostly reports for actual bugs in my experience.

I can continue running the tests and send patches, but it feels
like a waste of time when they should have been found by the
original developers. Any other suggestions?

	Arnd

Arnd Bergmann (5):
  gpio: pca954x: fix undefined error code from remove
  video: ARM CLCD: fix endpoint lookup logic
  rxrpc: fix last_call processing
  net_sched: fix use of uninitialized ethertype variable in cls_flower
  net/xgene: fix error handling during reset

 drivers/gpio/gpio-pca953x.c                       |  2 ++
 drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c | 12 +++++++++---
 drivers/video/fbdev/amba-clcd.c                   |  9 +++------
 net/rxrpc/input.c                                 |  8 ++++----
 net/sched/cls_flower.c                            | 21 +++++++++++----------
 5 files changed, 29 insertions(+), 23 deletions(-)

Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: David Howells <dhowells@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Fushen Chen <fchen@apm.com> 
Cc: Hadar Hen Zion <hadarh@mellanox.com>
Cc: Iyappan Subramanian <isubramanian@apm.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Keyur Chudgar <kchudgar@apm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Phil Reid <preid@electromag.com.au>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-gpio@vger.kernel.org
Cc: netdev@vger.kernel.org


-- 
2.9.0

[toc] | [next] | [standalone]

#1470821 — [PATCH 3/5] rxrpc: fix last_call processing

A change to the retransmission handling in rxrpc caused a use-before-init
bug in rxrpc_data_ready(), as indicated by "gcc -Wmaybe-uninitialized":

net/rxrpc/input.c: In function 'rxrpc_data_ready':
net/rxrpc/input.c:735:34: error: 'call' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This moves the initialization of the local variable before the first
user, which presumably is what was intended here.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 18bfeba50dfd ("rxrpc: Perform terminal call ACK/ABORT retransmission from conn processor")
---
Cc: David Howells <dhowells@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org

 net/rxrpc/input.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 66cdeb56f44f..3c22e43a58fd 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -728,6 +728,10 @@ void rxrpc_data_ready(struct sock *sk)
 		if (sp->hdr.callNumber < chan->last_call)
 			goto discard_unlock;
 
+		call = rcu_dereference(chan->call);
+		if (!call || atomic_read(&call->usage) == 0)
+			goto cant_route_call;
+
 		if (sp->hdr.callNumber == chan->last_call) {
 			/* For the previous service call, if completed
 			 * successfully, we discard all further packets.
@@ -744,10 +748,6 @@ void rxrpc_data_ready(struct sock *sk)
 			goto out_unlock;
 		}
 
-		call = rcu_dereference(chan->call);
-		if (!call || atomic_read(&call->usage) == 0)
-			goto cant_route_call;
-
 		rxrpc_post_packet_to_call(call, skb);
 		goto out_unlock;
 	}
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1471113 — Re: [PATCH 3/5] rxrpc: fix last_call processing

Arnd Bergmann <arnd@arndb.de> wrote:

> A change to the retransmission handling in rxrpc caused a use-before-init
> bug in rxrpc_data_ready(), as indicated by "gcc -Wmaybe-uninitialized":
> 
> net/rxrpc/input.c: In function 'rxrpc_data_ready':
> net/rxrpc/input.c:735:34: error: 'call' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This moves the initialization of the local variable before the first
> user, which presumably is what was intended here.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 18bfeba50dfd ("rxrpc: Perform terminal call ACK/ABORT retransmission from conn processor")
> ---
> Cc: David Howells <dhowells@redhat.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> 
>  net/rxrpc/input.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 66cdeb56f44f..3c22e43a58fd 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -728,6 +728,10 @@ void rxrpc_data_ready(struct sock *sk)
>  		if (sp->hdr.callNumber < chan->last_call)
>  			goto discard_unlock;
>  
> +		call = rcu_dereference(chan->call);
> +		if (!call || atomic_read(&call->usage) == 0)
> +			goto cant_route_call;
> +
>  		if (sp->hdr.callNumber == chan->last_call) {
>  			/* For the previous service call, if completed
>  			 * successfully, we discard all further packets.
> @@ -744,10 +748,6 @@ void rxrpc_data_ready(struct sock *sk)
>  			goto out_unlock;
>  		}
>  
> -		call = rcu_dereference(chan->call);
> -		if (!call || atomic_read(&call->usage) == 0)
> -			goto cant_route_call;
> -
>  		rxrpc_post_packet_to_call(call, skb);
>  		goto out_unlock;
>  	}

You can't rearrange these like this.  I have a different fix.

David

[toc] | [prev] | [next] | [standalone]

#1471312 — Re: [PATCH 3/5] rxrpc: fix last_call processing

This is fixed by:

	commit 2266ffdef5737fdfa96005204fc5606dbd559956
	subject: rxrpc: Fix conn-based retransmit

which is in net-next.

David

[toc] | [prev] | [next] | [standalone]

#1473376 — Re: [PATCH 3/5] rxrpc: fix last_call processing

On Sunday, August 28, 2016 9:42:17 AM CEST David Howells wrote:
> This is fixed by:
> 
>         commit 2266ffdef5737fdfa96005204fc5606dbd559956
>         subject: rxrpc: Fix conn-based retransmit
> 
> which is in net-next.

I've merged net-next into the last linux-next release now for
testing (no linux-next this week) and can confirm that your
fix is correct. However, I got a new (valid) warning after
your f5c17aaeb2ae ("rxrpc: Calls should only have one terminal
state"), and another (false-positive) one for another patch
in net-next.

I'll follow up with the fixes, both of which are rather
straightforward.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1473867 — Re: [PATCH 3/5] rxrpc: fix last_call processing

Arnd Bergmann <arnd@arndb.de> wrote:

> I'll follow up with the fixes, both of which are rather
> straightforward.

Are they both in?

	[PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

David

[toc] | [prev] | [next] | [standalone]

#1470824 — [PATCH 1/5] gpio: pca954x: fix undefined error code from remove

The recent addition of the regulator support has led to the pca953x_remove
function returning uninitialized data when no platform data pointer is
provided, as gcc warns when using -Wmaybe-uninitialized:

drivers/gpio/gpio-pca953x.c: In function 'pca953x_remove':
drivers/gpio/gpio-pca953x.c:860:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This restores the previous behavior, returning 0 on success.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: e23efa311110 ("gpio: pca954x: Add vcc regulator and enable it")
Cc: Phil Reid <preid@electromag.com.au>
---
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org

 drivers/gpio/gpio-pca953x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index cbe2824461eb..b9d31d737dbf 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -853,6 +853,8 @@ static int pca953x_remove(struct i2c_client *client)
 		if (ret < 0)
 			dev_err(&client->dev, "%s failed, %d\n",
 					"teardown", ret);
+	} else {
+		ret = 0;
 	}
 
 	regulator_disable(chip->regulator);
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1470849 — Re: [PATCH 1/5] gpio: pca954x: fix undefined error code from remove

On 26/08/2016 23:25, Arnd Bergmann wrote:
> The recent addition of the regulator support has led to the pca953x_remove
> function returning uninitialized data when no platform data pointer is
> provided, as gcc warns when using -Wmaybe-uninitialized:
>
> drivers/gpio/gpio-pca953x.c: In function 'pca953x_remove':
> drivers/gpio/gpio-pca953x.c:860:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This restores the previous behavior, returning 0 on success.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: e23efa311110 ("gpio: pca954x: Add vcc regulator and enable it")
> Cc: Phil Reid <preid@electromag.com.au>
> ---
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org
>
>  drivers/gpio/gpio-pca953x.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index cbe2824461eb..b9d31d737dbf 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -853,6 +853,8 @@ static int pca953x_remove(struct i2c_client *client)
>  		if (ret < 0)
>  			dev_err(&client->dev, "%s failed, %d\n",
>  					"teardown", ret);
> +	} else {
> +		ret = 0;
>  	}
>
>  	regulator_disable(chip->regulator);
>
Ahh, commit 8c7a92dad1621f38d1ff4fe9eaac898d6f33a0a3 gpio: pca953x: remove redundant assignments
removed the 'redundant' initialisation of ret in this function.
Looks like I did not have this commit in my tree when I submitted did the regulator patch.
Sorry about that. Alternative to the 'else' is to add init at definition.

Acked-by: Phil Reid <preid@electromag.com.au>


-- 
Regards
Phil Reid

[toc] | [prev] | [next] | [standalone]

#1478331 — Re: [PATCH 1/5] gpio: pca954x: fix undefined error code from remove

On Fri, Aug 26, 2016 at 5:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> The recent addition of the regulator support has led to the pca953x_remove
> function returning uninitialized data when no platform data pointer is
> provided, as gcc warns when using -Wmaybe-uninitialized:
>
> drivers/gpio/gpio-pca953x.c: In function 'pca953x_remove':
> drivers/gpio/gpio-pca953x.c:860:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This restores the previous behavior, returning 0 on success.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: e23efa311110 ("gpio: pca954x: Add vcc regulator and enable it")
> Cc: Phil Reid <preid@electromag.com.au>
> ---
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org

Patch applied with Phil's ACK.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1470827 — [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower

The addition of VLAN support caused a possible use of uninitialized
data if we encounter a zero TCA_FLOWER_KEY_ETH_TYPE key, as pointed
out by "gcc -Wmaybe-uninitialized":

net/sched/cls_flower.c: In function 'fl_change':
net/sched/cls_flower.c:366:22: error: 'ethertype' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This changes the code to only set the ethertype field if it
was nonzero, as before the patch.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 9399ae9a6cb2 ("net_sched: flower: Add vlan support")
Cc: Hadar Hen Zion <hadarh@mellanox.com>
Cc: Jiri Pirko <jiri@mellanox.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
 net/sched/cls_flower.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 532ab6751343..cf9ad5b50889 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -353,18 +353,19 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		       mask->eth.src, TCA_FLOWER_KEY_ETH_SRC_MASK,
 		       sizeof(key->eth.src));
 
-	if (tb[TCA_FLOWER_KEY_ETH_TYPE])
+	if (tb[TCA_FLOWER_KEY_ETH_TYPE]) {
 		ethertype = nla_get_be16(tb[TCA_FLOWER_KEY_ETH_TYPE]);
 
-	if (ethertype == htons(ETH_P_8021Q)) {
-		fl_set_key_vlan(tb, &key->vlan, &mask->vlan);
-		fl_set_key_val(tb, &key->basic.n_proto,
-			       TCA_FLOWER_KEY_VLAN_ETH_TYPE,
-			       &mask->basic.n_proto, TCA_FLOWER_UNSPEC,
-			       sizeof(key->basic.n_proto));
-	} else {
-		key->basic.n_proto = ethertype;
-		mask->basic.n_proto = cpu_to_be16(~0);
+		if (ethertype == htons(ETH_P_8021Q)) {
+			fl_set_key_vlan(tb, &key->vlan, &mask->vlan);
+			fl_set_key_val(tb, &key->basic.n_proto,
+				       TCA_FLOWER_KEY_VLAN_ETH_TYPE,
+				       &mask->basic.n_proto, TCA_FLOWER_UNSPEC,
+				       sizeof(key->basic.n_proto));
+		} else {
+			key->basic.n_proto = ethertype;
+			mask->basic.n_proto = cpu_to_be16(~0);
+		}
 	}
 
 	if (key->basic.n_proto == htons(ETH_P_IP) ||
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1471536 — Re: [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower

From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 26 Aug 2016 17:25:45 +0200

> The addition of VLAN support caused a possible use of uninitialized
> data if we encounter a zero TCA_FLOWER_KEY_ETH_TYPE key, as pointed
> out by "gcc -Wmaybe-uninitialized":
> 
> net/sched/cls_flower.c: In function 'fl_change':
> net/sched/cls_flower.c:366:22: error: 'ethertype' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This changes the code to only set the ethertype field if it
> was nonzero, as before the patch.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 9399ae9a6cb2 ("net_sched: flower: Add vlan support")

Applied.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web