[netifd] team: add simple bonding/teaming module

An initial version of a bonding/teaming module based on libteam and the
kernel team driver. It is capable of creating a team device and add
member ports. The ultimate goal is to support LAG offloaded to DSA
switch hardware with teamd handling the LACP management.

    config interface teamdev
        option ifname "lan1 lan2"
        option type team

The module requires libteam's teamd and teamdctl commands.

Signed-off-by: Pavel Šimerda <code@simerda.eu>
---
 CMakeLists.txt |   2 +-
 team.c         | 178 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+), 1 deletion(-)
 create mode 100644 team.c

Pavel Šimerda <code@simerda.eu> [2021-01-12 04:36:36]:

Hi,

> The module requires libteam's teamd and teamdctl commands.

This looks like an alien to the OpenWrt ecosystem, basically you're using
netifd just as a launcher for teamd, teamdctl without any proper error
handling instead of ubus for configuration etc.

> Signed-off-by: Pavel Šimerda <code@simerda.eu>
> ---
>  CMakeLists.txt |   2 +-
>  team.c         | 178 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 179 insertions(+), 1 deletion(-)
>  create mode 100644 team.c
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 9d19817..351e303 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -19,7 +19,7 @@ SET(SOURCES
>  	main.c utils.c system.c tunnel.c handler.c
>  	interface.c interface-ip.c interface-event.c
>  	iprule.c proto.c proto-static.c proto-shell.c
> -	config.c device.c bridge.c veth.c vlan.c alias.c
> +	config.c device.c bridge.c team.c veth.c vlan.c alias.c
>  	macvlan.c ubus.c vlandev.c wireless.c)
>  
>  
> diff --git a/team.c b/team.c
> new file mode 100644
> index 0000000..9b67566
> --- /dev/null
> +++ b/team.c
> @@ -0,0 +1,178 @@
> +/*
> + * netifd - network interface daemon
> + * Copyright (C) 2021 Pavel Šimerda <code@simerda.eu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <assert.h>
> +#include <errno.h>
> +#include <signal.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "netifd.h"
> +#include "device.h"
> +#include "interface.h"
> +#include "system.h"
> +
> +enum {
> +	TEAM_ATTR_IFNAME,
> +	__TEAM_ATTR_MAX
> +};
> +
> +static const struct blobmsg_policy team_attrs[__TEAM_ATTR_MAX] = {
> +	[TEAM_ATTR_IFNAME] = { "ifname", BLOBMSG_TYPE_ARRAY },
> +};
> +
> +static const struct uci_blob_param_info team_attr_info[__TEAM_ATTR_MAX] = {
> +	[TEAM_ATTR_IFNAME] = { .type = BLOBMSG_TYPE_STRING },
> +};
> +
> +static const struct uci_blob_param_list team_attr_list = {
> +	.n_params = __TEAM_ATTR_MAX,
> +	.params = team_attrs,
> +	.info = team_attr_info,
> +
> +	.n_next = 1,
> +	.next = { &device_attr_list },
> +};
> +
> +struct team_device {
> +	struct device dev;
> +	device_state_cb set_state;
> +
> +	struct blob_attr *config_data;
> +	struct blob_attr *ifnames;
> +
> +	int pid;
> +};
> +
> +static int
> +team_set_state(struct device *dev, bool up)
> +{
> +	struct team_device *teamdev = container_of(dev, struct team_device, dev);
> +
> +	if (up) {
> +		int pid;
> +		struct blob_attr *cur;
> +		int rem;
> +		char buffer[64];
> +
> +		printf("TEAM start teamd\n");

if this is needed at all, take a look around and use proper debug logging

> +		pid = fork();
> +		if (pid == -1)
> +			return -errno;
> +		if (pid == 0)
> +			execlp("teamd", "teamd", "-t", dev->ifname, NULL);

this is external dependency and you lack any check for that

> +		teamdev->pid = pid;
> +		// TODO: Better handling of newly created devices.

better? there is no handling of anything

> +		sleep(1);
> +		if (teamdev->ifnames) {
> +			printf("TEAM port init\n");
> +			blobmsg_for_each_attr(cur, teamdev->ifnames, rem) {
> +				printf("TEAM one port init\n");
> +				snprintf(buffer, sizeof buffer,
> +					"teamdctl %s port add %s",
> +					dev->ifname,
> +					blobmsg_get_string(cur));
> +				system(buffer);

puting aside usage of system() for service configuration this smells, you're
passing possibly malicious input directly to system() in the service running
as root, what could go wrong?

> +			}
> +		}
> +		teamdev->set_state(dev, up);
> +		return 0;
> +	} else {
> +		printf("TEAM: killing %d\n", teamdev->pid);
> +		if (teamdev->pid) {
> +			kill(teamdev->pid, SIGTERM);
> +			waitpid(teamdev->pid, NULL, 0);
> +			teamdev->pid = 0;
> +		}
> +		return 0;
> +	}
> +}
> +
> +static enum dev_change_type
> +team_reload(struct device *dev, struct blob_attr *attr)
> +{
> +	struct team_device *teamdev = container_of(dev, struct team_device, dev);
> +	struct blob_attr *tb_tm[__TEAM_ATTR_MAX];
> +
> +	attr = blob_memdup(attr);
> +
> +	blobmsg_parse(team_attrs, __TEAM_ATTR_MAX, tb_tm, blob_data(attr), blob_len(attr));
> +	teamdev->ifnames = tb_tm[TEAM_ATTR_IFNAME];
> +
> +	if (teamdev->pid) {
> +		// TODO: More fine-grained reconfiguration
> +		team_set_state(dev, false);
> +		team_set_state(dev, true);
> +	}
> +
> +	free(teamdev->config_data);
> +	teamdev->config_data = attr;
> +	return DEV_CONFIG_APPLIED;
> +}
> +
> +static struct device *
> +team_create(const char *name, struct device_type *devtype,
> +	struct blob_attr *attr)
> +{
> +	struct team_device *teamdev;
> +	struct device *dev = NULL;
> +
> +	teamdev = calloc(1, sizeof(*teamdev));
> +	if (!teamdev)
> +		return NULL;
> +	dev = &teamdev->dev;
> +
> +	if (device_init(dev, devtype, name) < 0) {
> +		device_cleanup(dev);
> +		free(teamdev);
> +		return NULL;
> +	}
> +
> +	teamdev->set_state = dev->set_state;
> +	dev->set_state = team_set_state;
> +
> +	device_set_present(dev, true);
> +	team_reload(dev, attr);
> +
> +	return dev;
> +}
> +
> +static void
> +team_free(struct device *dev)
> +{
> +	struct team_device *teamdev = container_of(dev, struct team_device, dev);
> +
> +	free(teamdev->config_data);
> +	free(teamdev);
> +}
> +
> +static struct device_type team_device_type = {
> +	.name = "team",
> +	.config_params = &team_attr_list,
> +
> +	.bridge_capability = true,
> +	.name_prefix = "tm",
> +
> +	.create = team_create,
> +	.reload = team_reload,
> +	.free = team_free,
> +};
> +
> +static void __init team_device_type_init(void)
> +{
> +	device_type_add(&team_device_type);
> +}
> -- 
> 2.29.2

On 1/16/21 1:55 PM, Petr Štetiar wrote:
> Pavel Šimerda <code@simerda.eu> [2021-01-12 04:36:36]:
> 
> Hi,
> 
>> The module requires libteam's teamd and teamdctl commands.
> 
> This looks like an alien to the OpenWrt ecosystem, basically you're using
> netifd just as a launcher for teamd, teamdctl without any proper error
> handling instead of ubus for configuration etc.

Hey Petr,

this is what I'm using right now to enable the hardware features that were *absent* in OpenWRT. Would you suggest that we keep a local fork for the time being, until someone is willing to invest their time into building a ubus wrapper for libteam?

Cheers,

Pavel Šimerda

> 
>> Signed-off-by: Pavel Šimerda <code@simerda.eu>
>> ---
>>   CMakeLists.txt |   2 +-
>>   team.c         | 178 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 179 insertions(+), 1 deletion(-)
>>   create mode 100644 team.c
>>
>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>> index 9d19817..351e303 100644
>> --- a/CMakeLists.txt
>> +++ b/CMakeLists.txt
>> @@ -19,7 +19,7 @@ SET(SOURCES
>>   	main.c utils.c system.c tunnel.c handler.c
>>   	interface.c interface-ip.c interface-event.c
>>   	iprule.c proto.c proto-static.c proto-shell.c
>> -	config.c device.c bridge.c veth.c vlan.c alias.c
>> +	config.c device.c bridge.c team.c veth.c vlan.c alias.c
>>   	macvlan.c ubus.c vlandev.c wireless.c)
>>   
>>   
>> diff --git a/team.c b/team.c
>> new file mode 100644
>> index 0000000..9b67566
>> --- /dev/null
>> +++ b/team.c
>> @@ -0,0 +1,178 @@
>> +/*
>> + * netifd - network interface daemon
>> + * Copyright (C) 2021 Pavel Šimerda <code@simerda.eu>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2
>> + * as published by the Free Software Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <assert.h>
>> +#include <errno.h>
>> +#include <signal.h>
>> +#include <sys/wait.h>
>> +#include <unistd.h>
>> +
>> +#include "netifd.h"
>> +#include "device.h"
>> +#include "interface.h"
>> +#include "system.h"
>> +
>> +enum {
>> +	TEAM_ATTR_IFNAME,
>> +	__TEAM_ATTR_MAX
>> +};
>> +
>> +static const struct blobmsg_policy team_attrs[__TEAM_ATTR_MAX] = {
>> +	[TEAM_ATTR_IFNAME] = { "ifname", BLOBMSG_TYPE_ARRAY },
>> +};
>> +
>> +static const struct uci_blob_param_info team_attr_info[__TEAM_ATTR_MAX] = {
>> +	[TEAM_ATTR_IFNAME] = { .type = BLOBMSG_TYPE_STRING },
>> +};
>> +
>> +static const struct uci_blob_param_list team_attr_list = {
>> +	.n_params = __TEAM_ATTR_MAX,
>> +	.params = team_attrs,
>> +	.info = team_attr_info,
>> +
>> +	.n_next = 1,
>> +	.next = { &device_attr_list },
>> +};
>> +
>> +struct team_device {
>> +	struct device dev;
>> +	device_state_cb set_state;
>> +
>> +	struct blob_attr *config_data;
>> +	struct blob_attr *ifnames;
>> +
>> +	int pid;
>> +};
>> +
>> +static int
>> +team_set_state(struct device *dev, bool up)
>> +{
>> +	struct team_device *teamdev = container_of(dev, struct team_device, dev);
>> +
>> +	if (up) {
>> +		int pid;
>> +		struct blob_attr *cur;
>> +		int rem;
>> +		char buffer[64];
>> +
>> +		printf("TEAM start teamd\n");
> 
> if this is needed at all, take a look around and use proper debug logging
> 
>> +		pid = fork();
>> +		if (pid == -1)
>> +			return -errno;
>> +		if (pid == 0)
>> +			execlp("teamd", "teamd", "-t", dev->ifname, NULL);
> 
> this is external dependency and you lack any check for that
> 
>> +		teamdev->pid = pid;
>> +		// TODO: Better handling of newly created devices.
> 
> better? there is no handling of anything
> 
>> +		sleep(1);
>> +		if (teamdev->ifnames) {
>> +			printf("TEAM port init\n");
>> +			blobmsg_for_each_attr(cur, teamdev->ifnames, rem) {
>> +				printf("TEAM one port init\n");
>> +				snprintf(buffer, sizeof buffer,
>> +					"teamdctl %s port add %s",
>> +					dev->ifname,
>> +					blobmsg_get_string(cur));
>> +				system(buffer);
> 
> puting aside usage of system() for service configuration this smells, you're
> passing possibly malicious input directly to system() in the service running
> as root, what could go wrong?
> 
>> +			}
>> +		}
>> +		teamdev->set_state(dev, up);
>> +		return 0;
>> +	} else {
>> +		printf("TEAM: killing %d\n", teamdev->pid);
>> +		if (teamdev->pid) {
>> +			kill(teamdev->pid, SIGTERM);
>> +			waitpid(teamdev->pid, NULL, 0);
>> +			teamdev->pid = 0;
>> +		}
>> +		return 0;
>> +	}
>> +}
>> +
>> +static enum dev_change_type
>> +team_reload(struct device *dev, struct blob_attr *attr)
>> +{
>> +	struct team_device *teamdev = container_of(dev, struct team_device, dev);
>> +	struct blob_attr *tb_tm[__TEAM_ATTR_MAX];
>> +
>> +	attr = blob_memdup(attr);
>> +
>> +	blobmsg_parse(team_attrs, __TEAM_ATTR_MAX, tb_tm, blob_data(attr), blob_len(attr));
>> +	teamdev->ifnames = tb_tm[TEAM_ATTR_IFNAME];
>> +
>> +	if (teamdev->pid) {
>> +		// TODO: More fine-grained reconfiguration
>> +		team_set_state(dev, false);
>> +		team_set_state(dev, true);
>> +	}
>> +
>> +	free(teamdev->config_data);
>> +	teamdev->config_data = attr;
>> +	return DEV_CONFIG_APPLIED;
>> +}
>> +
>> +static struct device *
>> +team_create(const char *name, struct device_type *devtype,
>> +	struct blob_attr *attr)
>> +{
>> +	struct team_device *teamdev;
>> +	struct device *dev = NULL;
>> +
>> +	teamdev = calloc(1, sizeof(*teamdev));
>> +	if (!teamdev)
>> +		return NULL;
>> +	dev = &teamdev->dev;
>> +
>> +	if (device_init(dev, devtype, name) < 0) {
>> +		device_cleanup(dev);
>> +		free(teamdev);
>> +		return NULL;
>> +	}
>> +
>> +	teamdev->set_state = dev->set_state;
>> +	dev->set_state = team_set_state;
>> +
>> +	device_set_present(dev, true);
>> +	team_reload(dev, attr);
>> +
>> +	return dev;
>> +}
>> +
>> +static void
>> +team_free(struct device *dev)
>> +{
>> +	struct team_device *teamdev = container_of(dev, struct team_device, dev);
>> +
>> +	free(teamdev->config_data);
>> +	free(teamdev);
>> +}
>> +
>> +static struct device_type team_device_type = {
>> +	.name = "team",
>> +	.config_params = &team_attr_list,
>> +
>> +	.bridge_capability = true,
>> +	.name_prefix = "tm",
>> +
>> +	.create = team_create,
>> +	.reload = team_reload,
>> +	.free = team_free,
>> +};
>> +
>> +static void __init team_device_type_init(void)
>> +{
>> +	device_type_add(&team_device_type);
>> +}
>> -- 
>> 2.29.2