netdev - Re: [patch net-next v2 07/12] mlxsw: spectrum: Add the multicast routing offloading logic

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: <5e6ba4b6-66c0-b0c8-e786-c734fb9131c6@mellanox.com>
Date:   Mon, 25 Sep 2017 13:53:08 +0300
From:   Yotam Gigi <yotamg@...lanox.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, idosch@...lanox.com, mlxsw@...lanox.com,
        andrew@...n.ch
Subject: Re: [patch net-next v2 07/12] mlxsw: spectrum: Add the multicast
 routing offloading logic

On 09/25/2017 01:40 PM, Nikolay Aleksandrov wrote:
> On 24/09/17 20:22, Jiri Pirko wrote:
>> From: Yotam Gigi <yotamg@...lanox.com>
>>
>> Add the multicast router offloading logic, which is in charge of handling
>> the VIF and MFC notifications and translating it to the hardware logic API.
>>
>> The offloading logic has to overcome several obstacles in order to safely
>> comply with the kernel multicast router user API:
>>  - It must keep track of the mapping between VIFs to netdevices. The user
>>    can add an MFC cache entry pointing to a VIF, delete the VIF and add
>>    re-add it with a different netdevice. The offloading logic has to handle
>>    this in order to be compatible with the kernel logic.
>>  - It must keep track of the mapping between netdevices to spectrum RIFs,
>>    as the current hardware implementation assume having a RIF for every
>>    port in a multicast router.
>>  - It must handle routes pointing to pimreg device to be trapped to the
>>    kernel, as the packet should be delivered to userspace.
>>  - It must handle routes pointing tunnel VIFs. The current implementation
>>    does not support multicast forwarding to tunnels, thus routes that point
>>    to a tunnel should be trapped to the kernel.
>>  - It must be aware of proxy multicast routes, which include both (*,*)
>>    routes and duplicate routes. Currently proxy routes are not offloaded
>>    and trigger the abort mechanism: removal of all routes from hardware and
>>    triggering the traffic to go through the kernel.
>>
>> The multicast routing offloading logic also updates the counters of the
>> offloaded MFC routes in a periodic work.
>>
>> Signed-off-by: Yotam Gigi <yotamg@...lanox.com>
>> Reviewed-by: Ido Schimmel <idosch@...lanox.com>
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>> ---
>> v1->v2:
>>  - Update the lastuse MFC entry field too, in addition to packets an bytes.
>> ---
>>  drivers/net/ethernet/mellanox/mlxsw/Makefile      |    3 +-
>>  drivers/net/ethernet/mellanox/mlxsw/spectrum.h    |    1 +
>>  drivers/net/ethernet/mellanox/mlxsw/spectrum_mr.c | 1014 +++++++++++++++++++++
>>  drivers/net/ethernet/mellanox/mlxsw/spectrum_mr.h |  133 +++
>>  4 files changed, 1150 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/net/ethernet/mellanox/mlxsw/spectrum_mr.c
>>  create mode 100644 drivers/net/ethernet/mellanox/mlxsw/spectrum_mr.h
>>
> [snip]
>> +static void mlxsw_sp_mr_route_erase(struct mlxsw_sp_mr_table *mr_table,
>> +				    struct mlxsw_sp_mr_route *mr_route)
>> +{
>> +	struct mlxsw_sp *mlxsw_sp = mr_table->mlxsw_sp;
>> +	struct mlxsw_sp_mr *mr = mlxsw_sp->mr;
>> +
>> +	mr->mr_ops->route_destroy(mlxsw_sp, mr->priv, mr_route->route_priv);
>> +	kfree(mr_route->route_priv);
>> +}
>> +
>> +static struct mlxsw_sp_mr_route *
>> +mlxsw_sp_mr_route4_create(struct mlxsw_sp_mr_table *mr_table,
>> +			  struct mfc_cache *mfc)
>> +{
>> +	struct mlxsw_sp_mr_route_vif_entry *rve, *tmp;
>> +	struct mlxsw_sp_mr_route *mr_route;
>> +	int err;
>> +	int i;
>> +
>> +	/* Allocate and init a new route and fill it with parameters */
>> +	mr_route = kzalloc(sizeof(*mr_table), GFP_KERNEL);
> sizeof(*mr_table) ? Shouldn't you allocate sizeof struct mlsw_sp_mr_route (*mr_route) here ?
>

Seems like you are right. Because of the fact that sizeof(*mr_table) is much
bigger than sizeof(*mr_route), all our tests did not notice it.

Thanks for that!

>> +	if (!mr_route)
>> +		return ERR_PTR(-ENOMEM);
>> +	INIT_LIST_HEAD(&mr_route->evif_list);
>> +	mlxsw_sp_mr_route4_key(mr_table, &mr_route->key, mfc);
>> +
>> +	/* Find min_mtu and link iVIF and eVIFs */
>> +	mr_route->min_mtu = ETH_MAX_MTU;
>> +	ipmr_cache_hold(mfc);
>> +	mr_route->mfc4 = mfc;
>> +	mr_route->mr_table = mr_table;
>> +	for (i = 0; i < MAXVIFS; i++) {
>> +		if (mfc->mfc_un.res.ttls[i] != 255) {
>> +			err = mlxsw_sp_mr_route_evif_link(mr_route,
>> +							  &mr_table->vifs[i]);
>> +			if (err)
>> +				goto err;
>> +			if (mr_table->vifs[i].dev &&
>> +			    mr_table->vifs[i].dev->mtu < mr_route->min_mtu)
>> +				mr_route->min_mtu = mr_table->vifs[i].dev->mtu;
>> +		}
>> +	}
>> +	mlxsw_sp_mr_route_ivif_link(mr_route, &mr_table->vifs[mfc->mfc_parent]);
>> +	if (err)
>> +		goto err;
>> +
>> +	mr_route->route_action = mlxsw_sp_mr_route_action(mr_route);
>> +	return mr_route;
>> +err:
>> +	ipmr_cache_put(mfc);
>> +	list_for_each_entry_safe(rve, tmp, &mr_route->evif_list, route_node)
>> +		mlxsw_sp_mr_route_evif_unlink(rve);
>> +	kfree(mr_route);
>> +	return ERR_PTR(err);
>> +}
>> +
>> +static void mlxsw_sp_mr_route4_destroy(struct mlxsw_sp_mr_table *mr_table,
>> +				       struct mlxsw_sp_mr_route *mr_route)
>> +{
>> +	struct mlxsw_sp_mr_route_vif_entry *rve, *tmp;
>> +
>> +	mlxsw_sp_mr_route_ivif_unlink(mr_route);
>> +	ipmr_cache_put(mr_route->mfc4);
>> +	list_for_each_entry_safe(rve, tmp, &mr_route->evif_list, route_node)
>> +		mlxsw_sp_mr_route_evif_unlink(rve);
>> +	kfree(mr_route);
>> +}
> [snip]
>