Re: Seeing new kernel unaligned access messages in linux-next on ia64

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[Jan Engelhardt]

Hi,


Tony Luck observes that the original IFLA_STATS64 submission causes
unaligned accesses. This is because nla_data() returns a pointer to a
memory region that is only aligned to 32 bits.

Using a temporary and memcpying it off would normally fix this,
as in the patch below. During testing however, I still get
unaligned messages even with the patch - and I would not know
what causes this. In fact, adding a printks magically fixes
it. (Bug in gcc-4.4-sparc compiler?)

 	memcpy(v, &a, sizeof(a));
+       printk(KERN_INFO "v=%p a=%p\n", v, &a);


origin git://dev.medozas.de/linux net
mode cherry-pick
parent 1c01fe14a87332cc88266fbd6e598319322eb96f (v2.6.34-rc1-1069-g1c01fe1)
commit 5480c9bb1b418bb09748340257dea1e57efeb18f
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Wed Mar 24 19:52:43 2010 +0100

net: fix unaligned access in IFLA_STATS64

Tony Luck observes that the original IFLA_STATS64 submission causes
unaligned accesses. This is because nla_data() returns a pointer to a
memory region that is only aligned to 32 bits. Do some memcpying to
workaround this.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/core/rtnetlink.c |   53 +++++++++++++++++++++--------------------
 1 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e1121f0..473d4b1 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -602,36 +602,39 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
 	a->tx_compressed = b->tx_compressed;
 }
 
-static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *a,
+static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *v,
 				   const struct net_device_stats *b)
 {
-	a->rx_packets = b->rx_packets;
-	a->tx_packets = b->tx_packets;
-	a->rx_bytes = b->rx_bytes;
-	a->tx_bytes = b->tx_bytes;
-	a->rx_errors = b->rx_errors;
-	a->tx_errors = b->tx_errors;
-	a->rx_dropped = b->rx_dropped;
-	a->tx_dropped = b->tx_dropped;
-
-	a->multicast = b->multicast;
-	a->collisions = b->collisions;
-
-	a->rx_length_errors = b->rx_length_errors;
-	a->rx_over_errors = b->rx_over_errors;
-	a->rx_crc_errors = b->rx_crc_errors;
-	a->rx_frame_errors = b->rx_frame_errors;
-	a->rx_fifo_errors = b->rx_fifo_errors;
-	a->rx_missed_errors = b->rx_missed_errors;
-
-	a->tx_aborted_errors = b->tx_aborted_errors;
-	a->tx_carrier_errors = b->tx_carrier_errors;
-	a->tx_fifo_errors = b->tx_fifo_errors;
-	a->tx_heartbeat_errors = b->tx_heartbeat_errors;
-	a->tx_window_errors = b->tx_window_errors;
-
-	a->rx_compressed = b->rx_compressed;
-	a->tx_compressed = b->tx_compressed;
+	struct rtnl_link_stats64 a;
+
+	a.rx_packets = b->rx_packets;
+	a.tx_packets = b->tx_packets;
+	a.rx_bytes = b->rx_bytes;
+	a.tx_bytes = b->tx_bytes;
+	a.rx_errors = b->rx_errors;
+	a.tx_errors = b->tx_errors;
+	a.rx_dropped = b->rx_dropped;
+	a.tx_dropped = b->tx_dropped;
+
+	a.multicast = b->multicast;
+	a.collisions = b->collisions;
+
+	a.rx_length_errors = b->rx_length_errors;
+	a.rx_over_errors = b->rx_over_errors;
+	a.rx_crc_errors = b->rx_crc_errors;
+	a.rx_frame_errors = b->rx_frame_errors;
+	a.rx_fifo_errors = b->rx_fifo_errors;
+	a.rx_missed_errors = b->rx_missed_errors;
+
+	a.tx_aborted_errors = b->tx_aborted_errors;
+	a.tx_carrier_errors = b->tx_carrier_errors;
+	a.tx_fifo_errors = b->tx_fifo_errors;
+	a.tx_heartbeat_errors = b->tx_heartbeat_errors;
+	a.tx_window_errors = b->tx_window_errors;
+
+	a.rx_compressed = b->rx_compressed;
+	a.tx_compressed = b->tx_compressed;
+	memcpy(v, &a, sizeof(a));
 }
 
 static inline int rtnl_vfinfo_size(const struct net_device *dev)
@@ -734,8 +737,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			sizeof(struct rtnl_link_stats64));
 	if (attr == NULL)
 		goto nla_put_failure;
