Re: [PATCH] bonding: 802.3ad: make aggregator_identifier bond-private

[Flavio Leitner <fbl@redhat.com>]

On Fri, Feb 14, 2014 at 09:51:47PM +0100, Jiri Bohac wrote:
> On Fri, Feb 14, 2014 at 05:12:43PM -0200, Flavio Leitner wrote:
> > On Fri, Feb 14, 2014 at 06:13:50PM +0100, Jiri Bohac wrote:
> > > Fix this by making aggregator_identifier private to the bond.
> > 
> > I don't see how you fix the duplicate agg id with this patch because
> > you initialize for each bond to 0, then use the same algo further on.
> > So, what is changing?
> 
> My understanding is that the aggregator identifier is used
> internally by the bond and never appears anywhere in the LACP
> traffic.
> 
> So having duplicate aggregator ids between two bonds on the same
> machine does not matter. But it is a problem if two aggregators
> in the same bond share the same id.
> 
> Is my understanding wrong?

No, I agree that having two agg id in the same bond is a problem
while we can have the same agg id in different bonds.

> > Actually, aggregator_identifier is a global variable to make sure the
> > counter is always increasing for new bonds.  So, the fix would be to
> > not reset it to zero, isn't it?
> 
> I was considering this fix, but my concern was that the variable
> (u16) would overflow sooner than it does now. It would take 2^16
> enslavings on the machine, while with my patch you need 2^16
> enslavings on a single bond.
> 
> Hypothetically, a rogue NET_ADMIN in one net namespace may cause
> this overflow to break a bond in another nemespace.

Well, I think it is easier to troubleshoot if you have one agg id
regardless of bonding. Also, the id is visible in /proc and I
recall to have seen a script using it somehow, but yeah you made a
good point.
 
> Maybe I'm being paranoid? ;)

I will defer to bonding maintainer to decide. :P

Thanks,
fbl