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

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

[Arnd Bergmann]

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

[PATCH 3/5] rxrpc: fix last_call processing

[Arnd Bergmann]

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

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

[David Howells]

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

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

[David Howells]

This is fixed by:

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

which is in net-next.

David

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

[Arnd Bergmann]

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

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

[David Howells]

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

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

[Arnd Bergmann]

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

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

[David Miller]

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.

[PATCH 5/5] net/xgene: fix error handling during reset

[Arnd Bergmann]

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

Re: [PATCH 5/5] net/xgene: fix error handling during reset

[David Miller]

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.