25 Apr, 2016

2 commits

  • There is a race-condition when updating the mdb offload flag without using
    the mulicast_lock. This reverts commit 9e8430f8d60d98 ("bridge: mdb:
    Passing the port-group pointer to br_mdb module").

    This patch marks offloaded MDB entry as "offload" by changing the port-
    group flags and marks it as MDB_PG_FLAGS_OFFLOAD.

    When switchdev PORT_MDB succeeded and adds a multicast group, a completion
    callback is been invoked "br_mdb_complete". The completion function
    locks the multicast_lock and finds the right net_bridge_port_group and
    marks it as offloaded.

    Fixes: 9e8430f8d60d98 ("bridge: mdb: Passing the port-group pointer to br_mdb module")
    Reported-by: Nikolay Aleksandrov
    Signed-off-by: Elad Raz
    Signed-off-by: Jiri Pirko
    Reviewed-by: Ido Schimmel
    Acked-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Elad Raz
     
  • There is duplicate code that translates br_mdb_entry to br_ip let's wrap it
    in a common function.

    Signed-off-by: Elad Raz
    Signed-off-by: Jiri Pirko
    Reviewed-by: Ido Schimmel
    Acked-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Elad Raz
     

02 Mar, 2016

1 commit


23 Feb, 2016

1 commit


20 Feb, 2016

2 commits

  • Currently mdb entries are exported directly as a structure inside
    MDBA_MDB_ENTRY_INFO attribute, we can't really extend it without
    breaking user-space. In order to export new mdb fields, I've converted
    the MDBA_MDB_ENTRY_INFO into a nested attribute which starts like before
    with struct br_mdb_entry (without header, as it's casted directly in
    iproute2) and continues with MDBA_MDB_EATTR_ attributes. This way we
    keep compatibility with older users and can export new data.
    I've tested this with iproute2, both with and without support for the
    added attribute and it works fine.
    So basically we again have MDBA_MDB_ENTRY_INFO with struct br_mdb_entry
    inside but it may contain also some additional MDBA_MDB_EATTR_ attributes
    such as MDBA_MDB_EATTR_TIMER which can be parsed by user-space.

    So the new structure is:
    [MDBA_MDB] = {
    [MDBA_MDB_ENTRY] = {
    [MDBA_MDB_ENTRY_INFO]
    [MDBA_MDB_ENTRY_INFO] {
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     
  • Switch the port check and skip if it's null, this allows us to reduce one
    indentation level.

    Signed-off-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     

17 Feb, 2016

1 commit

  • A recent change to the mdb code confused the compiler to the point
    where it did not realize that the port-group returned from
    br_mdb_add_group() is always valid when the function returns a nonzero
    return value, so we get a spurious warning:

    net/bridge/br_mdb.c: In function 'br_mdb_add':
    net/bridge/br_mdb.c:542:4: error: 'pg' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    __br_mdb_notify(dev, entry, RTM_NEWMDB, pg);

    Slightly rearranging the code in br_mdb_add_group() makes the problem
    go away, as gcc is clever enough to see that both functions check
    for 'ret != 0'.

    Signed-off-by: Arnd Bergmann
    Fixes: 9e8430f8d60d ("bridge: mdb: Passing the port-group pointer to br_mdb module")
    Signed-off-by: David S. Miller

    Arnd Bergmann
     

09 Feb, 2016

2 commits


11 Jan, 2016

1 commit


30 Sep, 2015

1 commit

  • This patch changes the bridge vlan implementation to use rhashtables
    instead of bitmaps. The main motivation behind this change is that we
    need extensible per-vlan structures (both per-port and global) so more
    advanced features can be introduced and the vlan support can be
    extended. I've tried to break this up but the moment net_port_vlans is
    changed and the whole API goes away, thus this is a larger patch.
    A few short goals of this patch are:
    - Extensible per-vlan structs stored in rhashtables and a sorted list
    - Keep user-visible behaviour (compressed vlans etc)
    - Keep fastpath ingress/egress logic the same (optimizations to come
    later)

    Here's a brief list of some of the new features we'd like to introduce:
    - per-vlan counters
    - vlan ingress/egress mapping
    - per-vlan igmp configuration
    - vlan priorities
    - avoid fdb entries replication (e.g. local fdb scaling issues)

    The structure is kept single for both global and per-port entries so to
    avoid code duplication where possible and also because we'll soon introduce
    "port0 / aka bridge as port" which should simplify things further
    (thanks to Vlad for the suggestion!).

    Now we have per-vlan global rhashtable (bridge-wide) and per-vlan port
    rhashtable, if an entry is added to a port it'll get a pointer to its
    global context so it can be quickly accessed later. There's also a
    sorted vlan list which is used for stable walks and some user-visible
    behaviour such as the vlan ranges, also for error paths.
    VLANs are stored in a "vlan group" which currently contains the
    rhashtable, sorted vlan list and the number of "real" vlan entries.
    A good side-effect of this change is that it resembles how hw keeps
    per-vlan data.
    One important note after this change is that if a VLAN is being looked up
    in the bridge's rhashtable for filtering purposes (or to check if it's an
    existing usable entry, not just a global context) then the new helper
    br_vlan_should_use() needs to be used if the vlan is found. In case the
    lookup is done only with a port's vlan group, then this check can be
    skipped.

    Things tested so far:
    - basic vlan ingress/egress
    - pvids
    - untagged vlans
    - undef CONFIG_BRIDGE_VLAN_FILTERING
    - adding/deleting vlans in different scenarios (with/without global ctx,
    while transmitting traffic, in ranges etc)
    - loading/removing the module while having/adding/deleting vlans
    - extracting bridge vlan information (user ABI), compressed requests
    - adding/deleting fdbs on vlans
    - bridge mac change, promisc mode
    - default pvid change
    - kmemleak ON during the whole time

    Signed-off-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     

04 Aug, 2015

2 commits

  • Instead of trying to access br->vlan_enabled directly use the provided
    helper br_vlan_enabled().

    Signed-off-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     
  • Before this patch when a vid was not specified, the entry was added with
    vid 0 which is useless when vlan_filtering is enabled. This patch makes
    the entry to be added on all configured vlans when vlan filtering is
    enabled and respectively deleted from all, if the entry vid is 0.
    This is also closer to the way fdb works with regard to vid 0 and vlan
    filtering.

    Example:
    Setup:
    $ bridge vlan add vid 256 dev eth4
    $ bridge vlan add vid 1024 dev eth4
    $ bridge vlan add vid 64 dev eth3
    $ bridge vlan add vid 128 dev eth3
    $ bridge vlan
    port vlan ids
    eth3 1 PVID Egress Untagged
    64
    128

    eth4 1 PVID Egress Untagged
    256
    1024
    $ echo 1 > /sys/class/net/br0/bridge/vlan_filtering

    Before:
    $ bridge mdb add dev br0 port eth3 grp 239.0.0.1
    $ bridge mdb
    dev br0 port eth3 grp 239.0.0.1 temp

    After:
    $ bridge mdb add dev br0 port eth3 grp 239.0.0.1
    $ bridge mdb
    dev br0 port eth3 grp 239.0.0.1 temp vid 1
    dev br0 port eth3 grp 239.0.0.1 temp vid 128
    dev br0 port eth3 grp 239.0.0.1 temp vid 64

    Signed-off-by: Satish Ashok
    Signed-off-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Satish Ashok
     

01 Aug, 2015

1 commit


30 Jul, 2015

1 commit

  • Since mdb states were introduced when deleting an entry the state was
    left as it was set in the delete request from the user which leads to
    the following output when doing a monitor (for example):
    $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
    (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
    $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
    (monitor) dev br0 port eth3 grp 239.0.0.1 temp
    ^^^
    Note the "temp" state in the delete notification which is wrong since
    the entry was permanent, the state in a delete is always reported as
    "temp" regardless of the real state of the entry.

    After this patch:
    $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
    (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
    $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
    (monitor) dev br0 port eth3 grp 239.0.0.1 permanent

    There's one important note to make here that the state is actually not
    matched when doing a delete, so one can delete a permanent entry by
    stating "temp" in the end of the command, I've chosen this fix in order
    not to break user-space tools which rely on this (incorrect) behaviour.

    So to give an example after this patch and using the wrong state:
    $ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
    (monitor) dev br0 port eth3 grp 239.0.0.1 permanent
    $ bridge mdb del dev br0 port eth3 grp 239.0.0.1 temp
    (monitor) dev br0 port eth3 grp 239.0.0.1 permanent

    Note the state of the entry that got deleted is correct in the
    notification.

    Signed-off-by: Nikolay Aleksandrov
    Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb entires")
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     

27 Jul, 2015

1 commit


23 Jul, 2015

1 commit


16 Jul, 2015

1 commit

  • Since the mdb add/del code was introduced there have been 2 br_mdb_notify
    calls when doing br_mdb_add() resulting in 2 notifications on each add.

    Example:
    Command: bridge mdb add dev br0 port eth1 grp 239.0.0.1 permanent
    Before patch:
    root@debian:~# bridge monitor all
    [MDB]dev br0 port eth1 grp 239.0.0.1 permanent
    [MDB]dev br0 port eth1 grp 239.0.0.1 permanent

    After patch:
    root@debian:~# bridge monitor all
    [MDB]dev br0 port eth1 grp 239.0.0.1 permanent

    Signed-off-by: Nikolay Aleksandrov
    Fixes: cfd567543590 ("bridge: add support of adding and deleting mdb entries")
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     

14 Jul, 2015

2 commits

  • Conflicts:
    net/bridge/br_mdb.c

    Minor conflict in br_mdb.c, in 'net' we added a memset of the
    on-stack 'ip' variable whereas in 'net-next' we assign a new
    member 'vid'.

    Signed-off-by: David S. Miller

    David S. Miller
     
  • Until now all user mdb entries were added in vlan 0, this patch adds
    support to allow the user to specify the vlan for the entry.
    About the uapi change a hole in struct br_mdb_entry is used so the size
    and offsets are kept the same (verified with pahole and tested with older
    iproute2).

    Example:
    $ bridge mdb
    dev br0 port eth1 grp 239.0.0.1 permanent vlan 2000
    dev br0 port eth1 grp 239.0.0.1 permanent vlan 200
    dev br0 port eth1 grp 239.0.0.1 permanent

    Signed-off-by: Nikolay Aleksandrov
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     

11 Jul, 2015

1 commit


10 Jul, 2015

1 commit


09 Jul, 2015

2 commits

  • Since commit b0e9a30dd669 ("bridge: Add vlan id to multicast groups")
    there's a check in br_ip_equal() for a matching vlan id, but the mdb
    functions were not modified to use (or at least zero it) so when an
    entry was added it would have a garbage vlan id (from the local br_ip
    variable in __br_mdb_add/del) and this would prevent it from being
    matched and also deleted. So zero out the whole local ip var to protect
    ourselves from future changes and also to fix the current bug, since
    there's no vlan id support in the mdb uapi - use always vlan id 0.
    Example before patch:
    root@debian:~# bridge mdb add dev br0 port eth1 grp 239.0.0.1 permanent
    root@debian:~# bridge mdb
    dev br0 port eth1 grp 239.0.0.1 permanent
    root@debian:~# bridge mdb del dev br0 port eth1 grp 239.0.0.1 permanent
    RTNETLINK answers: Invalid argument

    After patch:
    root@debian:~# bridge mdb add dev br0 port eth1 grp 239.0.0.1 permanent
    root@debian:~# bridge mdb
    dev br0 port eth1 grp 239.0.0.1 permanent
    root@debian:~# bridge mdb del dev br0 port eth1 grp 239.0.0.1 permanent
    root@debian:~# bridge mdb

    Signed-off-by: Nikolay Aleksandrov
    Fixes: b0e9a30dd669 ("bridge: Add vlan id to multicast groups")
    Signed-off-by: David S. Miller

    Nikolay Aleksandrov
     
  • Start the delete timer when adding temp static entries so they can expire.

    Signed-off-by: Satish Ashok
    Signed-off-by: Nikolay Aleksandrov
    Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb entires")
    Signed-off-by: David S. Miller

    Satish Ashok
     

30 Apr, 2015

1 commit

  • NLM_F_MULTI must be used only when a NLMSG_DONE message is sent. In fact,
    it is sent only at the end of a dump.

    Libraries like libnl will wait forever for NLMSG_DONE.

    Fixes: 37a393bc4932 ("bridge: notify mdb changes via netlink")
    CC: Cong Wang
    CC: Stephen Hemminger
    CC: bridge@lists.linux-foundation.org
    Signed-off-by: Nicolas Dichtel
    Signed-off-by: David S. Miller

    Nicolas Dichtel
     

18 Jan, 2015

1 commit

  • Contrary to common expectations for an "int" return, these functions
    return only a positive value -- if used correctly they cannot even
    return 0 because the message header will necessarily be in the skb.

    This makes the very common pattern of

    if (genlmsg_end(...) < 0) { ... }

    be a whole bunch of dead code. Many places also simply do

    return nlmsg_end(...);

    and the caller is expected to deal with it.

    This also commonly (at least for me) causes errors, because it is very
    common to write

    if (my_function(...))
    /* error condition */

    and if my_function() does "return nlmsg_end()" this is of course wrong.

    Additionally, there's not a single place in the kernel that actually
    needs the message length returned, and if anyone needs it later then
    it'll be very easy to just use skb->len there.

    Remove this, and make the functions void. This removes a bunch of dead
    code as described above. The patch adds lines because I did

    - return nlmsg_end(...);
    + nlmsg_end(...);
    + return 0;

    I could have preserved all the function's return values by returning
    skb->len, but instead I've audited all the places calling the affected
    functions and found that none cared. A few places actually compared
    the return value with < 0 with no change in behaviour, so I opted for the more
    efficient version.

    One instance of the error I've made numerous times now is also present
    in net/phonet/pn_netlink.c in the route_dumpit() function - it didn't
    check for
    Signed-off-by: David S. Miller

    Johannes Berg
     

16 Jan, 2015

1 commit


11 Jun, 2014

1 commit

  • The current naming of these two structs is very random, in that
    reversing their naming would not make any semantical difference.

    This patch tries to make the naming less confusing by giving them a more
    specific, distinguishable naming.

    This is also useful for the upcoming patches reintroducing the
    "struct bridge_mcast_querier" but for storing information about the
    selected querier (no matter if our own or a foreign querier).

    Signed-off-by: Linus Lüssing
    Signed-off-by: David S. Miller

    Linus Lüssing
     

23 Oct, 2013

1 commit

  • While this commit was a good attempt to fix issues occuring when no
    multicast querier is present, this commit still has two more issues:

    1) There are cases where mdb entries do not expire even if there is a
    querier present. The bridge will unnecessarily continue flooding
    multicast packets on the according ports.

    2) Never removing an mdb entry could be exploited for a Denial of
    Service by an attacker on the local link, slowly, but steadily eating up
    all memory.

    Actually, this commit became obsolete with
    "bridge: disable snooping if there is no querier" (b00589af3b)
    which included fixes for a few more cases.

    Therefore reverting the following commits (the commit stated in the
    commit message plus three of its follow up fixes):

    ====================
    Revert "bridge: update mdb expiration timer upon reports."
    This reverts commit f144febd93d5ee534fdf23505ab091b2b9088edc.
    Revert "bridge: do not call setup_timer() multiple times"
    This reverts commit 1faabf2aab1fdaa1ace4e8c829d1b9cf7bfec2f1.
    Revert "bridge: fix some kernel warning in multicast timer"
    This reverts commit c7e8e8a8f7a70b343ca1e0f90a31e35ab2d16de1.
    Revert "bridge: only expire the mdb entry when query is received"
    This reverts commit 9f00b2e7cf241fa389733d41b615efdaa2cb0f5b.
    ====================

    CC: Cong Wang
    Signed-off-by: Linus Lüssing
    Reviewed-by: Vlad Yasevich
    Signed-off-by: David S. Miller

    Linus Lüssing
     

06 Sep, 2013

2 commits

  • Conflicts:
    drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
    net/bridge/br_multicast.c
    net/ipv6/sit.c

    The conflicts were minor:

    1) sit.c changes overlap with change to ip_tunnel_xmit() signature.

    2) br_multicast.c had an overlap between computing max_delay using
    msecs_to_jiffies and turning MLDV2_MRC() into an inline function
    with a name using lowercase instead of uppercase letters.

    3) stmmac had two overlapping changes, one which conditionally allocated
    and hooked up a dma_cfg based upon the presence of the pbl OF property,
    and another one handling store-and-forward DMA made. The latter of
    which should not go into the new of_find_property() basic block.

    Signed-off-by: David S. Miller

    David S. Miller
     
  • The multicast snooping code should have matured enough to be safely
    applicable to IPv6 link-local multicast addresses (excluding the
    link-local all nodes address, ff02::1), too.

    Signed-off-by: Linus Lüssing
    Signed-off-by: David S. Miller

    Linus Lüssing
     

