Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-05 Thread David Miller
From: Mauricio Faria de Oliveira 
Date: Fri, 5 Oct 2018 18:46:47 -0300

> On Fri, Oct 5, 2018 at 6:24 PM David Ahern  wrote:
>>
>> On 10/5/18 3:22 PM, David Miller wrote:
>> > From: Mauricio Faria de Oliveira 
>> > Date: Mon, 1 Oct 2018 22:50:32 -0300
>> >
>> >> On Mon, Oct 1, 2018 at 12:38 PM Mauricio Faria de Oliveira
>> >>  wrote:
>> >>> Ok, thanks for your suggestions.
>> >>> I'll do some research/learning on them, and give it a try for a v2.
>> >>
>> >> FYI, that is "[PATCH v2 net-next] rtnetlink: fix rtnl_fdb_dump() for
>> >> ndmsg header".
>> >>
>> >> BTW, could please advise whether this should be net or net-next? It's a 
>> >> bug fix,
>> >> but it's late in the cycle, and this is not urgent (the problem has been 
>> >> around
>> >> since v4.12), so not sure it's really needed for v4.19.
>> >
>> > I've applied this to net and queued it up for -stable, thanks.
>> >
> 
> Thanks.
> 
>> there was a v2 for this problem. I reviewed it and been testing it as
>> part of the strict dump patches.
> 
> The v2 is the applied patch :- ) [1]
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=bd961c9bc66497f0c63f4ba1d02900bb85078366

Oh right, I did apply v2.


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-05 Thread David Miller
From: David Ahern 
Date: Fri, 5 Oct 2018 15:24:29 -0600

> On 10/5/18 3:22 PM, David Miller wrote:
>> From: Mauricio Faria de Oliveira 
>> Date: Mon, 1 Oct 2018 22:50:32 -0300
>> 
>>> On Mon, Oct 1, 2018 at 12:38 PM Mauricio Faria de Oliveira
>>>  wrote:
 Ok, thanks for your suggestions.
 I'll do some research/learning on them, and give it a try for a v2.
>>>
>>> FYI, that is "[PATCH v2 net-next] rtnetlink: fix rtnl_fdb_dump() for
>>> ndmsg header".
>>>
>>> BTW, could please advise whether this should be net or net-next? It's a bug 
>>> fix,
>>> but it's late in the cycle, and this is not urgent (the problem has been 
>>> around
>>> since v4.12), so not sure it's really needed for v4.19.
>> 
>> I've applied this to net and queued it up for -stable, thanks.
>> 
> 
> there was a v2 for this problem. I reviewed it and been testing it as
> part of the strict dump patches.

Oh crap, sorry, please send me a relative fixup :-/


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-05 Thread Mauricio Faria de Oliveira
On Fri, Oct 5, 2018 at 6:24 PM David Ahern  wrote:
>
> On 10/5/18 3:22 PM, David Miller wrote:
> > From: Mauricio Faria de Oliveira 
> > Date: Mon, 1 Oct 2018 22:50:32 -0300
> >
> >> On Mon, Oct 1, 2018 at 12:38 PM Mauricio Faria de Oliveira
> >>  wrote:
> >>> Ok, thanks for your suggestions.
> >>> I'll do some research/learning on them, and give it a try for a v2.
> >>
> >> FYI, that is "[PATCH v2 net-next] rtnetlink: fix rtnl_fdb_dump() for
> >> ndmsg header".
> >>
> >> BTW, could please advise whether this should be net or net-next? It's a 
> >> bug fix,
> >> but it's late in the cycle, and this is not urgent (the problem has been 
> >> around
> >> since v4.12), so not sure it's really needed for v4.19.
> >
> > I've applied this to net and queued it up for -stable, thanks.
> >

Thanks.

> there was a v2 for this problem. I reviewed it and been testing it as
> part of the strict dump patches.

The v2 is the applied patch :- ) [1]

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=bd961c9bc66497f0c63f4ba1d02900bb85078366

cheers,

