| Message ID | 20160418.234137.26692086759328024.davem@davemloft.net |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > Date: Mon, 18 Apr 2016 14:10:19 -0700 > > > This patch adds a new RTM_GETSTATS message to query link stats via > > netlink from the kernel. RTM_NEWLINK also dumps stats today, but > > RTM_NEWLINK returns a lot more than just stats and is expensive in > > some cases when frequent polling for stats from userspace is a > > common operation. > > I'm holding off on this until we sort out the 64-bit netlink > attribute alignment issue. > > Meanwhile, I'll some kind of a fix into the tree for the > rtnl_fill_stats() change so that it doesn't cause unaligned > accesses. Also it looks like my previous feedback for sctp_get_sctp_info() has been ignored. <Quoting http://www.spinics.net/lists/netdev/msg372523.html > info is not guaranteed to be aligned on 8 bytes. You need to use put_unaligned() Check commit ff5d749772018 ("tcp: beware of alignments in tcp_get_info()") for details. </quote>
On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote: > > + /* Add a zero length NOP attribute so that the nla_data() > + * of the IFLA_STATS64 will be 64-bit aligned. > + */ > +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS > + attr = nla_reserve(skb, IFLA_PAD, 0); > + if (!attr) > + return -EMSGSIZE; > +#endif You must do this only if current skb->data alignment is not correct. (IFLA_ALIAS for example could shift your expectations) Also you probably want to change if_nlmsg_size() to add + nla_total_size(0) /* IFLA_PAD */
On 4/18/16, 8:41 PM, David Miller wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > Date: Mon, 18 Apr 2016 14:10:19 -0700 > >> This patch adds a new RTM_GETSTATS message to query link stats via >> netlink from the kernel. RTM_NEWLINK also dumps stats today, but >> RTM_NEWLINK returns a lot more than just stats and is expensive in >> some cases when frequent polling for stats from userspace is a >> common operation. > I'm holding off on this until we sort out the 64-bit netlink > attribute alignment issue. sure, > > Meanwhile, I'll some kind of a fix into the tree for the > rtnl_fill_stats() change so that it doesn't cause unaligned > accesses. > > I just tested out a clever idea, where for architectures where > unaligned accesses is a problem, we insert a zero length NOP attribute > before the 64-bit stats. This makes it properly aligned. A quick > hack patch just passed testing on my sparc64 box, but I'll go over it > some more. > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index bb3a90b..5ffdcb3 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -155,6 +155,7 @@ enum { > IFLA_PROTO_DOWN, > IFLA_GSO_MAX_SEGS, > IFLA_GSO_MAX_SIZE, > + IFLA_PAD, > __IFLA_MAX > }; > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index a7a3d34..b192576 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -1052,6 +1052,15 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb, > struct rtnl_link_stats64 *sp; > struct nlattr *attr; > > + /* Add a zero length NOP attribute so that the nla_data() > + * of the IFLA_STATS64 will be 64-bit aligned. > + */ > +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS > + attr = nla_reserve(skb, IFLA_PAD, 0); > + if (!attr) > + return -EMSGSIZE; > +#endif > + > attr = nla_reserve(skb, IFLA_STATS64, > sizeof(struct rtnl_link_stats64)); > if (!attr) > that is cleaver :) thanks!
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 18 Apr 2016 21:32:04 -0700 > On Mon, 2016-04-18 at 23:41 -0400, David Miller wrote: >> >> + /* Add a zero length NOP attribute so that the nla_data() >> + * of the IFLA_STATS64 will be 64-bit aligned. >> + */ >> +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS >> + attr = nla_reserve(skb, IFLA_PAD, 0); >> + if (!attr) >> + return -EMSGSIZE; >> +#endif > > You must do this only if current skb->data alignment is not correct. I'll put an assertion there if it makes you happy. :-)
Le 19/04/2016 05:41, David Miller a écrit : [snip] > I just tested out a clever idea, where for architectures where > unaligned accesses is a problem, we insert a zero length NOP attribute > before the 64-bit stats. This makes it properly aligned. A quick > hack patch just passed testing on my sparc64 box, but I'll go over it > some more. We saw also problems with architectures like tilera. Just for reminder, there was a first attempt by Thomas to make this patch generic: http://patchwork.ozlabs.org/patch/207097/ But we finally discover that some netlink API use the attribute '0'. Ideally, it would be great to 'tell' to the libnetlink the attribute used for padding, so that every attribute can be aligned on 8 if needed, but I don't see a way to do it without changing the whole libnetlink API (and thus all users). Or could we done the opposite: using Thomas patch by default and introducing a new API in the libnetlink for netlink messages where the attribute '0' is used? Some were identified in this patch: 31e20bad8d58 ("diag: warn about missing first netlink attribute"). I like this idea, because it will fix all users, not only rtnl, but the risk is to forget someone. Any thoughts? Regards, Nicolas
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Tue, 19 Apr 2016 09:45:32 +0200 > But we finally discover that some netlink API use the attribute '0'. I explicitly did not use attribute zero, and instead made a new IFLA_* attribute exactly to avoid compatability problems. Every netlink attribute space that needs to deal with this padding issue will need to add such a new attribute. It is the only safe way to deal with this problem.
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index bb3a90b..5ffdcb3 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -155,6 +155,7 @@ enum { IFLA_PROTO_DOWN, IFLA_GSO_MAX_SEGS, IFLA_GSO_MAX_SIZE, + IFLA_PAD, __IFLA_MAX }; diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index a7a3d34..b192576 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1052,6 +1052,15 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb, struct rtnl_link_stats64 *sp; struct nlattr *attr; + /* Add a zero length NOP attribute so that the nla_data() + * of the IFLA_STATS64 will be 64-bit aligned. + */ +#ifndef HAVE_EFFICIENT_UNALIGNED_ACCESS + attr = nla_reserve(skb, IFLA_PAD, 0); + if (!attr) + return -EMSGSIZE; +#endif + attr = nla_reserve(skb, IFLA_STATS64, sizeof(struct rtnl_link_stats64)); if (!attr)