netdev - Re: [PATCH 1/3] net: add a new NETDEV

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] [day] [month] [year] [list]

Date:	Tue, 20 Aug 2013 18:30:09 +0100
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	netdev <netdev@...r.kernel.org>, amwang <amwang@...hat.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Stephen Hemminger <stephen@...workplumber.org>,
	kaber <kaber@...sh.net>, David Miller <davem@...emloft.net>,
	vyasevic <vyasevic@...hat.com>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type

2013/8/20 Johannes Berg <johannes@...solutions.net>:
> On Tue, 2013-08-20 at 16:30 +0100, Florian Fainelli wrote:
>
>> > Also, maybe it would be worth doing it in one call? If you need to
>> > change both, then you'd end up calling the notifier twice, which is less
>> > efficient?
>>
>> I have mixed feelings about this. I do not expect changing the
>> headroom/tailroom to be in a hot-path, and we would need to have a
>> name such as dev_set_head_and_tailroom() or something that clearly
>> states that it operates on both quantities. Looking at the subsystems
>> and drivers, there are quite a lot of users which only set one or the
>> other, occasionaly both before registration.
>
> No, it shouldn't be on a path that has any performance impact at all,
> that's true.
>
>> > I suppose you could make them 'int' arguments and reserve -1
>> > for no changes, or just require both new values to be given (if doing
>> > this at all.)
>>
>> What I like about keeping them separate is that we can use the
>> "native" storage type that is used in struct net_device, and have
>> compile-time checking of this.
>
> Makes sense.
>
> I was really more thinking about the notifier complexity.
>
> Right now, you can potentially blow up your iterations - for example if
> you have a vlan on a bridge:
>  * driver sets headroom (or tailroom)
>  * this iterates all netdevs, including the bridge
>  * bridge calls the function again, and while iterating iterates again,
> then
>    going into the vlan
>    (is it even valid to iterate while iterating?)
>  * vlan calls it again and it iterates again, doing nothing this time
>
> So now you've iterated the netdevs many times...

That's right, although I am not sure if we can really do something
about it, the network device stacking in that scheme, is just complex
by nature. Fortunately at some point we stop notifying since
dev->headroom == new_headroom. I am pretty sure the same applies
already for NETDEV_CHANGEADDR and NETDEV_CHANGEMTU events today.
-- 
Florian
--
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