netdev - Re: [PATCH net-next RESEND] net: Do not call ndo_dflt_fdb_dump if ndo_fdb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 12 Dec 2014 13:36:41 -0000
From:	"Hubert Sokolowski" <h.sokolowski@....edu.pl>
To:	"Jamal Hadi Salim" <jhs@...atatu.com>
Cc:	"Hubert Sokolowski" <h.sokolowski@....edu.pl>,
	"Roopa Prabhu" <roopa@...ulusnetworks.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Vlad Yasevich" <vyasevic@...hat.com>
Subject: Re: [PATCH net-next RESEND] net: Do not call ndo_dflt_fdb_dump if
 ndo_fdb_dump is defined.

> On 12/12/14 06:38, Hubert Sokolowski wrote:
>>> On 12/11/14, 9:06 AM, Hubert Sokolowski wrote:
>
>> Please see how the ndo_dflt_fdb_add and del are called:
>> 		if (dev->netdev_ops->ndo_fdb_add)
>> 			err = dev->netdev_ops->ndo_fdb_add(ndm, tb, dev, addr,
>> 							   vid,
>> 							   nlh->nlmsg_flags);
>> 		else
>> 			err = ndo_dflt_fdb_add(ndm, tb, dev, addr, vid,
>> 					       nlh->nlmsg_flags);
>>
>
> Semantically add and dump are not the same.
> Add adds a specific entry.
> Dump not only dumps the fdb table but also the unicast and multicast
> addresses.
this is not true for 3.16 and before. Please see:
http://lxr.free-electrons.com/source/net/core/rtnetlink.c?v=3.16#L2545
It lets you fully manage the FDB table by overwriding fdb_add, del and dump
in the same way.


>
>
>> As it was suggested by Ronen I can modify the patch so the dflt callback
>> is always called for bridge devices if this is desirable. Either by calling
>> it when following expression is true:
>>      (dev->priv_flags & IFF_BRIDGE_PORT)
>> or by modifying br_fdb_dump to call ndo_dflt_fdb_dump.
>>
>
> Are you saying the above is going to work? You need to TEST please.
yes, it works and it is not a rocket science :). we just need to agree
on the approach to use.

>
> It seems to me drivers which do this:
> ---
> .ndo_fdb_dump           = my_fdb_dump,
>
> and then my_fdb_dump is:
> return ndo_dflt_fdb_dump(skb, ncb, netdev, filter_dev, idx);
>
> are broken because we always have to dump the uni/multicast
> addresses *unconditionally* and these drivers claim to be
> overriding the dumper but are in fact calling the default dumper.
> They are not filtering anything as you had stated.
> I wish Vlad (Cced) would show up and say something ;->
>
> IOW, fix all the broken driver to not do that.

These "broken" drivers were written before 3.17 when the unconditional call
to dflt_dump was introduced. Some drivers implement own dump and don't
call dflt_dump at all, some call dflt_dump depending on some condition.
You can also call dflt_dump inside, save the idx result and continue
with own dump resulting in both uc/mc and own entries returned to the user.

thanks,
Hubert

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html