netdev - Re: [PATCH net-next 6/6] mlxsw: spectrum_span: Allow bridge for gretap mirror

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]

Message-ID: <20180426085833.2fbcd276@xeon-e3>
Date:   Thu, 26 Apr 2018 08:58:33 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     Ido Schimmel <idosch@...lanox.com>, netdev@...r.kernel.org,
        bridge@...ts.linux-foundation.org, davem@...emloft.net,
        jiri@...lanox.com, petrm@...lanox.com, mlxsw@...lanox.com
Subject: Re: [PATCH net-next 6/6] mlxsw: spectrum_span: Allow bridge for
 gretap mirror

On Thu, 26 Apr 2018 13:50:12 +0300
Nikolay Aleksandrov <nikolay@...ulusnetworks.com> wrote:

> On 26/04/18 12:06, Ido Schimmel wrote:
> > From: Petr Machata <petrm@...lanox.com>
> > 
> > When handling mirroring to a gretap or ip6gretap netdevice in mlxsw, the
> > underlay address (i.e. the remote address of the tunnel) may be routed
> > to a bridge.
> > 
> > In that case, look up the resolved neighbor Ethernet address in that
> > bridge's FDB. Then configure the offload to direct the mirrored traffic
> > to that port, possibly with tagging.
> > 
> > Signed-off-by: Petr Machata <petrm@...lanox.com>
> > Signed-off-by: Ido Schimmel <idosch@...lanox.com>
> > ---
> >   .../net/ethernet/mellanox/mlxsw/spectrum_span.c    | 102 +++++++++++++++++++--
> >   .../net/ethernet/mellanox/mlxsw/spectrum_span.h    |   1 +
> >   2 files changed, 97 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
> > index 65a77708ff61..92fb81839852 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
> > @@ -32,6 +32,7 @@
> >    * POSSIBILITY OF SUCH DAMAGE.
> >    */
> >   
> > +#include <linux/if_bridge.h>
> >   #include <linux/list.h>
> >   #include <net/arp.h>
> >   #include <net/gre.h>
> > @@ -39,8 +40,9 @@
> >   #include <net/ip6_tunnel.h>
> >   
> >   #include "spectrum.h"
> > -#include "spectrum_span.h"
> >   #include "spectrum_ipip.h"
> > +#include "spectrum_span.h"
> > +#include "spectrum_switchdev.h"
> >   
> >   int mlxsw_sp_span_init(struct mlxsw_sp *mlxsw_sp)
> >   {
> > @@ -167,6 +169,79 @@ mlxsw_sp_span_entry_unoffloadable(struct mlxsw_sp_span_parms *sparmsp)
> >   	return 0;
> >   }
> >   
> > +static struct net_device *
> > +mlxsw_sp_span_entry_bridge_8021q(const struct net_device *br_dev,
> > +				 unsigned char *dmac,
> > +				 u16 *p_vid)
> > +{
> > +	struct net_bridge_vlan_group *vg = br_vlan_group_rtnl(br_dev);
> > +	u16 pvid = br_vlan_group_pvid(vg);
> > +	struct net_device *edev = NULL;
> > +	struct net_bridge_vlan *v;
> > +  
> 
> Hi,
> These structures are not really exported anywhere outside of br_private.h
> Instead of passing them around and risking someone else actually trying to
> dereference an incomplete type, why don't you add just 2 new helpers -
> br_vlan_pvid(netdevice) and br_vlan_info(netdevice, vid, *bridge_vlan_info)
> 
> br_vlan_info can return the exported and already available type "bridge_vlan_info"
> (defined in uapi/linux/if_bridge.h) into the bridge_vlan_info arg
> and br_vlan_pvid is obvious - return the current dev pvid if available.
> 
> Another bonus you'll avoid dealing with the bridge's vlan internals.

I am particularly worried that any locking changes will impact the device
driver. Also lock inversion issues, and implied (or not RTNL).
Also what if a value is changed, there is no notification path back to the device.

Having an API that only takes net_device and vlan seems preferable.