Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

On Tue, 2 May 2017 14:39:40 -0600
David Ahern [off-list ref] wrote:

On 5/2/17 1:49 PM, Stephen Hemminger wrote:

quoted

I am not disagreeing that iproute2 should handle the extended
error format. Just want the solution to be as small as possible;
ie do no more than is absolutely necessary. And future proof
for the inevitable growth in new area.  

Understood. I was trying to not reinvent a wheel. nla_parse and
nla_validate have not really been touched in 10 years, and both are well
written with a good API to the rest of the code base.

From there, I grabbed whole snippets as opposed to just taking what is
needed for the ext-ack feature. I left the name as nlattr.c for easy
diff if future code is wanted. The header file was renamed to nlattr.h
to avoid confusion with other netlink.h files. Not adding it to
libnetlink.h facilitates pulling more code via diff as needed.

In short, I think it is good to re-use code from the kernel (lib and
tools/lib) where possible and doing so in a way that makes updates as
easy as header files.

The problem with copy and paste is that the code diverges and rots.
Also the security and error model in user space are different

quoted

quoted

+
+static const __u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+	[NLA_U8]	= sizeof(__u8),
+	[NLA_U16]	= sizeof(__u16),
+	[NLA_U32]	= sizeof(__u32),
+	[NLA_U64]	= sizeof(__u64),
+	[NLA_MSECS]	= sizeof(__u64),
+	[NLA_NESTED]	= NLA_HDRLEN,
+	[NLA_S8]	= sizeof(__s8),
+	[NLA_S16]	= sizeof(__s16),
+	[NLA_S32]	= sizeof(__s32),
+	[NLA_S64]	= sizeof(__s64),
+};
+  

This patch makes iproute2 now doing validation of netlink attributes
coming back from the kernel. What is the point, userspace should be
trusting the kernel.  

The kernel has bugs too; userspace should verify what it sends. In this
case the policy validation is just data types -- a string was expected
and a string was returned, or an attribute should be a u32 and it is.
You can argue it is overkill for iproute2, but it is good coding practice.

And for many netlink based features iproute2 tends to be the model that
is copied into other code bases.

Then why only for new code.

I am not trying to be overly picky. Just that review is the time to play
devil's advocate and look for issues.