linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next
@ 2016-08-26 15:25 Arnd Bergmann
  2016-08-26 15:25 ` [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-26 15:25 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Arnd Bergmann, Alexandre Courbot, David Howells,
	David S. Miller, Fushen Chen, Hadar Hen Zion,
	Iyappan Subramanian, Jiri Pirko, Keyur Chudgar, Linus Walleij,
	Phil Reid, Russell King, Tomi Valkeinen, linux-fbdev, linux-gpio,
	netdev

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/5] gpio: pca954x: fix undefined error code from remove
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
@ 2016-08-26 15:25 ` Arnd Bergmann
  2016-08-26 15:47   ` Phil Reid
  2016-09-07 14:15   ` Linus Walleij
  2016-08-26 15:25 ` [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic Arnd Bergmann
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-26 15:25 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Arnd Bergmann, Phil Reid, Linus Walleij,
	Alexandre Courbot, linux-gpio

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
  2016-08-26 15:25 ` [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann
@ 2016-08-26 15:25 ` Arnd Bergmann
  2016-08-29 13:18   ` Linus Walleij
  2016-08-26 15:25 ` [PATCH 3/5] rxrpc: fix last_call processing Arnd Bergmann
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-26 15:25 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Arnd Bergmann, Linus Walleij, linux-fbdev,
	Russell King, Tomi Valkeinen

The addition of the Nomadik support in this driver introduced
a bug in clcdfb_of_init_display(), which now calls init_panel
with an uninitialized 'endpoint' pointer, as "gcc -Wmaybe-uninitialized"
warns:

drivers/video/fbdev/amba-clcd.c: In function 'clcdfb_of_init_display':
drivers/video/fbdev/amba-clcd.c:785:5: error: 'endpoint' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This reverts the broken part of the function to what it was before
the patch, which is the best guess I have to what it should be.
I assume this was left over from an attempted rework of the
code that was partially backed out.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 046ad6cdeb3f ("video: ARM CLCD: support Nomadik variant")
Cc: Linus Walleij <linus.walleij@linaro.org>
---
Cc: linux-fbdev@vger.kernel.org
Cc: Russell King <linux@armlinux.org.uk>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
 drivers/video/fbdev/amba-clcd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index c342ff370108..ec2671d98abc 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -782,12 +782,9 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
 	/*
 	 * Fetch the panel endpoint.
 	 */
-	if (!endpoint) {
-		endpoint = of_graph_get_next_endpoint(fb->dev->dev.of_node,
-						      NULL);
-		if (!endpoint)
-			return -ENODEV;
-	}
+	endpoint = of_graph_get_next_endpoint(fb->dev->dev.of_node, NULL);
+	if (!endpoint)
+		return -ENODEV;
 
 	if (fb->vendor->init_panel) {
 		err = fb->vendor->init_panel(fb, endpoint);
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/5] rxrpc: fix last_call processing
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
  2016-08-26 15:25 ` [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann
  2016-08-26 15:25 ` [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic Arnd Bergmann
@ 2016-08-26 15:25 ` Arnd Bergmann
  2016-08-26 15:25 ` [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower Arnd Bergmann
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-26 15:25 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Arnd Bergmann, David Howells, David S. Miller, netdev

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
                   ` (2 preceding siblings ...)
  2016-08-26 15:25 ` [PATCH 3/5] rxrpc: fix last_call processing Arnd Bergmann
@ 2016-08-26 15:25 ` Arnd Bergmann
  2016-08-29  4:30   ` David Miller
  2016-08-26 15:25 ` [PATCH 5/5] net/xgene: fix error handling during reset Arnd Bergmann
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-26 15:25 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Arnd Bergmann, Hadar Hen Zion, Jiri Pirko,
	David S . Miller, netdev

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/5] net/xgene: fix error handling during reset
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
                   ` (3 preceding siblings ...)
  2016-08-26 15:25 ` [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower Arnd Bergmann
@ 2016-08-26 15:25 ` Arnd Bergmann
  2016-08-29  4:31   ` David Miller
  2016-08-27  7:01 ` [PATCH 3/5] rxrpc: fix last_call processing David Howells
  2016-08-28  8:42 ` David Howells
  6 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-26 15:25 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Arnd Bergmann, Fushen Chen, Iyappan Subramanian,
	Keyur Chudgar, David S. Miller, netdev

The newly added reset logic uses helper functions for the MMIO that
may fail. However, when the read operation fails, we end up writing
back uninitialized data to the register, as gcc warns:

drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c: In function 'xgene_enet_link_state':
drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c:213:2: error: 'data' may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c:209:6: note: 'data' was declared here
  u32 data;

We already print a warning to the console log if that happens,
the best alternative that I can see is skip the rest of the reset
sequence if the register value cannot be read: Most likely the
write would fail as well, and if it succeeded, worse things could
happen.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 3eb7cb9dc946 ("drivers: net: xgene: XFI PCS reset when link is down")
Cc: Fushen Chen <fchen@apm.com> 
---
Cc: Iyappan Subramanian <isubramanian@apm.com>
Cc: Keyur Chudgar <kchudgar@apm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org

 drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c b/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c
index d672e71b5a50..279ee27004f7 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c
@@ -155,19 +155,23 @@ static void xgene_enet_rd_mac(struct xgene_enet_pdata *pdata,
 			   rd_addr);
 }
 
-static void xgene_enet_rd_pcs(struct xgene_enet_pdata *pdata,
+static bool xgene_enet_rd_pcs(struct xgene_enet_pdata *pdata,
 			      u32 rd_addr, u32 *rd_data)
 {
 	void __iomem *addr, *rd, *cmd, *cmd_done;
+	bool success;
 
 	addr = pdata->pcs_addr + PCS_ADDR_REG_OFFSET;
 	rd = pdata->pcs_addr + PCS_READ_REG_OFFSET;
 	cmd = pdata->pcs_addr + PCS_COMMAND_REG_OFFSET;
 	cmd_done = pdata->pcs_addr + PCS_COMMAND_DONE_REG_OFFSET;
 
-	if (!xgene_enet_rd_indirect(addr, rd, cmd, cmd_done, rd_addr, rd_data))
+	success = xgene_enet_rd_indirect(addr, rd, cmd, cmd_done, rd_addr, rd_data);
+	if (!success)
 		netdev_err(pdata->ndev, "PCS read failed, addr: %04x\n",
 			   rd_addr);
+
+	return success;
 }
 
 static int xgene_enet_ecc_init(struct xgene_enet_pdata *pdata)
@@ -208,7 +212,9 @@ static void xgene_pcs_reset(struct xgene_enet_pdata *pdata)
 {
 	u32 data;
 
-	xgene_enet_rd_pcs(pdata, PCS_CONTROL_1, &data);
+	if (!xgene_enet_rd_pcs(pdata, PCS_CONTROL_1, &data))
+		return;
+
 	xgene_enet_wr_pcs(pdata, PCS_CONTROL_1, data | PCS_CTRL_PCS_RST);
 	xgene_enet_wr_pcs(pdata, PCS_CONTROL_1, data & ~PCS_CTRL_PCS_RST);
 }
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] gpio: pca954x: fix undefined error code from remove
  2016-08-26 15:25 ` [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann
@ 2016-08-26 15:47   ` Phil Reid
  2016-09-07 14:15   ` Linus Walleij
  1 sibling, 0 replies; 17+ messages in thread
From: Phil Reid @ 2016-08-26 15:47 UTC (permalink / raw)
  To: Arnd Bergmann, linux-kernel
  Cc: Linus Torvalds, Linus Walleij, Alexandre Courbot, linux-gpio

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] rxrpc: fix last_call processing
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
                   ` (4 preceding siblings ...)
  2016-08-26 15:25 ` [PATCH 5/5] net/xgene: fix error handling during reset Arnd Bergmann
@ 2016-08-27  7:01 ` David Howells
  2016-08-28  8:42 ` David Howells
  6 siblings, 0 replies; 17+ messages in thread
From: David Howells @ 2016-08-27  7:01 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: dhowells, linux-kernel, Linus Torvalds, David S. Miller, netdev

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] rxrpc: fix last_call processing
  2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
                   ` (5 preceding siblings ...)
  2016-08-27  7:01 ` [PATCH 3/5] rxrpc: fix last_call processing David Howells
@ 2016-08-28  8:42 ` David Howells
  2016-08-31 11:53   ` Arnd Bergmann
  2016-08-31 20:58   ` David Howells
  6 siblings, 2 replies; 17+ messages in thread
From: David Howells @ 2016-08-28  8:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: dhowells, linux-kernel, Linus Torvalds, David S. Miller, netdev

This is fixed by:

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

which is in net-next.

David

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower
  2016-08-26 15:25 ` [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower Arnd Bergmann
@ 2016-08-29  4:30   ` David Miller
  0 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2016-08-29  4:30 UTC (permalink / raw)
  To: arnd; +Cc: linux-kernel, torvalds, hadarh, jiri, netdev

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/5] net/xgene: fix error handling during reset
  2016-08-26 15:25 ` [PATCH 5/5] net/xgene: fix error handling during reset Arnd Bergmann
@ 2016-08-29  4:31   ` David Miller
  0 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2016-08-29  4:31 UTC (permalink / raw)
  To: arnd; +Cc: linux-kernel, torvalds, fchen, isubramanian, kchudgar, netdev

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