-- 
Mauricio Faria de Oliveira


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-05 Thread David Ahern
On 10/5/18 3:22 PM, David Miller wrote:
> From: Mauricio Faria de Oliveira 
> Date: Mon, 1 Oct 2018 22:50:32 -0300
> 
>> On Mon, Oct 1, 2018 at 12:38 PM Mauricio Faria de Oliveira
>>  wrote:
>>> Ok, thanks for your suggestions.
>>> I'll do some research/learning on them, and give it a try for a v2.
>>
>> FYI, that is "[PATCH v2 net-next] rtnetlink: fix rtnl_fdb_dump() for
>> ndmsg header".
>>
>> BTW, could please advise whether this should be net or net-next? It's a bug 
>> fix,
>> but it's late in the cycle, and this is not urgent (the problem has been 
>> around
>> since v4.12), so not sure it's really needed for v4.19.
> 
> I've applied this to net and queued it up for -stable, thanks.
> 

there was a v2 for this problem. I reviewed it and been testing it as
part of the strict dump patches.


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-05 Thread David Miller
From: Mauricio Faria de Oliveira 
Date: Mon, 1 Oct 2018 22:50:32 -0300

> On Mon, Oct 1, 2018 at 12:38 PM Mauricio Faria de Oliveira
>  wrote:
>> Ok, thanks for your suggestions.
>> I'll do some research/learning on them, and give it a try for a v2.
> 
> FYI, that is "[PATCH v2 net-next] rtnetlink: fix rtnl_fdb_dump() for
> ndmsg header".
> 
> BTW, could please advise whether this should be net or net-next? It's a bug 
> fix,
> but it's late in the cycle, and this is not urgent (the problem has been 
> around
> since v4.12), so not sure it's really needed for v4.19.

I've applied this to net and queued it up for -stable, thanks.


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-01 Thread Mauricio Faria de Oliveira
On Mon, Oct 1, 2018 at 12:38 PM Mauricio Faria de Oliveira
 wrote:
> Ok, thanks for your suggestions.
> I'll do some research/learning on them, and give it a try for a v2.

FYI, that is "[PATCH v2 net-next] rtnetlink: fix rtnl_fdb_dump() for
ndmsg header".

BTW, could please advise whether this should be net or net-next? It's a bug fix,
but it's late in the cycle, and this is not urgent (the problem has been around
since v4.12), so not sure it's really needed for v4.19.

Thanks,

-- 
Mauricio Faria de Oliveira


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-01 Thread Mauricio Faria de Oliveira
On Mon, Oct 1, 2018 at 12:01 PM David Ahern  wrote:
>
> 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.

Right, I see the point of the unknown header type, and wanted to understand
whether you had more reasons in mind, as I imagined the particular
users/attributes
involved in this function could have provided an way out.

*But*, as you pointed out, the general case is that userspace can do anything.
So, now I see -- the assumption for particular cases is not an option. Thanks.

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