31 Aug, 2013

1 commit

  • Currently we would still potentially suffer multicast packet loss if there
    is just either an IGMP or an MLD querier: For the former case, we would
    possibly drop IPv6 multicast packets, for the latter IPv4 ones. This is
    because we are currently assuming that if either an IGMP or MLD querier
    is present that the other one is present, too.

    This patch makes the behaviour and fix added in
    "bridge: disable snooping if there is no querier" (b00589af3b04)
    to also work if there is either just an IGMP or an MLD querier on the
    link: It refines the deactivation of the snooping to be protocol
    specific by using separate timers for the snooped IGMP and MLD queries
    as well as separate timers for our internal IGMP and MLD queriers.

    Signed-off-by: Linus Lüssing
    Signed-off-by: David S. Miller

    Linus Lüssing
     

05 Aug, 2013

1 commit


07 Jul, 2013

1 commit

  • Several people reported the warning: "kernel BUG at kernel/timer.c:729!"
    and the stack trace is:

    #7 [ffff880214d25c10] mod_timer+501 at ffffffff8106d905
    #8 [ffff880214d25c50] br_multicast_del_pg.isra.20+261 at ffffffffa0731d25 [bridge]
    #9 [ffff880214d25c80] br_multicast_disable_port+88 at ffffffffa0732948 [bridge]
    #10 [ffff880214d25cb0] br_stp_disable_port+154 at ffffffffa072bcca [bridge]
    #11 [ffff880214d25ce8] br_device_event+520 at ffffffffa072a4e8 [bridge]
    #12 [ffff880214d25d18] notifier_call_chain+76 at ffffffff8164aafc
    #13 [ffff880214d25d50] raw_notifier_call_chain+22 at ffffffff810858f6
    #14 [ffff880214d25d60] call_netdevice_notifiers+45 at ffffffff81536aad
    #15 [ffff880214d25d80] dev_close_many+183 at ffffffff81536d17
    #16 [ffff880214d25dc0] rollback_registered_many+168 at ffffffff81537f68
    #17 [ffff880214d25de8] rollback_registered+49 at ffffffff81538101
    #18 [ffff880214d25e10] unregister_netdevice_queue+72 at ffffffff815390d8
    #19 [ffff880214d25e30] __tun_detach+272 at ffffffffa074c2f0 [tun]
    #20 [ffff880214d25e88] tun_chr_close+45 at ffffffffa074c4bd [tun]
    #21 [ffff880214d25ea8] __fput+225 at ffffffff8119b1f1
    #22 [ffff880214d25ef0] ____fput+14 at ffffffff8119b3fe
    #23 [ffff880214d25f00] task_work_run+159 at ffffffff8107cf7f
    #24 [ffff880214d25f30] do_notify_resume+97 at ffffffff810139e1
    #25 [ffff880214d25f50] int_signal+18 at ffffffff8164f292

    this is due to I forgot to check if mp->timer is armed in
    br_multicast_del_pg(). This bug is introduced by
    commit 9f00b2e7cf241fa389733d41b6 (bridge: only expire the mdb entry
    when query is received).

    Same for __br_mdb_del().

    Tested-by: poma
    Reported-by: LiYonghua
    Reported-by: Robert Hancock
    Cc: Herbert Xu
    Cc: Stephen Hemminger
    Cc: "David S. Miller"
    Signed-off-by: Cong Wang
    Signed-off-by: David S. Miller

    Cong Wang
     

