Re: [patch libteam] ifindex 0 is invalid so do not process it.
by Jiri Pirko
Wed, Sep 28, 2022 at 12:00:18PM CEST, pauldsmith(a)microsoft.com wrote:
>Signed-off-by: "Paul D.Smith" <pauldsmith(a)microsoft.com>
>
>On some platforms, libraries underpinning libteam assert that an ifindex
>passed to them is not zero so an ifindex of zero cannot be passed down.
>This patch alters libteam processing order to ensure that this is the
>case.
Placed signed off line here where it belongs, adjusted subject a bit and
applied.
Thanks!
>---
> binding/python/team/core.py | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/binding/python/team/core.py b/binding/python/team/core.py
>index 54161bf..b8569f9 100644
>--- a/binding/python/team/core.py
>+++ b/binding/python/team/core.py
>@@ -101,7 +101,8 @@ class TeamNetDevice(object):
> @ifindex.setter
> def ifindex(self, ifindex):
> self._ifindex = ifindex
>- self.ifname = self._conv.dev_ifname(ifindex)
>+ if self.ifindex:
>+ self.ifname = self._conv.dev_ifname(ifindex)
>
> def get_hwaddr(self):
> err, hwaddr = capi.team_hwaddr_get(self._th, self.ifindex, 6)
>--
>2.37.3.windows.1
>
1 year, 6 months
Re: [PATCH 1/2] teamd: stop iterating callbacks when a loop restart
is requested
by Jiri Pirko
Wed, Aug 31, 2022 at 04:44:02PM CEST, lkundrak(a)v3.sk wrote:
>A crash was observed:
>
> Added loop callback: dbus_watch, 0x560d279e4530
> Added loop callback: dbus_watch, 0x560d279e4580
> ...
> Removed loop callback: dbus_watch, 0x560d279e4580
> Removed loop callback: dbus_watch, 0x560d279e4530
> Aug 31 11:54:11 holaprdel kernel: traps: teamd[557] general protection fault ip:560d26469a55 sp:7ffd43ca9650 error:0 in teamd[560d26463000+16000]
>
>Traceback (from a different run than above):
>
> Core was generated by `/usr/bin/teamd -o -n -U -D -N -t team0 -gg'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0 0x00005600ac090a55 in teamd_run_loop_do_callbacks (ctx=0x5600ad9bac70, fds=0x7fff40861250, lcb_list=0x5600ad9bad58) at /usr/src/debug/libteam-1.31-14.el9.x86_64/teamd/teamd.c:321
> 321 list_for_each_node_entry_safe(lcb, tmp, lcb_list, list) {
> (gdb) bt
> #0 0x00005600ac090a55 in teamd_run_loop_do_callbacks (ctx=0x5600ad9bac70, fds=0x7fff40861250, lcb_list=0x5600ad9bad58) at /usr/src/debug/libteam-1.31-14.el9.x86_64/teamd/teamd.c:321
> #1 teamd_run_loop_run (ctx=0x5600ad9bac70) at /usr/src/debug/libteam-1.31-14.el9.x86_64/teamd/teamd.c:415
> #2 0x00005600ac08d8cb in teamd_start (p_ret=<synthetic pointer>, ctx=<optimized out>) at /usr/src/debug/libteam-1.31-14.el9.x86_64/teamd/teamd.c:1557
> #3 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/libteam-1.31-14.el9.x86_64/teamd/teamd.c:1876
> (gdb) print *lcb
> Cannot access memory at address 0x9dd29944fb7e97fc
> (gdb) print *tmp
> Cannot access memory at address 0x9dd29944fb7e9804
> (gdb)
>
>What has happened is that libdbus called the remove_watch callback twice
>in a single go, causing two callbacks being destroyed in one iteration
>of teamd_run_loop_do_callbacks()'s list_for_each_node_entry_safe().
>
>This basically turns the _safe() variant of the macro unhelpful, as tmp
>points to stale data anyway.
>
>Let's use the unsafe variant then, and terminate the loop once
>teamd_loop_callback_del() asks for main loop's attention via
>teamd_run_loop_restart(). If there are other callbacks pending an action,
>they will get their turn in the next main loop iteration.
>
>Signed-off-by: Lubomir Rintel <lkundrak(a)v3.sk>
>---
> teamd/teamd.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
>diff --git a/teamd/teamd.c b/teamd/teamd.c
>index b310140..1a6dfb3 100644
>--- a/teamd/teamd.c
>+++ b/teamd/teamd.c
>@@ -309,16 +309,28 @@ static void teamd_run_loop_set_fds(struct list_item *lcb_list,
> }
> }
>
>+static int teamd_check_ctrl(struct teamd_context *ctx)
>+{
>+ int ctrl_fd = ctx->run_loop.ctrl_pipe_r;
>+ struct timeval tv;
>+ fd_set rfds;
>+
>+ FD_ZERO(&rfds);
>+ FD_SET(ctrl_fd, &rfds);
>+ tv.tv_sec = tv.tv_usec = 0;
>+
>+ return select(ctrl_fd + 1, &rfds, NULL, NULL, &tv);
Okay, I don't like the select on ctrl pipe here so much, but on the
other hand, I didn't find any better solution. :)
I would like to see the ctrl pipe handling only in the main loop. But
cannot find how to do it in non-cumbersome way.
>+}
>+
> static int teamd_run_loop_do_callbacks(struct list_item *lcb_list, fd_set *fds,
> struct teamd_context *ctx)
> {
> struct teamd_loop_callback *lcb;
>- struct teamd_loop_callback *tmp;
> int i;
> int events;
> int err;
>
>- list_for_each_node_entry_safe(lcb, tmp, lcb_list, list) {
>+ list_for_each_node_entry(lcb, lcb_list, list) {
> for (i = 0; i < 3; i++) {
> if (!(lcb->fd_event & (1 << i)))
> continue;
>@@ -339,6 +351,14 @@ static int teamd_run_loop_do_callbacks(struct list_item *lcb_list, fd_set *fds,
> teamd_log_dbg(ctx, "Failed loop callback: %s, %p",
> lcb->name, lcb->priv);
> }
>+
>+ /* If there's a control byte ready, it's possible that one
>+ * or more entries have been removed from the callback
>+ * list and restart has been requested. In any case, let the
>+ * main loop deal with it first so that we know we're safe
>+ * to proceed. */
Nit: Please follow the libteam comment format:
/*
* something
* something
*/
Also, please stay within 80cols.
>+ if (teamd_check_ctrl(ctx))
>+ return 0;
> }
> }
> return 0;
>--
>2.37.2
>
1 year, 6 months
Re: [PATCH] teamd: Include missing headers for strrchr and memcmp
by Jiri Pirko
Tue, Sep 27, 2022 at 03:20:18AM CEST, raj.khem(a)gmail.com wrote:
>Compiler does not see the prototype for these functions otherwise and
>build fails e.g.
>
>| ../../git/teamd/teamd_phys_port_check.c:52:10: error: call to undeclared library function 'strrchr' with type 'char *(const char *, int)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>| start = strrchr(link, '/');
>| ^
>
>Signed-off-by: Khem Raj <raj.khem(a)gmail.com>
Applied, thanks!
1 year, 6 months
Re: [patch libteam] ifindex 0 is invalid so do not process it.
by Jiri Pirko
Tue, Jul 05, 2022 at 10:10:55AM CEST, pauldsmith(a)microsoft.com wrote:
>From 89914e67018bdb8dfed5db1f7ddf7f39c7d1a493 Mon Sep 17 00:00:00 2001
>From: "Paul D.Smith" paul.d.smith(a)metaswitch.com<mailto:paul.d.smith@metaswitch.com>
>Date: Thu, 21 Oct 2021 17:28:01 +0100
>Subject: [PATCH] ifindex zero cannot identify interface.
This is very odd. Next time, could you please send git-send-email?
Thanks!
Applied with manual changes to fix the email formatting.
>
>---
>binding/python/team/core.py | 3 ++-
>1 file changed, 2 insertions(+), 1 deletion(-)
>
>On some platforms, libraries underpinning libteam assert that an ifindex passed
>to them is not zero so an ifindex of zero cannot be passed down. Patch alters
>libteam processing order to ensure that this is the case.
>
>diff --git a/binding/python/team/core.py b/binding/python/team/core.py
>index 54161bf..b2fc5e8 100644
>--- a/binding/python/team/core.py
>+++ b/binding/python/team/core.py
>@@ -101,7 +101,8 @@ class TeamNetDevice(object):
> @ifindex.setter
> def ifindex(self, ifindex):
> self._ifindex = ifindex
>- self.ifname = self._conv.dev_ifname(ifindex)
>+ if ifindex:
>+ self.ifname = self._conv.dev_ifname(ifindex)
>
> def get_hwaddr(self):
> err, hwaddr = capi.team_hwaddr_get(self._th, self.ifindex, 6)
>--
>2.34.0.rc0
>
1 year, 6 months
Re: [PATCH] teamd: make -NN not flush the ports
by Jiri Pirko
Thu, Sep 01, 2022 at 12:55:46PM CEST, lkundrak(a)v3.sk wrote:
>Add a possibility to terminate while leaving the team and the port
>intact. This is useful when running from NetworkManager which keeps
>tracks of the ports themselves.
>
>There's one important use case where this is essential: handover from
>initrd to real root. Systemd's isolation of swithch-root.target stops
>NetworkManager.service and then terminates its kids, including teamd.
>The real NetworkManager.service would eventually catch up and restart
>it, but there's a short period when team ports are removed which is not
>great if we're booting off that device. Also, it may be that ports come
>up in different order, causing team to get a different MAC address,
>which will invalidate the DHCP lease we got beforehands and screwing up
>L3 addressing.
>
>Let's also not add new option for this purpose, to save poor
>NetworkManager from unpleasantness of determining whether teamd supports
>it. The worst that could happen, with new NetworkManager and old teamd,
>is that things will behave the the way they always did.
>
>Related NetworkManager change:
>https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requ...
>
>Signed-off-by: Lubomir Rintel <lkundrak(a)v3.sk>
>---
> man/teamd.8 | 1 +
> teamd/teamd.c | 16 +++++++++++-----
> teamd/teamd.h | 2 +-
> 3 files changed, 13 insertions(+), 6 deletions(-)
>
>diff --git a/man/teamd.8 b/man/teamd.8
>index 3d27eae..f99285c 100644
>--- a/man/teamd.8
>+++ b/man/teamd.8
>@@ -80,6 +80,7 @@ Take over the device if it already exists.
> .TP
> .B "\-N, \-\-no-quit-destroy"
> This option also ensures that the team device is not removed after teamd finishes.
>+Repeating the option causes the ports to stay in place.
> .TP
> .BI "\-t " devicename ", \-\-team-dev " devicename
> Use the specified team device name (overrides "device" key in the configuration).
>diff --git a/teamd/teamd.c b/teamd/teamd.c
>index b310140..8379f23 100644
>--- a/teamd/teamd.c
>+++ b/teamd/teamd.c
>@@ -112,6 +112,7 @@ static void print_help(const struct teamd_context *ctx) {
> " already exists\n"
> " -o --take-over Take over the device if it already exists\n"
> " -N --no-quit-destroy Do not destroy the device on quit\n"
>+ " Also leave ports in place if used multiple times\n"
> " -t --team-dev=DEVNAME Use the specified team device\n"
> " -n --no-ports Start without ports\n"
> " -D --dbus-enable Enable D-Bus interface\n"
>@@ -204,7 +205,7 @@ static int parse_command_line(struct teamd_context *ctx,
> ctx->take_over = true;
> break;
> case 'N':
>- ctx->no_quit_destroy = true;
>+ ctx->no_quit_destroy++;
> break;
> case 't':
> free(ctx->team_devname);
>@@ -346,10 +347,13 @@ static int teamd_run_loop_do_callbacks(struct list_item *lcb_list, fd_set *fds,
>
> static int teamd_flush_ports(struct teamd_context *ctx)
> {
>- if (!ctx->no_quit_destroy)
>+ switch (ctx->no_quit_destroy) {
>+ case 0:
> return teamd_port_remove_all(ctx);
>- else
>+ case 1:
> teamd_port_obj_remove_all(ctx);
This is not correct. You have to do a proper cleanup. Avoiding this call
avoids the cleanup.
Actually, I think this is rather a matter of fixing a bug in
no_quit_destroy implementation. Because the reason for calling
teamd_port_obj_remove_all() is to actually do a cleanup in case of
no_quit_destroy, but incidentally the teamd_port_remove() is called
during it. Perhaps, only remove of call to teamd_port_remove(ctx, tdport)
from port_obj_remove() would fix your problem.
>+ /* If 2 or higher, don't remove any ports at all. */
>+ }
> return 0;
> }
>
>@@ -370,8 +374,10 @@ static int teamd_run_loop_run(struct teamd_context *ctx)
> */
>
> while (true) {
>- if (quit_in_progress && !teamd_has_ports(ctx))
>- return ctx->run_loop.err;
>+ if (quit_in_progress) {
>+ if (!teamd_has_ports(ctx) || ctx->no_quit_destroy > 1)
>+ return ctx->run_loop.err;
>+ }
>
> for (i = 0; i < 3; i++)
> FD_ZERO(&fds[i]);
>diff --git a/teamd/teamd.h b/teamd/teamd.h
>index f94c918..262634c 100644
>--- a/teamd/teamd.h
>+++ b/teamd/teamd.h
>@@ -102,7 +102,7 @@ struct teamd_context {
> char * log_output;
> bool force_recreate;
> bool take_over;
>- bool no_quit_destroy;
>+ unsigned int no_quit_destroy;
> bool init_no_ports;
> bool pre_add_ports;
> char * config_file;
>--
>2.37.2
>
1 year, 6 months
[PATCH] libteamdctl: validate the bus name before using it
by Xin Long
Using bus name without validating it will cause core dump generated,
and it can be reproduced by:
# ip link add dummy0.1 type dummy
# teamdctl dummy0.1 state dump
This is normally a bug in some application using the D-Bus library.
D-Bus not built with -rdynamic so unable to print a backtrace
Aborted (core dumped)
Doing this many times can even create too many core files, customers
may complain about it.
This is triggered when calling cli_method_call("ConfigDump") in
cli_init(), so fix it by returning err in cli->init/cli_dbus_init()
if the bus name fails to validate.
Note this is safe, as with dbus, we can't use invalid dbus name to
create the team dev either.
Fixes: d8163e34c25c ("libteamdctl: do test method call instead or Introspect call")
Reported-by: Uday Patel <upatel(a)redhat.com>
Signed-off-by: Xin Long <lucien.xin(a)gmail.com>
---
libteamdctl/cli_dbus.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/libteamdctl/cli_dbus.c b/libteamdctl/cli_dbus.c
index dfef5c4..242ef86 100644
--- a/libteamdctl/cli_dbus.c
+++ b/libteamdctl/cli_dbus.c
@@ -183,12 +183,17 @@ static int cli_dbus_init(struct teamdctl *tdc, const char *team_name, void *priv
if (ret == -1)
return -errno;
+ err = -EINVAL;
dbus_error_init(&error);
+ if (!dbus_validate_bus_name(cli_dbus->service_name, &error)) {
+ err(tdc, "dbus: Could not validate bus name: %s - %s",
+ error.name, error.message);
+ goto free_service_name;
+ }
cli_dbus->conn = dbus_bus_get(DBUS_BUS_SYSTEM, &error);
if (!cli_dbus->conn) {
err(tdc, "dbus: Could not acquire the system bus: %s - %s",
error.name, error.message);
- err = -EINVAL;
goto free_service_name;
}
err = 0;
--
2.31.1
1 year, 6 months
Re: Release anytime soon?
by Jiri Pirko
Mon, Jun 27, 2022 at 07:16:36PM CEST, janos2010(a)farkas.ch wrote:
>There was no patch needed for the integration on the libteam side. I've
>been testing the built package for several months now, but it's unlikely to
>be accepted as a package from the git master branch.
>
>The concern was that Void prefers an actual upstream release, with active
>development, and a versioned tarball as a base point, and to avoid
>requiring an internal maintainer. An ideal Void packaging is just a
>template to integrity check an upstream tarball, a specification of
>build//runtime dependencies for the given package, and preferably no
>integration patches.
>
>1.31 may have worked for the basic functionality, but for all I can see,
>most of the commits merged into the master branch seem essential. At least
>to avoid the daemon using 100% CPU seems critical enough.
>
>Or perhaps I'm misunderstanding and you would accept a patch to bump the
>version and publish 1.32 as a release?
Yes.
>
>Janos
>
>On Mon, Jun 27, 2022 at 5:45 PM Jiri Pirko <jiri(a)resnulli.us> wrote:
>
>> Wed, Oct 06, 2021 at 08:11:55PM CEST, chexum+fedora(a)gmail.com wrote:
>> >Hi!
>> >
>> >I'm using the latest code from github, and I would also like to have it
>> packaged for a distribution which does not have libteam yet -
>> https://github.com/void-linux/void-packages/pull/33028
>>
>> Please send a patch. Pull requests are ignored. See file SubmittingPatches
>>
>> >
>> >They are making a point that it's preferable using a proper upstream
>> release. As far as I can see, the commits since 1.31 appear somewhat
>> important. On the other hand, even the latest github changes are somewhat
>> old, so I can't see anything unstable about it.
>> >
>> >For the benefit of a cleaner packaging, are there any plans to make a
>> numbered release anytime soon?
>> >
>> >Janos
>> >_______________________________________________
>> >libteam mailing list -- libteam(a)lists.fedorahosted.org
>> >To unsubscribe send an email to libteam-leave(a)lists.fedorahosted.org
>> >Fedora Code of Conduct:
>> https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>> >List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>> >List Archives:
>> https://lists.fedorahosted.org/archives/list/libteam@lists.fedorahosted.org
>> >Do not reply to spam on the list, report it:
>> https://pagure.io/fedora-infrastructure
>>
1 year, 6 months