Message-ID: <9fd4bf6f-47d1-4178-d16e-ec3b3d099ae3@gmail.com>
Date:   Mon, 1 Oct 2018 09:01:27 -0600
From:   David Ahern <dsahern@...il.com>
To:     Mauricio Faria de Oliveira <mfo@...onical.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter
 family headers

On 10/1/18 6:44 AM, Mauricio Faria de Oliveira wrote:
>> I suspect rtnl_fdb_dump is forever stuck with the ifinfomsg struct as
>> the header if any kernel side filtering is to be done. [snip]
> 
> Why exactly?   I understand currently there may be little information
> to distinguish family headers, but if it comes down to certain attributes
> the function expects/uses, that can be checked if nlmsg_parse() is OK.
> 
> Otherwise, if it comes down to some struct field that is not common
> between both structs, then.. well. Maybe something else/new.

struct ifinfomsg is 16 bytes; ndmsg is 12 bytes. The difference is
ifi_change in the ifinfomsg. If you don't know which header is sent, how
can you reliably parse -- and believe -- the result of the parsing?

Yes, iproute2 0's out the structs. So does FRR. But the general argument
is that userspace may not and the kernel has to this point happily
ignored fields it was not using.

The short of it is that ifi_change may be non-0 and should not be relied
on. That's the theory anyways ...

> 
>> [...] As for the change
>> above, I suggest something like this:
>>
>>         /* if header struct is ndmsg, no attributes can be appended */
>>         if (nlmsg_len(nlh) != sizeof(struct ndmsg)) {
>>                 current ifinfomsg based code
>>         }
>>
> 
> Hm, not sure -- ndmsg can be used with attributes too in iproute2, e.g., 'dev'.
> In that case, the payload size is different/greater, and even makes
> the length check pass:
> 
> $ ip --family bridge neigh
> [  585.111034] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 12
> RTNETLINK answers: Invalid argument
> Dump terminated
> 
> $ ip --family bridge neigh show dev ens3
> [  540.231443] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 20
> [  540.234433] DEBUG: rtnl_fdb_dump():3749 nlmsg_len(nlh) 20
> lladdr 33:33:00:00:00:01 PERMANENT
> lladdr 01:00:5e:00:00:01 PERMANENT
> lladdr 33:33:ff:e9:9d:60 PERMANENT
> 
>> We certainly do not want to ignore parse failures.
> 
> Well, if there are no attributes, that shouldn't be a problem,
> but unfortunately that's what happens in this case :- (
> 
> Now, given the example above makes the attribute parsing pass,
> that allows the payload interpretation as ifm struct to be used.
> Fortunately this field is commonly aligned on both structs,
> and is not used in ndmsg per iproute2 ipneigh.c do_show_or_flush().
> But this might pose problems in the future if things change.
> 
> If you could explain your thoughts a bit more about it, it would be
> really great.
> 

Perhaps there is a work around. IFLA_MASTER is the only supported
attribute that can be appended, and it is sent as a u32. Then the
rtnl_fdb_dump function has 4 legitimate cases:

1. ndmsg = size 12 bytes
2. ndmsg + MASTER = 20 ?
3. ifinfomsg = size 16 bytes
4. ifinfomsg + MASTER = 24 ?

'ip neigh show' could send NDA_IFINDEX as an additional attribute. That
is my mistake. I should have set ndm_ifindex rather than using the
attribute, but that ship sailed 3 years ago. Anyways, that case too
might be uniquely detected. The size checks have been used in other
places, so should be ok here too.

At this point the use of ifinfomsg for dumps has created a mess
extending kernel side filtering. The point of the PROPER_HDR patch set
is to give a point in time across all dump functions where the kernel
and userspace can reliably communicate about what is wanted.

Powered by blists - more mailing lists