netdev - Re: [PATCH] bonding: Don't allow mode change via sysfs with slaves present

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: <20111115210431.GA9768@darkmag.usersys.redhat.com>
Date:	Tue, 15 Nov 2011 22:04:31 +0100
From:	Veaceslav Falico <vfalico@...hat.com>
To:	Nicolas de Pesloüan 
	<nicolas.2p.debian@...il.com>
Cc:	Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org,
	Jay Vosburgh <fubar@...ibm.com>
Subject: Re: [PATCH] bonding: Don't allow mode change via sysfs with slaves
 present

On Tue, Nov 15, 2011 at 08:24:29PM +0100, Nicolas de Pesloüan wrote:
> Le 15/11/2011 18:00, Andy Gospodarek a écrit :
> >On Tue, Nov 15, 2011 at 05:44:42PM +0100, Veaceslav Falico wrote:
> >>When changing mode via bonding's sysfs, the slaves are not initialized
> >>correctly. Forbid to change modes with slaves present to ensure that every
> >>slave is initialized correctly via bond_enslave().
> >>
> >>Signed-off-by: Veaceslav Falico<vfalico@...hat.com>
> >
> >Looks good.  This behavior forces someone who wants to change to mode to
> >go through steps that are almost as destructive as when module options
> >are used to configure the mode.  I do not see a problem with this.
> 
> Except the fact that is enforce one more constraint on the exact
> order one should write into sysfs to setup a bonding interface. We
> already have many such constraints and probably don't need more.
> 
> Currently, it is possible to enslave slaves before selecting the
> mode. The ifenslave-2.6 package from Debian currently enslave slaves
> before setting the mode and would break with this change.

Yes, it's possible, however the enslaved interfaces are initialized with
the current mode parameters, and when the mode is changed - they aren't
reinitialized at all. There are a lot of mode-specific initialization stuff
that's present only in bond_enslave(), and here are only some of the
(most obvious) snippets:

ALB/TLB balancing:
<snip>
        if (bond_is_lb(bond)) {
                /* bond_alb_init_slave() must be called before all other
 * stages since
                 * it might fail and we do not want to have to undo
                 * everything
                 */
                res = bond_alb_init_slave(bond, new_slave);
                if (res)
                        goto err_close;
        }    
</snip>

bond_alb_init_slave() is called only in this case. This means that the mac
address won't be changed at all, and some other stuff won't be properly
changed as well.

802.3ad:
<snip>
        if (bond->params.mode == BOND_MODE_8023AD) {
                /* add lacpdu mc addr to mc list */
                u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;

                dev_mc_add(slave_dev, lacpdu_multicast);
        }
</snip>

This means that the slave device will just drop all the LACPDUs.

So this means that at least two modes won't work if your first load the
bonding module with the default mode and then change it with slaves
attached. And I'm *really* sceptic on the other modes.

So even if the kernel doesn't show any error, it still doesn't work as
expected. To *really* fix this bug without adding any constraints would
require quite a few lines of code, and before it is fixed this patch is the
best way to avoid it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html