Re: linux-next: upstream tree build warnings

Re: linux-next: upstream tree build warnings

[David Miller]

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 23 Apr 2009 16:38:47 +1000

> Today's linux-next build (powerpc allyesconfig gcc4.4.0) produced these
> warnings:
> 
> drivers/net/virtio_net.c: In function 'virnet_vlan_rx_add_vid':
> include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> drivers/net/virtio_net.c:746: note: 'sg' was declared here
> drivers/net/virtio_net.c: In function 'virnet_vlan_rx_kill_vid':
> include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> drivers/net/virtio_net.c:758: note: 'sg' was declared here
> 
> in each case, sg is uninitialised when its address is passed to
> sg_set_buf() which passes it to gs_set_page() which passes it to
> sg_assign_page() which dereferences it (to use ->page_link).

I wonder if this is a side effect of changes that went in via Rusty's
tree?  I don't remember touching this driver in a while.

Re: linux-next: upstream tree build warnings

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

Hi Dave,

On Thu, 23 Apr 2009 00:24:03 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Thu, 23 Apr 2009 16:38:47 +1000
> 
> > Today's linux-next build (powerpc allyesconfig gcc4.4.0) produced these
> > warnings:
> > 
> > drivers/net/virtio_net.c: In function 'virnet_vlan_rx_add_vid':
> > include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> > drivers/net/virtio_net.c:746: note: 'sg' was declared here
> > drivers/net/virtio_net.c: In function 'virnet_vlan_rx_kill_vid':
> > include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> > drivers/net/virtio_net.c:758: note: 'sg' was declared here
> > 
> > in each case, sg is uninitialised when its address is passed to
> > sg_set_buf() which passes it to gs_set_page() which passes it to
> > sg_assign_page() which dereferences it (to use ->page_link).
> 
> I wonder if this is a side effect of changes that went in via Rusty's
> tree?  I don't remember touching this driver in a while.

