Patchwork [net-next,0/7] Allow to monitor multicast cache event via rtnetlink

login
register
mail settings
Submitter Nicolas Dichtel
Date Dec. 6, 2012, 8:43 a.m.
Message ID <50C05AC2.1050504@6wind.com>
Download mbox | patch
Permalink /patch/204175/
State RFC
Delegated to: David Miller
Headers show

Comments

Nicolas Dichtel - Dec. 6, 2012, 8:43 a.m.
Le 05/12/2012 18:54, David Miller a écrit :
> From: "David Laight" <David.Laight@ACULAB.COM>
> Date: Wed, 5 Dec 2012 11:41:33 -0000
>
>> Probably worth commenting that the 64bit items might only be 32bit aligned.
>> Just to stop anyone trying to read/write them with pointer casts.
>
> Rather, let's not create this situation at all.
>
> It's totally inappropriate to have special code to handle every single
> time we want to put 64-bit values into netlink messages.
>
> We need a real solution to this issue.
>
The easiest way is to update *_ALIGNTO values (maybe we can keep NLMSG_ALIGNTO 
to 4). But I think that many userland apps have these values hardcoded and, the 
most important thing, this may increase size of many netlink messages. Hence we 
need probably to find something better.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Graf - Dec. 6, 2012, 5:49 p.m.
On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
> Le 05/12/2012 18:54, David Miller a écrit :
> >From: "David Laight" <David.Laight@ACULAB.COM>
> >Date: Wed, 5 Dec 2012 11:41:33 -0000
> >
> >>Probably worth commenting that the 64bit items might only be 32bit aligned.
> >>Just to stop anyone trying to read/write them with pointer casts.
> >
> >Rather, let's not create this situation at all.
> >
> >It's totally inappropriate to have special code to handle every single
> >time we want to put 64-bit values into netlink messages.
> >
> >We need a real solution to this issue.
> >
> The easiest way is to update *_ALIGNTO values (maybe we can keep
> NLMSG_ALIGNTO to 4). But I think that many userland apps have these
> values hardcoded and, the most important thing, this may increase
> size of many netlink messages. Hence we need probably to find
> something better.

We can't do this, as you say, ALIGNTO is compiled into all the
binaries.

A simple backwards compatible workaround would be to include an
unknown, empty padding attribute if needed. That would be 4 bytes
in size and could be used to include padding as needed.

We could use nla_type = 0 as it is a reserved value that should
be available in all protocols. All readers (kernel and user space)
must ignore such an attribute just like any other unknown
attribute they encounter.

We could easily extend nla_put_u64() and variants to automatically
include such a padding attribute as needed.

The only situation that I can think of where this would not work
is if we have code like this:

   foo = nla_nest_start();
   for ([..])
      nla_put_u64([...])
   nla_nest_end([...])

and a reader would stupidly do a nla_for_each_attr() in user space
and assume all attributes found must be NLA_U64 without even
checking the length of the attribute.