-
-	stats = dev_get_stats(dev);
 	copy_rtnl_link_stats64(nla_data(attr), stats);
 
 	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) {
-- 
# Created with git-export-patch

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[Andreas Schwab]

Jan Engelhardt <jengelh@medozas.de> writes:

> Using a temporary and memcpying it off would normally fix this,
> as in the patch below. During testing however, I still get
> unaligned messages even with the patch - and I would not know
> what causes this.

The memcpy will not fix the alignment issue because the copy operation
is fully equivalent to a direct assignment, and the compiler can still
take advantage of the known alignment of the types.  You have to
explicitly tell the compiler about the reduced alignment guarantee.

> In fact, adding a printks magically fixes it. (Bug in gcc-4.4-sparc
> compiler?)
>
>  	memcpy(v, &a, sizeof(a));
> +       printk(KERN_INFO "v=%p a=%p\n", v, &a);

Presumably the extended lifetime of the variables caused the compiler to
use a different expansion for memcpy which is less dependent on
alignment.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[Jan Engelhardt]

On Wednesday 2010-03-24 23:47, Andreas Schwab wrote:
>Jan Engelhardt writes:
>
>> Using a temporary and memcpying it off would normally fix this,
>> as in the patch below. During testing however, I still get
>> unaligned messages even with the patch - and I would not know
>> what causes this.
>
>The memcpy will not fix the alignment issue because the copy operation
>is fully equivalent to a direct assignment, and the compiler can still
>take advantage of the known alignment of the types.  You have to
>explicitly tell the compiler about the reduced alignment guarantee.

You're right, I remember seeing that sort of optimization before. So I 
have changed the function's signature to read

-static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *v,
-                                  const struct net_device_stats *b)
+static void copy_rtnl_link_stats64(void *v, const struct net_device_stats *b)
 {
 ...
 	memcpy(v, &a, sizeof(a));
 }

No more unaligned messages - but is this an acceptable solution?

thanks,
Jan

RE: Seeing new kernel unaligned access messages in linux-next on ia64

[Luck, Tony]

> No more unaligned messages - but is this an acceptable solution?