The callers came in throught commit
0bde95690d65653e420d04856c5d5783155c747c ("virtio_net: Add support for
VLAN filtering in the hypervisor") which I assume came in throught the
net tree (it has you SOB on it). Committed Feb 5, 2009, went into Linus'
tree before 2.6.30-rc1.

scatterlist.h hasn't changed since July, 2008.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: upstream tree build warnings

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

Hi Dave,

On Thu, 23 Apr 2009 21:55:33 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> The callers came in throught commit
> 0bde95690d65653e420d04856c5d5783155c747c ("virtio_net: Add support for
> VLAN filtering in the hypervisor") which I assume came in throught the
> net tree (it has you SOB on it). Committed Feb 5, 2009, went into Linus'
> tree before 2.6.30-rc1.
> 
> scatterlist.h hasn't changed since July, 2008.

In other words, I think the bug has always been there and just never hit
(yet).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: upstream tree build warnings

[Alex Williamson]

On Thu, 2009-04-23 at 22:51 +1000, Stephen Rothwell wrote:
> Hi Dave,
> 
> On Thu, 23 Apr 2009 21:55:33 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > The callers came in throught commit
> > 0bde95690d65653e420d04856c5d5783155c747c ("virtio_net: Add support for
> > VLAN filtering in the hypervisor") which I assume came in throught the
> > net tree (it has you SOB on it). Committed Feb 5, 2009, went into Linus'
> > tree before 2.6.30-rc1.
> > 
> > scatterlist.h hasn't changed since July, 2008.
> 
> In other words, I think the bug has always been there and just never hit
> (yet).

Hmm, I can't seem to get the warning on an x86_64 allyesconfig build
with gcc-4.4.0.  Did all the archs other than powerpc miss this warning?
Thanks,

Alex

[PATCH] virtio_net: memset scatterlist before using

[Alex Williamson]

A powerpc gcc-4.4 build generates the following warnings:

drivers/net/virtio_net.c: In function 'virnet_vlan_rx_add_vid':
include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
drivers/net/virtio_net.c:746: note: 'sg' was declared here
drivers/net/virtio_net.c: In function 'virnet_vlan_rx_kill_vid':
include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
drivers/net/virtio_net.c:758: note: 'sg' was declared here

We need to clear the scatterlist before using it.  This sg entry will
get copied into another scatterlist before it's used, so memset it
rather than using sg_init_table or sg_init_one to avoid a bogus end
marker.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0718558..be1a9fe 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -745,6 +745,7 @@ static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg;
 
+	memset(&sg, 0, sizeof(sg));
 	sg_set_buf(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
@@ -757,6 +758,7 @@ static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg;
 
+	memset(&sg, 0, sizeof(sg));
 	sg_set_buf(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,

Re: linux-next: upstream tree build warnings

[Rusty Russell]

On Thu, 23 Apr 2009 04:54:03 pm David Miller wrote:
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Thu, 23 Apr 2009 16:38:47 +1000
> 
> > Today's linux-next build (powerpc allyesconfig gcc4.4.0) produced these
> > warnings:
> > 
> > drivers/net/virtio_net.c: In function 'virnet_vlan_rx_add_vid':
> > include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> > drivers/net/virtio_net.c:746: note: 'sg' was declared here
> > drivers/net/virtio_net.c: In function 'virnet_vlan_rx_kill_vid':
> > include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> > drivers/net/virtio_net.c:758: note: 'sg' was declared here
> > 
> > in each case, sg is uninitialised when its address is passed to
> > sg_set_buf() which passes it to gs_set_page() which passes it to
> > sg_assign_page() which dereferences it (to use ->page_link).
> 
> I wonder if this is a side effect of changes that went in via Rusty's
> tree?  I don't remember touching this driver in a while.

AFAICT gcc is right: it should be sg_init_one().

It doesn't currently *matter*, since no virtio add_buf implementation
does that chained-sg crap (I had a patch once, but it made me barf) and
so the uninitialized lower three bits are ignored.

Alex, if you agree, patch welcome...
Rusty.

Re: [PATCH] virtio_net: memset scatterlist before using

[Rusty Russell]

On Sat, 25 Apr 2009 12:07:03 am Alex Williamson wrote:
> A powerpc gcc-4.4 build generates the following warnings:
> 
> drivers/net/virtio_net.c: In function 'virnet_vlan_rx_add_vid':
> include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> drivers/net/virtio_net.c:746: note: 'sg' was declared here
> drivers/net/virtio_net.c: In function 'virnet_vlan_rx_kill_vid':
> include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
> drivers/net/virtio_net.c:758: note: 'sg' was declared here
> 
> We need to clear the scatterlist before using it.  This sg entry will
> get copied into another scatterlist before it's used, so memset it
> rather than using sg_init_table or sg_init_one to avoid a bogus end
> marker.

This will break CONFIG_DEBUG_SG=y.

Why not sg_init_one?  This is what it's for...

Thanks,
Rusty

Re: [PATCH] virtio_net: memset scatterlist before using

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Sun, 26 Apr 2009 22:28:08 +0930

> On Sat, 25 Apr 2009 12:07:03 am Alex Williamson wrote:
>> A powerpc gcc-4.4 build generates the following warnings:
>> 
>> drivers/net/virtio_net.c: In function 'virnet_vlan_rx_add_vid':
>> include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
>> drivers/net/virtio_net.c:746: note: 'sg' was declared here
>> drivers/net/virtio_net.c: In function 'virnet_vlan_rx_kill_vid':
>> include/linux/scatterlist.h:57: warning: 'sg' is used uninitialized in this function
>> drivers/net/virtio_net.c:758: note: 'sg' was declared here
>> 
>> We need to clear the scatterlist before using it.  This sg entry will
>> get copied into another scatterlist before it's used, so memset it
>> rather than using sg_init_table or sg_init_one to avoid a bogus end
>> marker.
> 
> This will break CONFIG_DEBUG_SG=y.
> 
> Why not sg_init_one?  This is what it's for...

Right.

Also, patch not sent to netdev due to a typo, and if it's not
sent to netdev it won't get tracked in patchwork and is therefore
likely to get lost.

Re: linux-next: upstream tree build warnings

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Sun, 26 Apr 2009 21:53:10 +0930

> Alex, if you agree, patch welcome...

And please CC: netdev properly so that the patch gets tracked
in patchwork, thanks.

[PATCH] virtio_net: Cleanup command queue scatterlist usage

[Alex Williamson]

We were avoiding calling sg_init* on scatterlists passed
into virtnet_send_command to prevent extraneous end markers.
This caused build warnings for uninitialized variables.
Cleanup the code to create proper scatterlists.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 drivers/net/virtio_net.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0718558..ed70df4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -616,10 +616,11 @@ static int virtnet_open(struct net_device *dev)
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *data, int out, int in)
 {
-	struct scatterlist sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
+	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
 	struct virtio_net_ctrl_hdr ctrl;
 	virtio_net_ctrl_ack status = ~0;
 	unsigned int tmp;
+	int i;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
@@ -634,7 +635,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	sg_init_table(sg, out + in);
 
 	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
-	memcpy(&sg[1], data, sizeof(struct scatterlist) * (out + in - 2));
+	for_each_sg(data, s, out + in - 2, i)
+		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
 	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
 
 	BUG_ON(vi->cvq->vq_ops->add_buf(vi->cvq, sg, out, in, vi));
@@ -688,7 +690,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	promisc = ((dev->flags & IFF_PROMISC) != 0);
 	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-	sg_set_buf(sg, &promisc, sizeof(promisc));
+	sg_init_one(sg, &promisc, sizeof(promisc));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC,
@@ -696,7 +698,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
 			 promisc ? "en" : "dis");
 
-	sg_set_buf(sg, &allmulti, sizeof(allmulti));
+	sg_init_one(sg, &allmulti, sizeof(allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI,
@@ -712,6 +714,8 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 		return;
 	}
 
+	sg_init_table(sg, 2);
+
 	/* Store the unicast list and count in the front of the buffer */
 	mac_data->entries = dev->uc_count;
 	addr = dev->uc_list;
@@ -745,7 +749,7 @@ static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg;
 
-	sg_set_buf(&sg, &vid, sizeof(vid));
+	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
 				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1, 0))
@@ -757,7 +761,7 @@ static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg;
 
-	sg_set_buf(&sg, &vid, sizeof(vid));
+	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
 				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1, 0))

Re: [PATCH] virtio_net: Cleanup command queue scatterlist usage

[David Miller]

From: Alex Williamson <alex.williamson@hp.com>
Date: Mon, 27 Apr 2009 09:50:03 -0600

> We were avoiding calling sg_init* on scatterlists passed
> into virtnet_send_command to prevent extraneous end markers.
> This caused build warnings for uninitialized variables.
> Cleanup the code to create proper scatterlists.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Looks good to me, what say you Rusty?

Re: [PATCH] virtio_net: Cleanup command queue scatterlist usage

[David Miller]

From: Alex Williamson <alex.williamson@hp.com>
Date: Mon, 27 Apr 2009 09:50:03 -0600

> We were avoiding calling sg_init* on scatterlists passed
> into virtnet_send_command to prevent extraneous end markers.
> This caused build warnings for uninitialized variables.
> Cleanup the code to create proper scatterlists.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

This patch does not apply even to Linus's current tree.

Re: [PATCH] virtio_net: Cleanup command queue scatterlist usage

[Alex Williamson]

On Fri, 2009-05-01 at 15:33 -0700, David Miller wrote:
> From: Alex Williamson <alex.williamson@hp.com>
> Date: Mon, 27 Apr 2009 09:50:03 -0600
> 
> > We were avoiding calling sg_init* on scatterlists passed
> > into virtnet_send_command to prevent extraneous end markers.
> > This caused build warnings for uninitialized variables.
> > Cleanup the code to create proper scatterlists.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> 
> This patch does not apply even to Linus's current tree.

It was against net-next-2.6.git, I'll update it for linux-2.6.git.
IIRC, it applied with some fuzz.

Alex

Re: [PATCH] virtio_net: Cleanup command queue scatterlist usage

[David Miller]

From: Alex Williamson <alex.williamson@hp.com>
Date: Fri, 01 May 2009 16:43:14 -0600

> On Fri, 2009-05-01 at 15:33 -0700, David Miller wrote:
>> From: Alex Williamson <alex.williamson@hp.com>
>> Date: Mon, 27 Apr 2009 09:50:03 -0600
>> 
>> > We were avoiding calling sg_init* on scatterlists passed
>> > into virtnet_send_command to prevent extraneous end markers.
>> > This caused build warnings for uninitialized variables.
>> > Cleanup the code to create proper scatterlists.
>> > 
>> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
>> 
>> This patch does not apply even to Linus's current tree.
> 
> It was against net-next-2.6.git, I'll update it for linux-2.6.git.
> IIRC, it applied with some fuzz.

Thanks.  I want to apply this to net-2.6 to fix those uninitialed
scatterlist structure issues.

[PATCH] virtio_net: Cleanup command queue scatterlist usage

[Alex Williamson]

We were avoiding calling sg_init* on scatterlists passed
into virtnet_send_command to prevent extraneous end markers.
This caused build warnings for uninitialized variables.
Cleanup the code to create proper scatterlists.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

 Patch against net-2.6.git 7a67e56fd362d3edfde1f19170893508c3940d3a
 
 drivers/net/virtio_net.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9c82a39..23eb282 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -616,10 +616,11 @@ static int virtnet_open(struct net_device *dev)
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *data, int out, int in)
 {
-	struct scatterlist sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
+	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
 	struct virtio_net_ctrl_hdr ctrl;
 	virtio_net_ctrl_ack status = ~0;
 	unsigned int tmp;
+	int i;
 
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
 		BUG();  /* Caller should know better */
@@ -637,7 +638,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	sg_init_table(sg, out + in);
 
 	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
-	memcpy(&sg[1], data, sizeof(struct scatterlist) * (out + in - 2));
+	for_each_sg(data, s, out + in - 2, i)
+		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
 	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
 
 	if (vi->cvq->vq_ops->add_buf(vi->cvq, sg, out, in, vi) != 0)
@@ -692,7 +694,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	promisc = ((dev->flags & IFF_PROMISC) != 0);
 	allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-	sg_set_buf(sg, &promisc, sizeof(promisc));
+	sg_init_one(sg, &promisc, sizeof(promisc));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC,
@@ -700,7 +702,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
 			 promisc ? "en" : "dis");
 
