[PATCH netifd] team: add simple bonding/teaming module

[Petr Štetiar]

Pavel Šimerda <code at 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 at 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 at 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