Network managers like netifd (used in OpenWRT for instance) try to
configure interface options after creation but before setting the
interface up.
Unfortunately the sysfs / bridge currently only allows to configure the
hash_max and multicast_router options when the bridge interface is up.
But since br_multicast_init() doesn't start any timers and only sets
default values and initializes timers it should be save to reconfigure
the default values after that, before things actually get active after
the bridge is set up.
With this patch hash_max and multicast_router attributes can be
changed even if the according bridge (port) is down, just like other
other bridge (port) attributes allow too.
Signed-off-by: Linus Lüssing <[email protected]>
---
I'm currently a little unsure about a few things (that's why I'm sending
this as an RFC):
* For i=br_multicast_init(), e=br_multicast_enable() and
s=br_multicast_stop() is the order i->e->s->e->s->e->... always
ensured by the netdev API? Will this work even if I have
many fast and wild calls to "ip link set up dev br0" and
"ip link set down dev br0" and some changes to hash_max and
multicast_router in between?
* Might calls to br_multicast_add_router() via br_multicast_enable_port()
cause unintended side-effects?
* (maybe independent of this patch: )
Can fast changes to the multicast_router attribute of a bridge (port)
cause race conditions because a del_timer() instead of a
del_timer_sync() is used in br_multicast_set_{port,}router()?
(or: why does br_multicast_stop() use del_timer_sync() while
br_multicast_disable_port() doesn't?)
* @Herbert: Do you remember whether there was a reason for
checking netif_running() or the bridge port state which
I might have overlooked?
PS: Running this patch didn't create any crashes for me so far
and multicast traffic still runs fine. No locust infestation in sight
yet either. Still careful though :P.
net/bridge/br_multicast.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2d69d5c..066199e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1775,8 +1775,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
int err = -ENOENT;
spin_lock_bh(&br->multicast_lock);
- if (!netif_running(br->dev))
- goto unlock;
switch (val) {
case 0:
@@ -1793,7 +1791,6 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
break;
}
-unlock:
spin_unlock_bh(&br->multicast_lock);
return err;
@@ -1802,18 +1799,15 @@ unlock:
int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
{
struct net_bridge *br = p->br;
- int err = -ENOENT;
+ int err = 0;
spin_lock(&br->multicast_lock);
- if (!netif_running(br->dev) || p->state == BR_STATE_DISABLED)
- goto unlock;
switch (val) {
case 0:
case 1:
case 2:
p->multicast_router = val;
- err = 0;
if (val < 2 && !hlist_unhashed(&p->rlist))
hlist_del_init_rcu(&p->rlist);
@@ -1834,7 +1828,6 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
break;
}
-unlock:
spin_unlock(&br->multicast_lock);
return err;
@@ -1939,15 +1932,11 @@ unlock:
int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val)
{
- int err = -ENOENT;
+ int err = -EINVAL;
u32 old;
struct net_bridge_mdb_htable *mdb;
spin_lock_bh(&br->multicast_lock);
- if (!netif_running(br->dev))
- goto unlock;
-
- err = -EINVAL;
if (!is_power_of_2(val))
goto unlock;
--
1.7.10.4
On Thu, May 21, 2015 at 04:59:22AM +0200, Linus L?ssing wrote:
>
> * For i=br_multicast_init(), e=br_multicast_enable() and
> s=br_multicast_stop() is the order i->e->s->e->s->e->... always
> ensured by the netdev API? Will this work even if I have
br_multicast_init always happens first obviously. But there is
no ordering requirement between e and s.
> many fast and wild calls to "ip link set up dev br0" and
> "ip link set down dev br0" and some changes to hash_max and
> multicast_router in between?
The timer operations are all supposed to be idempotent. So enabling
a port twice or stopping it twice should be OK.
> * Might calls to br_multicast_add_router() via br_multicast_enable_port()
> cause unintended side-effects?
What do you mean? How does add_router get called from enable_port?
> * (maybe independent of this patch: )
> Can fast changes to the multicast_router attribute of a bridge (port)
> cause race conditions because a del_timer() instead of a
> del_timer_sync() is used in br_multicast_set_{port,}router()?
> (or: why does br_multicast_stop() use del_timer_sync() while
> br_multicast_disable_port() doesn't?)
I don't think so. Firstly everything should happen under the
multicast lock except for stop/del_port. The latter rely on
those adding timers to check netif_running and port != disabled
to avoid races.
Apart from that you can always mod_timer twice or del_timer
twice so that's not really an issue.
> * @Herbert: Do you remember whether there was a reason for
> checking netif_running() or the bridge port state which
> I might have overlooked?
See above. It's there so that you don't readd a timer when we're
calling del_timer_sync. del_timer_sync has to be called without
the multicast lock so that's why we need another mechanism to
prevent the timers from being readded.
AFAICS the spots you patched aren't adding timers so they *should*
be OK.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, May 21, 2015 at 11:49:21AM +0800, Herbert Xu wrote:
> The timer operations are all supposed to be idempotent. So enabling
> a port twice or stopping it twice should be OK.
Oki doki.
>
> > * Might calls to br_multicast_add_router() via br_multicast_enable_port()
> > cause unintended side-effects?
>
> What do you mean? How does add_router get called from enable_port?
Sorry, ment br_multicast_add_router() via
br_multicast_set_port_router(). But it's not modifying any timers,
and other modifications are locked by the multicast lock, right.
> See above. It's there so that you don't readd a timer when we're
> calling del_timer_sync. del_timer_sync has to be called without
> the multicast lock so that's why we need another mechanism to
> prevent the timers from being readded.
Right, all the touched functions never rearm a timer. The
multicast_router timer may only get readded upon receiving a
multicast query.
(br_multicast_query_received()->br_multicast_mark_router() )
By removing the netif_running check we might only delete a timer
which wasn't running anyway which as you said already is safe.
>
> AFAICS the spots you patched aren't adding timers so they *should*
> be OK.
Okay, thanks for your thorough explanations about the timers and
how the locking is supposed to work. After your explanations I
went over the code a few more times and am fairly confident too
now, that this patch is supposed to work fine.
Going to resend this patch without the RFC tag.
Cheers, Linus