And/or ndm_ifindex, which in the ifinfomsg type cast, gives brport_idx
a non-zero value
I don't yet know if it's equivalent, but if not, that's a problem.
(i.e., specify a non-brport thing, but it's processed as such)

> 
> 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.

Ok, thanks for your suggestions.
I'll do some research/learning on them, and give it a try for a v2.

> 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.

Nice. I guess for this particular problem/thread, we'll won't be able to
use that one, since existing/older userspace that doesn't know about
it should be fixed for.. but definitely a great thing that should help in
this general problem.

Thank you,

-- 
Mauricio Faria de Oliveira


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-01 Thread David Ahern
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.


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-10-01 Thread Mauricio Faria de Oliveira
On Sun, Sep 30, 2018 at 10:06 PM David Ahern  wrote:
>
> On 9/28/18 1:35 PM, Mauricio Faria de Oliveira wrote:
> > Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
> > which is not always true.  For example, 'struct ndmsg' is used by iproute2
> > as well (in the 'ip neigh' command).
> >
> > The problem is, the function bails out early if nlmsg_parse() fails, which
> > does occur for iproute2 usage of 'struct ndmsg' because the payload length
> > is shorter than the family header alone (as 'struct ifinfomsg' is assumed).
> >
> > This breaks backward compatibility with userspace (different response) and
> > is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from
> >  rtnl_fdb_dump() on parse error").
> ...
>
> >
> > Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on 
> > parse error")
> > Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
> > Reported-by: Aidan Obley 
> > Signed-off-by: Mauricio Faria de Oliveira 
> > ---
> > P.S.: this may be 'net', but labeling as 'net-next' for possible relation 
> > to recent thread
> > [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data 
> > in dump request
> >
> >  net/core/rtnetlink.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index 60c928894a78..9695a27cc9b9 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -3744,16 +3744,17 @@ static int rtnl_fdb_dump(struct sk_buff *skb, 
> > struct netlink_callback *cb)
> >   int err = 0;
> >   int fidx = 0;
> >
> > - err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> > -   IFLA_MAX, ifla_policy, NULL);
> > - if (err < 0) {
> > - return -EINVAL;
> > - } else if (err == 0) {
> > + /* The family header may _not_ be struct ifinfomsg
> > +  * (e.g., struct ndmsg).  Usage of the ifm pointer
> > +  * must check payload length (e.g., nlmsg_parse()).
> > +  */
> > + if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> > + IFLA_MAX, ifla_policy, NULL) == 0) {
> >   if (tb[IFLA_MASTER])
> >   br_idx = nla_get_u32(tb[IFLA_MASTER]);
> > - }
> >
> > - brport_idx = ifm->ifi_index;
> > + brport_idx = ifm->ifi_index;
> > + }
> >
> >   if (br_idx) {
> >   br_dev = __dev_get_by_index(net, br_idx);
> >
>

David, thanks for reviewing this.

> 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.

> [...] 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.

Thanks again,

-- 
Mauricio Faria de Oliveira


Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-09-30 Thread David Ahern
On 9/28/18 1:35 PM, Mauricio Faria de Oliveira wrote:
> Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
> which is not always true.  For example, 'struct ndmsg' is used by iproute2
> as well (in the 'ip neigh' command).
> 
> The problem is, the function bails out early if nlmsg_parse() fails, which
> does occur for iproute2 usage of 'struct ndmsg' because the payload length
> is shorter than the family header alone (as 'struct ifinfomsg' is assumed).
> 
> This breaks backward compatibility with userspace (different response) and
> is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from 
>  rtnl_fdb_dump() on parse error").
...

> 
> Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse 
> error")
> Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
> Reported-by: Aidan Obley 
> Signed-off-by: Mauricio Faria de Oliveira 
> ---
> P.S.: this may be 'net', but labeling as 'net-next' for possible relation to 
> recent thread
> [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in 
> dump request
> 
>  net/core/rtnetlink.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 60c928894a78..9695a27cc9b9 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3744,16 +3744,17 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct 
> netlink_callback *cb)
>   int err = 0;
>   int fidx = 0;
>  
> - err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> -   IFLA_MAX, ifla_policy, NULL);
> - if (err < 0) {
> - return -EINVAL;
> - } else if (err == 0) {
> + /* The family header may _not_ be struct ifinfomsg
> +  * (e.g., struct ndmsg).  Usage of the ifm pointer
> +  * must check payload length (e.g., nlmsg_parse()).
> +  */
> + if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> + IFLA_MAX, ifla_policy, NULL) == 0) {
>   if (tb[IFLA_MASTER])
>   br_idx = nla_get_u32(tb[IFLA_MASTER]);
> - }
>  
> - brport_idx = ifm->ifi_index;
> + brport_idx = ifm->ifi_index;
> + }
>  
>   if (br_idx) {
>   br_dev = __dev_get_by_index(net, br_idx);
> 

I suspect rtnl_fdb_dump is forever stuck with the ifinfomsg struct as
the header if any kernel side filtering is to be done. 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
}

We certainly do not want to ignore parse failures.


[PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers

2018-09-28 Thread Mauricio Faria de Oliveira
Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
which is not always true.  For example, 'struct ndmsg' is used by iproute2
as well (in the 'ip neigh' command).

The problem is, the function bails out early if nlmsg_parse() fails, which
does occur for iproute2 usage of 'struct ndmsg' because the payload length
is shorter than the family header alone (as 'struct ifinfomsg' is assumed).

This breaks backward compatibility with userspace (different response) and
is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from 
 rtnl_fdb_dump() on parse error").

Some examples with iproute2 and netlink library for go [1]:

 1) $ bridge fdb show