> The newly added reset logic uses helper functions for the MMIO that
> may fail. However, when the read operation fails, we end up writing
> back uninitialized data to the register, as gcc warns:
> 
> drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c: In function 'xgene_enet_link_state':
> drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c:213:2: error: 'data' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c:209:6: note: 'data' was declared here
>   u32 data;
> 
> We already print a warning to the console log if that happens,
> the best alternative that I can see is skip the rest of the reset
> sequence if the register value cannot be read: Most likely the
> write would fail as well, and if it succeeded, worse things could
> happen.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 3eb7cb9dc946 ("drivers: net: xgene: XFI PCS reset when link is down")

Applied.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic
  2016-08-26 15:25 ` [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic Arnd Bergmann
@ 2016-08-29 13:18   ` Linus Walleij
  2016-08-29 13:41     ` Arnd Bergmann
  2016-08-30  8:35     ` Tomi Valkeinen
  0 siblings, 2 replies; 17+ messages in thread
From: Linus Walleij @ 2016-08-29 13:18 UTC (permalink / raw)
  To: Arnd Bergmann, Colin Ian King
  Cc: linux-kernel, linux-fbdev, Russell King, Tomi Valkeinen

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

[I don't see why Torvalds was CC'ed on this patch? Was there
some specific complaint from his side that I screw things up
or just the wrong Linus?]

