Message-Id: <201603171752.u2HHq8q1010929@d06av03.portsmouth.uk.ibm.com>
In-Reply-To: <1458234085-217667-1-git-send-email-agraf@suse.de>
Subject: Re: [PATCH] qeth: Default to allow promiscuous mode
From: "Evgeny Cherkashin" <Eugene.Crosser@ru.ibm.com>
To: Alexander Graf <agraf@suse.de>
Cc: linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        heicars2@linux.vnet.ibm.com, mschwid2@linux.vnet.ibm.com,
        Ursula Braun <ubraun@linux.vnet.ibm.com>, ihno@suse.de, mt@suse.com,
        pwieczorkiewicz@suse.de
Date: Thu, 17 Mar 2016 17:52:00 +0000
MIME-Version: 1.0
Sensitivity: 
Importance: Normal
References: <1458234085-217667-1-git-send-email-agraf@suse.de>
Content-Transfer-Encoding: 8BIT
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2820
Lines: 70

Hello all,

-----Alexander Graf <agraf@suse.de> wrote: -----

>To: linux-s390@vger.kernel.org
>From: Alexander Graf <agraf@suse.de>
>Date: 2016-03-17 20:01
>Cc: Evgeny Cherkashin/Russia/IBM@IBMRU, linux-kernel@vger.kernel.org,
>Heiko Carstens <heiko.carstens@de.ibm.com>, Martin Schwidefsky
><schwidefsky@de.ibm.com>, Ursula Braun <ubraun@linux.vnet.ibm.com>,
>ihno@suse.de, mt@suse.com, pwieczorkiewicz@suse.de
>Subject: [PATCH] qeth: Default to allow promiscuous mode
>
>When a qeth device is in bridge role, one of the ports of an adapter
>can
>ask for promiscuous mode and get it enabled.
>
>The default until now was to not allow user space to enable
>promiscuous mode
>without switching sysfs attributes as well though, diverging from
>usual
>network device semantics.
>
>This patch sets the default to allow promiscuous enablement. That way
>all
>existing tools "just work", albeit only one port of an adapter can be
>in
>promiscuous mode at a given time.
>
>Signed-off-by: Alexander Graf <agraf@suse.de>
>---
> drivers/s390/net/qeth_l2_sys.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/s390/net/qeth_l2_sys.c
>b/drivers/s390/net/qeth_l2_sys.c
>index 692db49..98c7ac5 100644
>--- a/drivers/s390/net/qeth_l2_sys.c
>+++ b/drivers/s390/net/qeth_l2_sys.c
>@@ -258,6 +258,10 @@ void qeth_l2_setup_bridgeport_attrs(struct
>qeth_card *card)
> 		return;
> 	if (!card->options.sbp.supported_funcs)
> 		return;
>+
>+	/* Allow to set promiscuous by default */
>+	card->options.sbp.reflect_promisc = 1;
>+
> 	if (card->options.sbp.role != QETH_SBP_ROLE_NONE) {
> 		/* Conditional to avoid spurious error messages */
> 		qeth_bridgeport_setrole(card, card->options.sbp.role);
>-- 
>1.8.5.6
>
>

1. The patch changes the default behaviour (which is, on s390, to ignore promisc setting) and has potential to break existing setups. One potentially dangerous scenario is that a Linux instance may inadvertently snatch the BRIDGEPORT role from a zVM VSWITCH that bridges the HiperSockets LAN to the outer world, disrupting connectivity for many users.

2. The patch sets the reflect_promisc field without setting the reflect_promisc_primary field, making it unclear to the user what is the default role. (It will be "secondary" but it's better to set it explicitly.)

3. The patch should probably go via the -networking maillist.

It seems to me that it would be better to use the sysfs attribute from a startup script or a udev rule, *only* when the system configuration requires "real" promiscuous mode, e.g. when the interface is a member of a software bridge.

Thanks,

Evgeny Cherkashin / Евгений Черкашин / Eugene Crosser (preferred)
Software Engineer, IBM Science and Technology Center, Linux on z-Systems Development
tel. +7 495 775 8800 ext.1103, Moscow 123317, Presnenskaya emb. 10