-	sg_set_buf(sg, &allmulti, sizeof(allmulti));
+	sg_init_one(sg, &allmulti, sizeof(allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI,
@@ -716,6 +718,8 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 		return;
 	}
 
+	sg_init_table(sg, 2);
+
 	/* Store the unicast list and count in the front of the buffer */
 	mac_data->entries = dev->uc_count;
 	addr = dev->uc_list;
@@ -749,7 +753,7 @@ static void virnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg;
 
-	sg_set_buf(&sg, &vid, sizeof(vid));
+	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
 				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1, 0))
@@ -761,7 +765,7 @@ static void virnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg;
 
-	sg_set_buf(&sg, &vid, sizeof(vid));
+	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
 				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1, 0))

Re: [PATCH] virtio_net: Cleanup command queue scatterlist usage

[David Miller]

From: Alex Williamson <alex.williamson@hp.com>
Date: Fri, 01 May 2009 21:27:56 -0600

> We were avoiding calling sg_init* on scatterlists passed
> into virtnet_send_command to prevent extraneous end markers.
> This caused build warnings for uninitialized variables.
> Cleanup the code to create proper scatterlists.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Applied.

Re: [PATCH] virtio_net: Cleanup command queue scatterlist usage

[Rusty Russell]

On Tue, 28 Apr 2009 01:20:03 am Alex Williamson wrote:
> We were avoiding calling sg_init* on scatterlists passed
> into virtnet_send_command to prevent extraneous end markers.
> This caused build warnings for uninitialized variables.
> Cleanup the code to create proper scatterlists.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.