33:33:00:00:00:01 dev ens3 self permanent
01:00:5e:00:00:01 dev ens3 self permanent
33:33:ff:15:98:30 dev ens3 self permanent

  This one works, as it uses 'struct ifinfomsg'.

  fdb_show() @ iproute2/bridge/fdb.c
"""
.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
...
if (rtnl_dump_request(, RTM_GETNEIGH, [...]
"""

 2) $ ip --family bridge neigh
RTNETLINK answers: Invalid argument
Dump terminated

  This one fails, as it uses 'struct ndmsg'.

  do_show_or_flush() @ iproute2/ip/ipneigh.c
"""
.n.nlmsg_type = RTM_GETNEIGH,
.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)),
"""

 3) $ ./neighlist
< no output >

  This one fails, as it uses 'struct ndmsg'-based.

  neighList() @ netlink/neigh_linux.go
"""
req := h.newNetlinkRequest(unix.RTM_GETNEIGH, [...]
msg := Ndmsg{
"""

The actual breakage was introduced by commit 0ff50e83b512 ("net: rtnetlink:
bail out from rtnl_fdb_dump() on parse error"), because nlmsg_parse() fails
if the payload length (with the _actual_ family header) is less than the
family header length alone (which is assumed, in parameter 'hdrlen').
This is true in the examples above with struct ndmsg, with size and payload
length shorter than struct ifinfomsg.

However, that commit just intends to fix something under the assumption the
family header is indeed an 'struct ifinfomsg' - by preventing access to the
payload as such (via 'ifm' pointer) if the payload length is not sufficient
to actually contain it.

The assumption was introduced by commit 5e6d24358799 ("bridge: netlink dump
interface at par with brctl"), to support iproute2's 'bridge fdb' command
(not 'ip neigh') which indeed uses 'struct ifinfomsg', thus is not broken.

So, in order to unbreak shorter family headers as 'struct ndmsg' and still
prevent access to invalid payload data, let's revert that former fix, and
move ifinfomsg payload access (via ifm) into nlmsg_parse() successful case
(i.e., the payload length is sufficient to be accessed as struct ifinfomsg)
which also works/returns 0 even in case there are no attributes in payload.

Same examples with this patch applied (or revert/before the original fix):

$ bridge fdb show
33:33:00:00:00:01 dev ens3 self permanent
01:00:5e:00:00:01 dev ens3 self permanent
33:33:ff:15:98:30 dev ens3 self permanent

$ ip --family bridge neigh
dev ens3 lladdr 33:33:00:00:00:01 PERMANENT
dev ens3 lladdr 01:00:5e:00:00:01 PERMANENT
dev ens3 lladdr 33:33:ff:15:98:30 PERMANENT

$ ./neighlist
netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, 
IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0x0, 0x0, 0x0, 0x1}, 
LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, 
IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x1, 0x0, 0x5e, 0x0, 0x0, 0x1}, 
LLIPAddr:net.IP(nil), Vlan:0, VNI:0}
netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, 
IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0xff, 0x15, 0x98, 
0x30}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0}

Tested on mainline (ad0371482b1e) and net-next (a804e5e21875) on Sep. 28.

References:

[1] netlink library for go (test-case)
https://github.com/vishvananda/netlink

$ cat ~/go/src/neighlist/main.go
package main
import ("fmt"; "syscall"; "github.com/vishvananda/netlink")
func main() {
neighs, _ := netlink.NeighList(0, syscall.AF_BRIDGE)
for _, neigh := range neighs { fmt.Printf("%#v\n", neigh) }
}

$ export GOPATH=~/go
$ go get github.com/vishvananda/netlink
$ go build neighlist
$ ~/go/src/neighlist/neighlist

Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse 
error")
Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
Reported-by: Aidan Obley 
Signed-off-by: Mauricio Faria de Oliveira 
---
P.S.: this may be 'net', but labeling as 'net-next' for possible relation to 
recent thread
[PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in 
dump request

 net/core/rtnetlink.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git