From: David Miller <da...@davemloft.net>
Date: Mon, 18 Apr 2016 20:57:55 -0400 (EDT)
> From: Eric Dumazet <eric.duma...@gmail.com>
> Date: Mon, 18 Apr 2016 14:35:56 -0700
>
>> On Mon, 2016-04-18 at 14:10 -0700, Roopa Prabhu wrote:
>>
>>> + if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64)) {
>>> + struct rtnl_link_stats64 *sp;
>>> +
>>> + attr = nla_reserve(skb, IFLA_STATS_LINK_64,
>>> + sizeof(struct rtnl_link_stats64));
>>> + if (!attr)
>>> + goto nla_put_failure;
>>> +
>>> + sp = nla_data(attr);
>>
>> Are you sure we have a guarantee that sp is aligned to u64 fields ?
>>
>> x86 does not care, but some arches would have a potential misalign
>> access here.
>
> I'll do some testing on sparc and deal with any fallout.
Just thinking out loud before I start testing, yeah I think you are
right. nlmsghdr is 64-bit aligned, but the nlattr header is 32-bit
which will thus make the attribute data area not be aligned.
I think the time has probably come to have a new netlink attribute
format that doesn't have this multi-decade old problem.
There are a lot of bits left in nla_type, we can use one to indicate
that the nlattr struct is another 4 bytes in length in order to
archieve proper alignment of the payload data.
+struct nlattr64 {
+ __u16 nla_len;
+ __u16 nla_type;
+ __u32 nla_pad;
+};
...
+#define NLA_F_64BIT_ALIGNED (1 << 13)
-#define NLA_TYPE_MASK ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
+#define NLA_TYPE_MASK ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER |
NLA_F_64BIT_ALIGNED)
...
#define NLA64_ALIGNTO 8
#define NLA64_ALIGN(len) (((len) + NLA64_ALIGNTO - 1) & ~(NLA64_ALIGNTO
- 1))
#define NLA64_HDRLEN ((int) NLA64_ALIGN(sizeof(struct nlattr64)))
We're going to need some new nla64_*() helpers and code added to some
of the existing ones to test that new bit.
For example, nla_data would now be:
static inline void *nla_data(const struct nlattr *nla)
{
if (nla->nla_type & NLA_F_64BIT_ALIGNED)
return (char *) nla + NLA64_HDRLEN;
else
return (char *) nla + NLA_HDRLEN;
}
nla_len would be:
static inline int nla_len(const struct nlattr *nla)
{
int hdrlen = NLA_HDRLEN;
if (nla->nla_type & NLA_F_64BIT_ALIGNED)
hdrlen = NLA64_hdrlen;
return nla->nla_len - hdrlen;
}
etc. etc.
And anyways, I get unaligned accesses without Roopa's changes :-/
davem@patience:~$ ip l l
[3391066.656729] Kernel unaligned access at TPC[7d6c14]
loopback_get_stats64+0x74/0xa0
[3391066.672020] Kernel unaligned access at TPC[7d6c18]
loopback_get_stats64+0x78/0xa0
[3391066.687282] Kernel unaligned access at TPC[7d6c1c]
loopback_get_stats64+0x7c/0xa0
[3391066.702573] Kernel unaligned access at TPC[7d6c20]
loopback_get_stats64+0x80/0xa0
[3391066.717858] Kernel unaligned access at TPC[8609dc] dev_get_stats+0x3c/0xe0