I would say we take that risk and let such code die horribly.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - Dec. 6, 2012, 9:49 p.m.
Le 06/12/2012 18:49, Thomas Graf a écrit :
> On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
>> Le 05/12/2012 18:54, David Miller a écrit :
>>> From: "David Laight" <David.Laight@ACULAB.COM>
>>> Date: Wed, 5 Dec 2012 11:41:33 -0000
>>>
>>>> Probably worth commenting that the 64bit items might only be 32bit aligned.
>>>> Just to stop anyone trying to read/write them with pointer casts.
>>>
>>> Rather, let's not create this situation at all.
>>>
>>> It's totally inappropriate to have special code to handle every single
>>> time we want to put 64-bit values into netlink messages.
>>>
>>> We need a real solution to this issue.
>>>
>> The easiest way is to update *_ALIGNTO values (maybe we can keep
>> NLMSG_ALIGNTO to 4). But I think that many userland apps have these
>> values hardcoded and, the most important thing, this may increase
>> size of many netlink messages. Hence we need probably to find
>> something better.
>
> We can't do this, as you say, ALIGNTO is compiled into all the
> binaries.
>
> A simple backwards compatible workaround would be to include an
> unknown, empty padding attribute if needed. That would be 4 bytes
> in size and could be used to include padding as needed.
>
> We could use nla_type = 0 as it is a reserved value that should
> be available in all protocols. All readers (kernel and user space)
> must ignore such an attribute just like any other unknown
> attribute they encounter.
>
> We could easily extend nla_put_u64() and variants to automatically
> include such a padding attribute as needed.
>
> The only situation that I can think of where this would not work
> is if we have code like this:
>
>     foo = nla_nest_start();
>     for ([..])
>        nla_put_u64([...])
>     nla_nest_end([...])
>
> and a reader would stupidly do a nla_for_each_attr() in user space
> and assume all attributes found must be NLA_U64 without even
> checking the length of the attribute.
>
> I would say we take that risk and let such code die horribly.
>
Ok, I can work to a patch next week if you want (I will be off until Tuesday).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight - Dec. 7, 2012, 10:38 a.m.
> On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
> > Le 05/12/2012 18:54, David Miller a écrit :
> > >From: "David Laight" <David.Laight@ACULAB.COM>
> > >Date: Wed, 5 Dec 2012 11:41:33 -0000
> > >
> > >>Probably worth commenting that the 64bit items might only be 32bit aligned.
> > >>Just to stop anyone trying to read/write them with pointer casts.
> > >
> > >Rather, let's not create this situation at all.
> > >
> > >It's totally inappropriate to have special code to handle every single
> > >time we want to put 64-bit values into netlink messages.
> > >
> > >We need a real solution to this issue.
> > >
> > The easiest way is to update *_ALIGNTO values (maybe we can keep
> > NLMSG_ALIGNTO to 4). But I think that many userland apps have these
> > values hardcoded and, the most important thing, this may increase
> > size of many netlink messages. Hence we need probably to find
> > something better.
> 
> We can't do this, as you say, ALIGNTO is compiled into all the
> binaries.
> 
> A simple backwards compatible workaround would be to include an
> unknown, empty padding attribute if needed. That would be 4 bytes
> in size and could be used to include padding as needed.

What are you going to align the data with respect to?
I doubt you can assume that the start of the netlink
message itself is 8 byte aligned - so any attempt
to 8 byte align an item is probably doomed to failure.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Graf - Dec. 7, 2012, 10:58 a.m.
On 12/07/12 at 10:38am, David Laight wrote:
> What are you going to align the data with respect to?
> I doubt you can assume that the start of the netlink
> message itself is 8 byte aligned - so any attempt
> to 8 byte align an item is probably doomed to failure.

Correct me if I'm wrong but skb->head will be aligned to
SKB_DATA_ALIGN() which as I understand is guaranted to
be 8 bytse aligned. So we can just add needed padding if
skb->data would not be aligned correctly after the next
netlink attribute header was added.

All user space needs to do is use a receive buffer that
is 8 byte aligned as well and we are good to go.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - Dec. 11, 2012, 3:03 p.m.
Le 06/12/2012 18:49, Thomas Graf a écrit :
> On 12/06/12 at 09:43am, Nicolas Dichtel wrote:
>> Le 05/12/2012 18:54, David Miller a écrit :
>>> From: "David Laight" <David.Laight@ACULAB.COM>
>>> Date: Wed, 5 Dec 2012 11:41:33 -0000
>>>
>>>> Probably worth commenting that the 64bit items might only be 32bit aligned.
>>>> Just to stop anyone trying to read/write them with pointer casts.
>>>
>>> Rather, let's not create this situation at all.
>>>
>>> It's totally inappropriate to have special code to handle every single
>>> time we want to put 64-bit values into netlink messages.
>>>
>>> We need a real solution to this issue.
>>>
>> The easiest way is to update *_ALIGNTO values (maybe we can keep
>> NLMSG_ALIGNTO to 4). But I think that many userland apps have these
>> values hardcoded and, the most important thing, this may increase
>> size of many netlink messages. Hence we need probably to find
>> something better.
>
> We can't do this, as you say, ALIGNTO is compiled into all the
> binaries.
>
> A simple backwards compatible workaround would be to include an
> unknown, empty padding attribute if needed. That would be 4 bytes
> in size and could be used to include padding as needed.
>
> We could use nla_type = 0 as it is a reserved value that should
> be available in all protocols. All readers (kernel and user space)
> must ignore such an attribute just like any other unknown
> attribute they encounter.
>
> We could easily extend nla_put_u64() and variants to automatically
> include such a padding attribute as needed.
In fact, it seems not so easy because most users of nlmsg_new() calculate
the exact needed length, thus if we add an unpredicted attribute, the message 
will be too small.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Graf - Dec. 11, 2012, 6:40 p.m.
On 12/11/12 at 04:03pm, Nicolas Dichtel wrote:
> In fact, it seems not so easy because most users of nlmsg_new() calculate
> the exact needed length, thus if we add an unpredicted attribute,
> the message will be too small.