> The addition of the Nomadik support in this driver introduced
> a bug in clcdfb_of_init_display(), which now calls init_panel
> with an uninitialized 'endpoint' pointer, as "gcc -Wmaybe-uninitialized"
> warns:
>
> drivers/video/fbdev/amba-clcd.c: In function 'clcdfb_of_init_display':
> drivers/video/fbdev/amba-clcd.c:785:5: error: 'endpoint' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This reverts the broken part of the function to what it was before
> the patch, which is the best guess I have to what it should be.
> I assume this was left over from an attempted rework of the
> code that was partially backed out.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 046ad6cdeb3f ("video: ARM CLCD: support Nomadik variant")
> Cc: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Tomi: I think this bug was also reported by Ian King, so suggest
adding his
Reported-by: Colin Ian King <colin.king@canonical.com>

When applying.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic
  2016-08-29 13:18   ` Linus Walleij
@ 2016-08-29 13:41     ` Arnd Bergmann
  2016-08-30  8:35     ` Tomi Valkeinen
  1 sibling, 0 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-29 13:41 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Colin Ian King, linux-kernel, linux-fbdev, Russell King, Tomi Valkeinen

On Monday 29 August 2016, Linus Walleij wrote:
> On Fri, Aug 26, 2016 at 5:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> [I don't see why Torvalds was CC'ed on this patch? Was there
> some specific complaint from his side that I screw things up
> or just the wrong Linus?]

It was intentional: as mentioned in the introductory mail, he did
the patch that prevented you from seeing the five warnings that I got
after reverting 6e8d666e9253.

	Arnd

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic
  2016-08-29 13:18   ` Linus Walleij
  2016-08-29 13:41     ` Arnd Bergmann
@ 2016-08-30  8:35     ` Tomi Valkeinen
  1 sibling, 0 replies; 17+ messages in thread
