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

Message-ID: <wihh8nyunlk.fsf@dev-r-vrt-156.mtr.labs.mlnx>
Date:   Thu, 26 Apr 2018 16:03:03 +0300
From:   Petr Machata <petrm@...lanox.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     Ido Schimmel <idosch@...lanox.com>, netdev@...r.kernel.org,
        bridge@...ts.linux-foundation.org, davem@...emloft.net,
        stephen@...workplumber.org, jiri@...lanox.com, mlxsw@...lanox.com
Subject: Re: [PATCH net-next 6/6] mlxsw: spectrum_span: Allow bridge for gretap mirror

Nikolay Aleksandrov <nikolay@...ulusnetworks.com> writes:

> On 26/04/18 12:06, Ido Schimmel wrote:
>> From: Petr Machata <petrm@...lanox.com>
>>
>> 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
>> @@ -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.

All right, I'll do it like that.

>
>> +	if (pvid)
>> +		edev = br_fdb_find_port_hold(br_dev, dmac, pvid);
>> +	if (!edev)
>> +		return NULL;
>> +
>> +	/* RTNL prevents edev from being removed. */
>> +	dev_put(edev);
>
> Minor point here - since this is always called in rtnl, the dev_hold/put dance isn't needed,
> unless you plan to use the fdb_find_port_hold outside of rtnl of course. In case that is
> not planned why don't you do br_fdb_find_port_rtnl() instead with RCU inside (thus not blocking
> learning) and ASSERT_RTNL() to avoid some complexity ?

OK, sounds good.

I'll spin a v2.

Thanks,
Petr