On Thu, Jan 02, 2025 at 11:50:25AM -0800, Octavian Purdila wrote:

quoted

I didn't test, what if enslave veth0 first and then add enslave veth0.1 later.

Hi Hangbin,

Thanks for the review!

I was not able to reproduce the crash with this scenario. I think this
is because adding veth0.1 does not affect the link state for veth0,
while in the original scenario bringing up veth0 would also bring up
veth0.1.

Regardless, allowing this setup seems risky and syzkaller may find
other ways to trigger it, so maybe a more generic check like below
would be better?

        list_for_each_entry(tmp, &team->port_list, list) {
                if (netdev_has_upper_dev(tmp->dev, port_dev) ||
                        netdev_has_upper_dev(port_dev, tmp->dev)) {
                        NL_SET_ERR_MSG(extack, "Device is a lower or
upper of an enslaved device");
                        netdev_err(dev, "Device %s is a lower or upper
device of enslaved device %s\n",
                                   portname, tmp->dev->name);
                        return -EBUSY;
                }
        }

Although I am not sure if there are legitimate use-cases that this may restrict?

The logic makes sense to me. Let's see if Jiri has any comments.

Thanks
Hangbin