22 Mar, 2013

1 commit


10 Mar, 2013

1 commit

  • The bridging code discloses heap and stack bytes via the RTM_GETMDB
    netlink interface and via the notify messages send to group RTNLGRP_MDB
    afer a successful add/del.

    Fix both cases by initializing all unset members/padding bytes with
    memset(0).

    Cc: Stephen Hemminger
    Signed-off-by: Mathias Krause
    Signed-off-by: David S. Miller

    Mathias Krause
     

28 Feb, 2013

1 commit

  • I'm not sure why, but the hlist for each entry iterators were conceived

    list_for_each_entry(pos, head, member)

    The hlist ones were greedy and wanted an extra parameter:

    hlist_for_each_entry(tpos, pos, head, member)

    Why did they need an extra pos parameter? I'm not quite sure. Not only
    they don't really need it, it also prevents the iterator from looking
    exactly like the list iterator, which is unfortunate.

    Besides the semantic patch, there was some manual work required:

    - Fix up the actual hlist iterators in linux/list.h
    - Fix up the declaration of other iterators based on the hlist ones.
    - A very small amount of places were using the 'node' parameter, this
    was modified to use 'obj->member' instead.
    - Coccinelle didn't handle the hlist_for_each_entry_safe iterator
    properly, so those had to be fixed up manually.

    The semantic patch which is mostly the work of Peter Senna Tschudin is here:

    @@
    iterator name hlist_for_each_entry, hlist_for_each_entry_continue, hlist_for_each_entry_from, hlist_for_each_entry_rcu, hlist_for_each_entry_rcu_bh, hlist_for_each_entry_continue_rcu_bh, for_each_busy_worker, ax25_uid_for_each, ax25_for_each, inet_bind_bucket_for_each, sctp_for_each_hentry, sk_for_each, sk_for_each_rcu, sk_for_each_from, sk_for_each_safe, sk_for_each_bound, hlist_for_each_entry_safe, hlist_for_each_entry_continue_rcu, nr_neigh_for_each, nr_neigh_for_each_safe, nr_node_for_each, nr_node_for_each_safe, for_each_gfn_indirect_valid_sp, for_each_gfn_sp, for_each_host;

    type T;
    expression a,c,d,e;
    identifier b;
    statement S;
    @@

    -T b;

    [akpm@linux-foundation.org: drop bogus change from net/ipv4/raw.c]
    [akpm@linux-foundation.org: drop bogus hunk from net/ipv6/raw.c]
    [akpm@linux-foundation.org: checkpatch fixes]
    [akpm@linux-foundation.org: fix warnings]
    [akpm@linux-foudnation.org: redo intrusive kvm changes]
    Tested-by: Peter Senna Tschudin
    Acked-by: Paul E. McKenney
    Signed-off-by: Sasha Levin
    Cc: Wu Fengguang
    Cc: Marcelo Tosatti
    Cc: Gleb Natapov
    Signed-off-by: Andrew Morton
    Signed-off-by: Linus Torvalds

    Sasha Levin
     

05 Feb, 2013

1 commit


20 Dec, 2012

2 commits