netdev - Re: [PATCH] bonding: Allow tun-interfaces as slaves

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: <20160811182418.GC25877@cork>
Date:	Thu, 11 Aug 2016 11:24:18 -0700
From:	Jörn Engel <joern@...estorage.com>
To:	Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:	David Miller <davem@...emloft.net>, dingtianhong@...wei.com,
	zyjzyj2000@...il.com, andy@...yhouse.net, netdev@...r.kernel.org
Subject: Re: [PATCH] bonding: Allow tun-interfaces as slaves

On Wed, Aug 10, 2016 at 05:58:38PM -0700, Jay Vosburgh wrote:
> Jörn Engel <joern@...estorage.com> wrote:
> >On Wed, Aug 10, 2016 at 02:26:49PM -0700, Jörn Engel wrote:
> >> 
> >> Having to set one more parameter is a bit annoying.  It would have to be
> >> documented in a prominent place and people would still often miss it.
> >> So I wonder if we can make the interface a little nicer.
> >> 
> >> Options:
> >> - If there are no slaves yet and the first slave added is tun, we trust
> >>   the users to know what they are doing.  Automatically set
> >>   bond->params.fail_over_mac = BOND_FOM_KEEPMAC
> >>   Maybe do a printk to inform the user in case of a mistake.
> 
> 	I don't think this is feasible, as I don't see a reliable way to
> test for a slave being a tun device (ARPHRD_NONE is not just tun, and we
> cannot check the ops as they are not statically built into the kernel).
> I'm also not sure that heuristics are the proper way to enable this
> functionality in general.

I was looking for a slightly more generic thing than "is this device
tun?".  Something along the lines of "is this device L3 only?".  We can
always introduce a new flag and have tun set the flag.  Naïve me thought
ARPHRD_NONE might already match what I was looking for.

But if such an approach causes problems for others, it is a non-starter.

> >> - If we get an error and the slave device is tun, do a printk giving the
> >>   user enough information to find this parameter.
> 
> 	This could probably be done as a change the existing logic, e.g.,
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1f276fa30ba6..019c1a689aae 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1443,6 +1443,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  				res = -EOPNOTSUPP;
>  				goto err_undo_flags;
>  			}
> +		} else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP &&
> +			   bond->params.fail_over_mac != BOND_FOM_KEEPMAC) {
> +				netdev_err(bond_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac\n");
>  		}
>  	}
>  
> 	I haven't tested this, and I'm not sure it will get all corner
> cases correct, but this should basically cover it.

Nit: Indentation is wrong (two tabs instead of one).

It should provide enough information for anyone that reads kernel messages.
Works for me.

[588380.721349] bond1: Adding slave tun0
[588380.721402] bond1: The slave device specified does not support setting the MAC address
[588380.721404] bond1: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to keepmac

Jörn

--
It is easier to lead men to combat, stirring up their passion, than to
restrain them and direct them toward the patient labours of peace.
-- Andre Gide