Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Vlad Yasevich <vyasevic@redhat.com>]

On 03/13/2013 11:36 AM, Stephen Hemminger wrote:
> On Wed, 13 Mar 2013 10:18:58 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> Range/validity checks on rta_type in rtnetlink_rcv_msg() do
>> not account for flags that may be set.  This causes the function
>> to return -EINVAL when flags are set on the type (for example
>> NLA_F_NESTED).
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>   net/core/rtnetlink.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 1868625..dc5edf1 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2538,7 +2538,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>>   		struct rtattr *attr = (void *)nlh + NLMSG_ALIGN(min_len);
>>
>>   		while (RTA_OK(attr, attrlen)) {
>> -			unsigned int flavor = attr->rta_type;
>> +			unsigned int flavor = attr->rta_type & NLA_TYPE_MASK;
>>   			if (flavor) {
>>   				if (flavor > rta_max[sz_idx])
>>   					return -EINVAL;
>
> No. This is effectively an ABI change. It adds nothing.
>
> The NLA_F_NESTED attribute wasn't in the first generation version of netlink
> (before my time with Linux). It doesn't make sense to all of sudden start
> accepting it on requests. Also, then you would expect the query to set
> the NESTED flag as well, and that would be another ABI change.
>

So let me rebuff this a bit more intelligently.

1) NLA_F_NESTED is used by netfilter in a lot of places.  It seems that
only rtnetlink interface doesn't account for it.

2) The following commit:

commit 25c71c75ac87508528db053b818944f3650dd7a6
Author: stephen hemminger <shemminger@vyatta.com>
Date:   Tue Nov 13 07:53:05 2012 +0000

     bridge: bridge port parameters over netlink

introduced NLA_F_NESTED usage in rtnetlink for both setlink and
getlink, so one could argue that was an ABI change.  However, I can
prove that without the change I am introducing, one can not use the 
above mentioned bridge API.  Feel free to try it with iproute2 patches
I sent earlier ([PATCH iproute2 0/2] Add support for bridge port link 
information).

Now, if we truly want to say that we may not introduce NLA_F_NESTED into
the rtnetlink patch, then the above commit to bridge needs to be
reworked.

Do you still object to this patch?  If so, we would have to rework the
bridge netlink api.  Otherwise, it enables the current API to actually work.

Thanks
-vlad