From: Tomi Valkeinen @ 2016-08-30  8:35 UTC (permalink / raw)
  To: Linus Walleij, Arnd Bergmann, Colin Ian King
  Cc: linux-kernel, linux-fbdev, Russell King


[-- Attachment #1.1: Type: text/plain, Size: 1380 bytes --]


On 29/08/16 16:18, Linus Walleij wrote:
> On Fri, Aug 26, 2016 at 5:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> [I don't see why Torvalds was CC'ed on this patch? Was there
> some specific complaint from his side that I screw things up
> or just the wrong Linus?]
> 
>> The addition of the Nomadik support in this driver introduced
>> a bug in clcdfb_of_init_display(), which now calls init_panel
>> with an uninitialized 'endpoint' pointer, as "gcc -Wmaybe-uninitialized"
>> warns:
>>
>> drivers/video/fbdev/amba-clcd.c: In function 'clcdfb_of_init_display':
>> drivers/video/fbdev/amba-clcd.c:785:5: error: 'endpoint' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> This reverts the broken part of the function to what it was before
>> the patch, which is the best guess I have to what it should be.
>> I assume this was left over from an attempted rework of the
>> code that was partially backed out.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Fixes: 046ad6cdeb3f ("video: ARM CLCD: support Nomadik variant")
>> Cc: Linus Walleij <linus.walleij@linaro.org>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Tomi: I think this bug was also reported by Ian King, so suggest
> adding his
> Reported-by: Colin Ian King <colin.king@canonical.com>
> 

Thanks, queued for v4.9.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] rxrpc: fix last_call processing
  2016-08-28  8:42 ` David Howells
@ 2016-08-31 11:53   ` Arnd Bergmann
  2016-08-31 20:58   ` David Howells
  1 sibling, 0 replies; 17+ messages in thread
From: Arnd Bergmann @ 2016-08-31 11:53 UTC (permalink / raw)
  To: David Howells; +Cc: linux-kernel, Linus Torvalds, David S. Miller, netdev

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] rxrpc: fix last_call processing
  2016-08-28  8:42 ` David Howells
  2016-08-31 11:53   ` Arnd Bergmann
@ 2016-08-31 20:58   ` David Howells
  1 sibling, 0 replies; 17+ messages in thread
From: David Howells @ 2016-08-31 20:58 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: dhowells, linux-kernel, Linus Torvalds, David S. Miller, netdev

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] gpio: pca954x: fix undefined error code from remove
  2016-08-26 15:25 ` [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann
  2016-08-26 15:47   ` Phil Reid
@ 2016-09-07 14:15   ` Linus Walleij
  1 sibling, 0 replies; 17+ messages in thread
From: Linus Walleij @ 2016-09-07 14:15 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-kernel, Linus Torvalds, Phil Reid, Alexandre Courbot, linux-gpio

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-09-07 14:15 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-26 15:25 [PATCH 0/5] -Wmaybe-uninitialized bug fixes for linux-next Arnd Bergmann
2016-08-26 15:25 ` [PATCH 1/5] gpio: pca954x: fix undefined error code from remove Arnd Bergmann
2016-08-26 15:47   ` Phil Reid
2016-09-07 14:15   ` Linus Walleij
2016-08-26 15:25 ` [PATCH 2/5] video: ARM CLCD: fix endpoint lookup logic Arnd Bergmann
2016-08-29 13:18   ` Linus Walleij
2016-08-29 13:41     ` Arnd Bergmann
2016-08-30  8:35     ` Tomi Valkeinen
2016-08-26 15:25 ` [PATCH 3/5] rxrpc: fix last_call processing Arnd Bergmann
2016-08-26 15:25 ` [PATCH 4/5] net_sched: fix use of uninitialized ethertype variable in cls_flower Arnd Bergmann
2016-08-29  4:30   ` David Miller
2016-08-26 15:25 ` [PATCH 5/5] net/xgene: fix error handling during reset Arnd Bergmann
2016-08-29  4:31   ` David Miller
2016-08-27  7:01 ` [PATCH 3/5] rxrpc: fix last_call processing David Howells
2016-08-28  8:42 ` David Howells
2016-08-31 11:53   ` Arnd Bergmann
2016-08-31 20:58   ` David Howells

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).