True, we would either need to fix the calculations by accounting
for an additional 4 bytes for each 64bit arg or just reserve an
additional fixed amount for padding per message in nlmsg_new().
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - Dec. 12, 2012, 5:30 p.m.
Le 11/12/2012 19:40, Thomas Graf a écrit :
> On 12/11/12 at 04:03pm, Nicolas Dichtel wrote:
>> In fact, it seems not so easy because most users of nlmsg_new() calculate
>> the exact needed length, thus if we add an unpredicted attribute,
>> the message will be too small.
>
> True, we would either need to fix the calculations by accounting
> for an additional 4 bytes for each 64bit arg or just reserve an
> additional fixed amount for padding per message in nlmsg_new().
>
I would say that reserving additional space is better, because we also
need to align attributes that contain u64 fields:

struct attribute_foo {
    __u32 bar;
    __u32 bar2;
    __u64 foo;
};

I wonder if it is better to align all attribute on 64-bits or only u64 and
add a new function nla_put_align64() for attribute with u64 fields.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/uapi/linux/netfilter/nfnetlink_compat.h 
b/include/uapi/linux/netfilter/nfnetlink_compat.h
index ffb9503..121e62a 100644
--- a/include/uapi/linux/netfilter/nfnetlink_compat.h
+++ b/include/uapi/linux/netfilter/nfnetlink_compat.h
@@ -33,7 +33,7 @@  struct nfattr {
  #define NFNL_NFA_NEST	0x8000
  #define NFA_TYPE(attr) 	((attr)->nfa_type & 0x7fff)

-#define NFA_ALIGNTO     4
+#define NFA_ALIGNTO     8
  #define NFA_ALIGN(len)	(((len) + NFA_ALIGNTO - 1) & ~(NFA_ALIGNTO - 1))
  #define NFA_OK(nfa,len)	((len) > 0 && (nfa)->nfa_len >= sizeof(struct nfattr) \
  	&& (nfa)->nfa_len <= (len))
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 78d5b8a..66d2a26 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -75,7 +75,7 @@  struct nlmsghdr {
     Check		NLM_F_EXCL
   */

-#define NLMSG_ALIGNTO	4U
+#define NLMSG_ALIGNTO	8U
  #define NLMSG_ALIGN(len) ( ((len)+NLMSG_ALIGNTO-1) & ~(NLMSG_ALIGNTO-1) )
  #define NLMSG_HDRLEN	 ((int) NLMSG_ALIGN(sizeof(struct nlmsghdr)))
  #define NLMSG_LENGTH(len) ((len)+NLMSG_ALIGN(NLMSG_HDRLEN))
@@ -145,7 +145,7 @@  struct nlattr {
  #define NLA_F_NET_BYTEORDER	(1 << 14)
  #define NLA_TYPE_MASK		~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)

-#define NLA_ALIGNTO		4
+#define NLA_ALIGNTO		8
  #define NLA_ALIGN(len)		(((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
  #define NLA_HDRLEN		((int) NLA_ALIGN(sizeof(struct nlattr)))

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 33d29ce..ee898c1 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -146,7 +146,7 @@  struct rtattr {

  /* Macros to handle rtattributes */

-#define RTA_ALIGNTO	4
+#define RTA_ALIGNTO	8
  #define RTA_ALIGN(len) ( ((len)+RTA_ALIGNTO-1) & ~(RTA_ALIGNTO-1) )
  #define RTA_OK(rta,len) ((len) >= (int)sizeof(struct rtattr) && \
  			 (rta)->rta_len >= sizeof(struct rtattr) && \
@@ -322,7 +322,7 @@  struct rtnexthop {

  /* Macros to handle hexthops */

-#define RTNH_ALIGNTO	4
+#define RTNH_ALIGNTO	8
  #define RTNH_ALIGN(len) ( ((len)+RTNH_ALIGNTO-1) & ~(RTNH_ALIGNTO-1) )
  #define RTNH_OK(rtnh,len) ((rtnh)->rtnh_len >= sizeof(struct rtnexthop) && \
  			   ((int)(rtnh)->rtnh_len) <= (len))