netdev - Re: [net-next PATCH 2/2] bridge: netlink dump interface at par with brctl

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]

Message-ID: <53970745.7040002@redhat.com>
Date:	Tue, 10 Jun 2014 09:25:25 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>, davem@...emloft.net,
	stephen@...workplumber.org
CC:	netdev@...r.kernel.org, sfeldma@...ulusnetworks.com,
	john.r.fastabend@...el.com, roopa@...ulusnetworks.com
Subject: Re: [net-next PATCH 2/2] bridge: netlink dump interface at par with
 brctl

On 06/10/2014 07:41 AM, Jamal Hadi Salim wrote:
> On 06/09/14 12:41, Vlad Yasevich wrote:
>> On 06/07/2014 10:27 AM, Jamal Hadi Salim wrote:
>>> From: Jamal Hadi Salim <jhs@...atatu.com>
>>>
>>> Actually better than brctl showmacs because we can filter by bridge
>>> port in the kernel.
>>> The current bridge netlink interface doesnt scale when you have many
>>> bridges each with large fdbs or even bridges with many bridge ports
>>>
>>> For example usage look at accompanying iproute2 patch.
>>
>> The code was a bit tough to follow.  I think the main reason is
>> that you now always pass a filtering devices even when there was
>> no filtering information requested.
>>
>> I am wondering if it could be made simpler...
>>
> 
> The patch may be hard to follow i think. I cant think of a simple
> way to do filtering by br and brport. If you have suggestions, shoot.
> 

I gave it some thought and I think something like the following
pseudo-code would work.

dump_dev_fdbs(dev, filter)
{
        if (dev->dumper)
                dev->ndo_dumper(dev, filter);
        else
                default_dumper(dev, filter);
}

for_each_netdev() {
        if (bridge_filter) {
                if (dev->index != bridge_filter)
                        skip;

                dump_dev_fdbs(dev, port_filter);
        } else {
                if (port_filter) {
                        if (bridge_port &&
                            dev->index != port_filter)
                                skip;

                }

                if (bridge_port) {
                        br_dev = get_bridge();
                        dump_dev_fdbs(br_dev, port_filter);
                }

                dump_dev_fdbs(dev, port_filter);
        }
}


What do you think?

-vlad
>>>       rcu_read_lock();
>>> +    if (br_idx) {
>>> +        br_dev = __dev_get_by_index(net, br_idx);
>>> +        if (!br_dev) {
>>> +            rcu_read_unlock();
>>> +            return -ENODEV;
>>> +        }
>>> +        ops = br_dev->netdev_ops;
>>> +        bdev = br_dev;
>>> +    }
>>> +
>>
>> I think this can be outside of the rcu since you hold an rtnl at this
>> time.
>>
> 
> Will fix on next iteration.
> 
> cheers,
> jamal

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