On Tue, 2 May 2017 14:39:40 -0600
David Ahern <d...@cumulusnetworks.com> wrote:
> On 5/2/17 1:49 PM, Stephen Hemminger wrote:
> > 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
> >
> >> +
> >> +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.