There are a bunch of macros in include/linux/unaligned/*.h to
handle this sort of thing.

-Tony

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[David Miller]

From: "Luck, Tony" <tony.luck@intel.com>
Date: Wed, 24 Mar 2010 16:19:38 -0700

>> No more unaligned messages - but is this an acceptable solution?
> 
> There are a bunch of macros in include/linux/unaligned/*.h to
> handle this sort of thing.

It's totally unnecessary here and it would be overkill to
use those interfaces one at a time on every struct member
when a proper memcpy() fro ma type-pruned void pointer
suffices.

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[David Miller]

From: Jan Engelhardt <jengelh@medozas.de>
Date: Thu, 25 Mar 2010 00:17:01 +0100 (CET)

> No more unaligned messages - but is this an acceptable solution?

We already rely on this elsewhere, particularly in the
xfrm_user code.

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[Jan Engelhardt]

On Thursday 2010-03-25 04:32, David Miller wrote:
>From: Jan Engelhardt
>Date: Thu, 25 Mar 2010 00:17:01 +0100 (CET)
>
>> No more unaligned messages - but is this [using void*]
>> an acceptable solution?
>
>We already rely on this elsewhere, particularly in the
>xfrm_user code.

Ok, here's it. (We're also getting rid of the double-obtain statistics
that was mentioned in the initial submission.)


Please apply this one, or pull, or cherry-pick from
	git://dev.medozas.de/linux net

parent b79d1d54cf0672f764402fe4711ef5306f917bd3 (v2.6.34-rc1-1275-gb79d1d5)
commit c5c57d7c7837858aa499610a3ee760b39f1de937
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Wed Mar 24 19:52:43 2010 +0100

net: fix unaligned access in IFLA_STATS64

Tony Luck observes that the original IFLA_STATS64 submission causes
unaligned accesses. This is because nla_data() returns a pointer to a
memory region that is only aligned to 32 bits. Do some memcpying to
workaround this.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/core/rtnetlink.c |   54 +++++++++++++++++++++---------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ffc6cf3..ed0766f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -602,36 +602,38 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
 	a->tx_compressed = b->tx_compressed;
 }
 
-static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *a,
-				   const struct net_device_stats *b)
+static void copy_rtnl_link_stats64(void *v, const struct net_device_stats *b)
 {
-	a->rx_packets = b->rx_packets;
-	a->tx_packets = b->tx_packets;
-	a->rx_bytes = b->rx_bytes;
-	a->tx_bytes = b->tx_bytes;
-	a->rx_errors = b->rx_errors;
-	a->tx_errors = b->tx_errors;
-	a->rx_dropped = b->rx_dropped;
-	a->tx_dropped = b->tx_dropped;
-
-	a->multicast = b->multicast;
-	a->collisions = b->collisions;
-
-	a->rx_length_errors = b->rx_length_errors;
-	a->rx_over_errors = b->rx_over_errors;
-	a->rx_crc_errors = b->rx_crc_errors;
-	a->rx_frame_errors = b->rx_frame_errors;
-	a->rx_fifo_errors = b->rx_fifo_errors;
-	a->rx_missed_errors = b->rx_missed_errors;
-
-	a->tx_aborted_errors = b->tx_aborted_errors;
-	a->tx_carrier_errors = b->tx_carrier_errors;
-	a->tx_fifo_errors = b->tx_fifo_errors;
-	a->tx_heartbeat_errors = b->tx_heartbeat_errors;
-	a->tx_window_errors = b->tx_window_errors;
-
-	a->rx_compressed = b->rx_compressed;
-	a->tx_compressed = b->tx_compressed;
+	struct rtnl_link_stats64 a;
+
+	a.rx_packets = b->rx_packets;
+	a.tx_packets = b->tx_packets;
+	a.rx_bytes = b->rx_bytes;
+	a.tx_bytes = b->tx_bytes;
+	a.rx_errors = b->rx_errors;
+	a.tx_errors = b->tx_errors;
+	a.rx_dropped = b->rx_dropped;
+	a.tx_dropped = b->tx_dropped;
+
+	a.multicast = b->multicast;
+	a.collisions = b->collisions;
+
+	a.rx_length_errors = b->rx_length_errors;
+	a.rx_over_errors = b->rx_over_errors;
+	a.rx_crc_errors = b->rx_crc_errors;
+	a.rx_frame_errors = b->rx_frame_errors;
+	a.rx_fifo_errors = b->rx_fifo_errors;
+	a.rx_missed_errors = b->rx_missed_errors;
+
+	a.tx_aborted_errors = b->tx_aborted_errors;
+	a.tx_carrier_errors = b->tx_carrier_errors;
+	a.tx_fifo_errors = b->tx_fifo_errors;
+	a.tx_heartbeat_errors = b->tx_heartbeat_errors;
+	a.tx_window_errors = b->tx_window_errors;
+
+	a.rx_compressed = b->rx_compressed;
+	a.tx_compressed = b->tx_compressed;
+	memcpy(v, &a, sizeof(a));
 }
 
 static inline int rtnl_vfinfo_size(const struct net_device *dev)
@@ -734,8 +736,6 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			sizeof(struct rtnl_link_stats64));
 	if (attr == NULL)
 		goto nla_put_failure;
-
-	stats = dev_get_stats(dev);
 	copy_rtnl_link_stats64(nla_data(attr), stats);
 
 	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) {
-- 
# Created with git-export-patch

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[David Miller]

From: Jan Engelhardt <jengelh@medozas.de>
Date: Sat, 27 Mar 2010 13:14:46 +0100 (CET)

> net: fix unaligned access in IFLA_STATS64

Applied to net-next-2.6, thanks Jan.

Hey, don't we need some adjustments to if_nlmsg_size()?  I don't see
it accounting for IFLA_STATS64/"struct rtnl_link_stats64" there.

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[Jan Engelhardt]

On Sunday 2010-03-28 00:37, David Miller wrote:
>
>> net: fix unaligned access in IFLA_STATS64
>
>Applied to net-next-2.6, thanks Jan.
>
>Hey, don't we need some adjustments to if_nlmsg_size()?  I don't see
>it accounting for IFLA_STATS64/"struct rtnl_link_stats64" there.

If I am not mistaken, the answer is "not strictly". But of course it's 
nicer if we don't need to realloc just because we were too conservative 
in the initial calculation.


git://dev.medozas.de/linux net

parent c5c57d7c7837858aa499610a3ee760b39f1de937 (v2.6.34-rc1-1276-gc5c57d7)
commit 305876b7c1c720db30239d08d56b3a058d56aa21
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Sun Mar 28 01:03:32 2010 +0100

net: increase preallocated size of nlmsg to accomodate for IFLA_STATS64

When more data is stuffed into an nlmsg than initially projected, an
extra allocation needs to be done. Reserve enough for IFLA_STATS64 so
that this does not to needlessy happen.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/core/rtnetlink.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ed0766f..bf919b6 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -653,6 +653,7 @@ static inline size_t if_nlmsg_size(const struct net_device *dev)
 	       + nla_total_size(IFNAMSIZ) /* IFLA_QDISC */
 	       + nla_total_size(sizeof(struct rtnl_link_ifmap))
 	       + nla_total_size(sizeof(struct rtnl_link_stats))
+	       + nla_total_size(sizeof(struct rtnl_link_stats64))
 	       + nla_total_size(MAX_ADDR_LEN) /* IFLA_ADDRESS */
 	       + nla_total_size(MAX_ADDR_LEN) /* IFLA_BROADCAST */
 	       + nla_total_size(4) /* IFLA_TXQLEN */
-- 
# Created with git-export-patch

Re: Seeing new kernel unaligned access messages in linux-next on ia64

[David Miller]

From: Jan Engelhardt <jengelh@medozas.de>
Date: Sun, 28 Mar 2010 01:11:13 +0100 (CET)

> On Sunday 2010-03-28 00:37, David Miller wrote:
>>
>>> net: fix unaligned access in IFLA_STATS64
>>
>>Applied to net-next-2.6, thanks Jan.
>>
>>Hey, don't we need some adjustments to if_nlmsg_size()?  I don't see
>>it accounting for IFLA_STATS64/"struct rtnl_link_stats64" there.
> 
> If I am not mistaken, the answer is "not strictly". But of course it's 
> nicer if we don't need to realloc just because we were too conservative 
> in the initial calculation.

Right.

> net: increase preallocated size of nlmsg to accomodate for IFLA_STATS64
> 
> When more data is stuffed into an nlmsg than initially projected, an
> extra allocation needs to be done. Reserve enough for IFLA_STATS64 so
> that this does not to needlessy happen.
> 
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

Applied, thanks a lot!