* [PATCH] [v2] myri10ge: avoid uninitialized variable use
@ 2025-06-21 19:35 Arnd Bergmann
2025-06-22 16:27 ` Simon Horman
0 siblings, 1 reply; 2+ messages in thread
From: Arnd Bergmann @ 2025-06-21 19:35 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Simon Horman, Arnd Bergmann, Ingo Molnar, Thomas Gleixner, netdev,
linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
While compile testing on less common architectures, I noticed that gcc-10 on
s390 finds a bug that all other configurations seem to miss:
drivers/net/ethernet/myricom/myri10ge/myri10ge.c: In function 'myri10ge_set_multicast_list':
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:391:25: error: 'cmd.data0' is used uninitialized in this function [-Werror=uninitialized]
391 | buf->data0 = htonl(data->data0);
| ^~
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:392:25: error: '*((void *)&cmd+4)' is used uninitialized in this function [-Werror=uninitialized]
392 | buf->data1 = htonl(data->data1);
| ^~
drivers/net/ethernet/myricom/myri10ge/myri10ge.c: In function 'myri10ge_allocate_rings':
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:392:13: error: 'cmd.data1' is used uninitialized in this function [-Werror=uninitialized]
392 | buf->data1 = htonl(data->data1);
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:1939:22: note: 'cmd.data1' was declared here
1939 | struct myri10ge_cmd cmd;
| ^~~
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:393:13: error: 'cmd.data2' is used uninitialized in this function [-Werror=uninitialized]
393 | buf->data2 = htonl(data->data2);
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:1939:22: note: 'cmd.data2' was declared here
1939 | struct myri10ge_cmd cmd;
It would be nice to understand how to make other compilers catch this as
well, but for the moment I'll just shut up the warning by fixing the
undefined behavior in this driver.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: initialize two more instances of these [Simon Horman]
---
.../net/ethernet/myricom/myri10ge/myri10ge.c | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index e611ff7fa3fa..4743064bc6d4 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -688,6 +688,9 @@ static int myri10ge_get_firmware_capabilities(struct myri10ge_priv *mgp)
/* probe for IPv6 TSO support */
mgp->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_TSO;
+ cmd.data0 = 0,
+ cmd.data1 = 0,
+ cmd.data2 = 0,
status = myri10ge_send_cmd(mgp, MXGEFW_CMD_GET_MAX_TSO6_HDR_SIZE,
&cmd, 0);
if (status == 0) {
@@ -806,6 +809,7 @@ static int myri10ge_update_mac_address(struct myri10ge_priv *mgp,
| (addr[2] << 8) | addr[3]);
cmd.data1 = ((addr[4] << 8) | (addr[5]));
+ cmd.data2 = 0;
status = myri10ge_send_cmd(mgp, MXGEFW_SET_MAC_ADDRESS, &cmd, 0);
return status;
@@ -817,6 +821,9 @@ static int myri10ge_change_pause(struct myri10ge_priv *mgp, int pause)
int status, ctl;
ctl = pause ? MXGEFW_ENABLE_FLOW_CONTROL : MXGEFW_DISABLE_FLOW_CONTROL;
+ cmd.data0 = 0,
+ cmd.data1 = 0,
+ cmd.data2 = 0,
status = myri10ge_send_cmd(mgp, ctl, &cmd, 0);
if (status) {
@@ -834,6 +841,9 @@ myri10ge_change_promisc(struct myri10ge_priv *mgp, int promisc, int atomic)
int status, ctl;
ctl = promisc ? MXGEFW_ENABLE_PROMISC : MXGEFW_DISABLE_PROMISC;
+ cmd.data0 = 0;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
status = myri10ge_send_cmd(mgp, ctl, &cmd, atomic);
if (status)
netdev_err(mgp->dev, "Failed to set promisc mode\n");
@@ -1946,6 +1956,8 @@ static int myri10ge_allocate_rings(struct myri10ge_slice_state *ss)
/* get ring sizes */
slice = ss - mgp->ss;
cmd.data0 = slice;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
status = myri10ge_send_cmd(mgp, MXGEFW_CMD_GET_SEND_RING_SIZE, &cmd, 0);
tx_ring_size = cmd.data0;
cmd.data0 = slice;
@@ -2238,6 +2250,8 @@ static int myri10ge_get_txrx(struct myri10ge_priv *mgp, int slice)
status = 0;
if (slice == 0 || (mgp->dev->real_num_tx_queues > 1)) {
cmd.data0 = slice;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
status = myri10ge_send_cmd(mgp, MXGEFW_CMD_GET_SEND_OFFSET,
&cmd, 0);
ss->tx.lanai = (struct mcp_kreq_ether_send __iomem *)
@@ -2312,6 +2326,7 @@ static int myri10ge_open(struct net_device *dev)
if (mgp->num_slices > 1) {
cmd.data0 = mgp->num_slices;
cmd.data1 = MXGEFW_SLICE_INTR_MODE_ONE_PER_SLICE;
+ cmd.data2 = 0;
if (mgp->dev->real_num_tx_queues > 1)
cmd.data1 |= MXGEFW_SLICE_ENABLE_MULTIPLE_TX_QUEUES;
status = myri10ge_send_cmd(mgp, MXGEFW_CMD_ENABLE_RSS_QUEUES,
@@ -2414,6 +2429,8 @@ static int myri10ge_open(struct net_device *dev)
/* now give firmware buffers sizes, and MTU */
cmd.data0 = dev->mtu + ETH_HLEN + VLAN_HLEN;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
status = myri10ge_send_cmd(mgp, MXGEFW_CMD_SET_MTU, &cmd, 0);
cmd.data0 = mgp->small_bytes;
status |=
@@ -2956,6 +2973,9 @@ static void myri10ge_set_multicast_list(struct net_device *dev)
/* Disable multicast filtering */
+ cmd.data0 = 0;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
err = myri10ge_send_cmd(mgp, MXGEFW_ENABLE_ALLMULTI, &cmd, 1);
if (err != 0) {
netdev_err(dev, "Failed MXGEFW_ENABLE_ALLMULTI, error status: %d\n",
--
2.39.5
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] [v2] myri10ge: avoid uninitialized variable use
2025-06-21 19:35 [PATCH] [v2] myri10ge: avoid uninitialized variable use Arnd Bergmann
@ 2025-06-22 16:27 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2025-06-22 16:27 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Arnd Bergmann, Ingo Molnar, Thomas Gleixner, netdev,
linux-kernel
On Sat, Jun 21, 2025 at 09:35:11PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> While compile testing on less common architectures, I noticed that gcc-10 on
> s390 finds a bug that all other configurations seem to miss:
>
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c: In function 'myri10ge_set_multicast_list':
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:391:25: error: 'cmd.data0' is used uninitialized in this function [-Werror=uninitialized]
> 391 | buf->data0 = htonl(data->data0);
> | ^~
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:392:25: error: '*((void *)&cmd+4)' is used uninitialized in this function [-Werror=uninitialized]
> 392 | buf->data1 = htonl(data->data1);
> | ^~
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c: In function 'myri10ge_allocate_rings':
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:392:13: error: 'cmd.data1' is used uninitialized in this function [-Werror=uninitialized]
> 392 | buf->data1 = htonl(data->data1);
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:1939:22: note: 'cmd.data1' was declared here
> 1939 | struct myri10ge_cmd cmd;
> | ^~~
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:393:13: error: 'cmd.data2' is used uninitialized in this function [-Werror=uninitialized]
> 393 | buf->data2 = htonl(data->data2);
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:1939:22: note: 'cmd.data2' was declared here
> 1939 | struct myri10ge_cmd cmd;
>
> It would be nice to understand how to make other compilers catch this as
> well, but for the moment I'll just shut up the warning by fixing the
> undefined behavior in this driver.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: initialize two more instances of these [Simon Horman]
Sorry, but looking at this again I found a few more.
diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index 4743064bc6d4..feda51e23958 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -2258,6 +2258,8 @@ static int myri10ge_get_txrx(struct myri10ge_priv *mgp, int slice)
(mgp->sram + cmd.data0);
}
cmd.data0 = slice;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
status |= myri10ge_send_cmd(mgp, MXGEFW_CMD_GET_SMALL_RX_OFFSET,
&cmd, 0);
ss->rx_small.lanai = (struct mcp_kreq_ether_recv __iomem *)
@@ -2489,7 +2491,6 @@ static int myri10ge_open(struct net_device *dev)
static int myri10ge_close(struct net_device *dev)
{
struct myri10ge_priv *mgp = netdev_priv(dev);
- struct myri10ge_cmd cmd;
int status, old_down_cnt;
int i;
@@ -2508,8 +2509,13 @@ static int myri10ge_close(struct net_device *dev)
netif_tx_stop_all_queues(dev);
if (mgp->rebooted == 0) {
+ struct myri10ge_cmd cmd;
+
old_down_cnt = mgp->down_cnt;
mb();
+ cmd.data0 = 0;
+ cmd.data1 = 0;
+ cmd.data2 = 0;
status =
myri10ge_send_cmd(mgp, MXGEFW_CMD_ETHERNET_DOWN, &cmd, 0);
if (status)
Even with this, I'm sure there are cases
where stale values are passed in cases like this.
But that seems out of scope for this patch.
/* setup cmd.data* */
myri10ge_send_cmd(...);
/* Something else */
myri10ge_send_cmd(..);
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-06-22 16:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-21 19:35 [PATCH] [v2] myri10ge: avoid uninitialized variable use Arnd Bergmann
2025-06-22 16:27 ` Simon Horman
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).