[PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Hiroshi Shimamoto]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5145 bytes --]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Add netlink directives and ndo entry to allow VF multicast promiscuous mode.

The administrator wants to allow dedicatedly multicast promiscuous per VF.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
CC: Choi, Sy Jong <sy.jong.choi@intel.com>
---
 include/linux/if_link.h      |  1 +
 include/linux/netdevice.h    |  3 +++
 include/uapi/linux/if_link.h |  6 ++++++
 net/core/rtnetlink.c         | 18 ++++++++++++++++--
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 119130e..bc29ddf 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -14,5 +14,6 @@ struct ifla_vf_info {
 	__u32 linkstate;
 	__u32 min_tx_rate;
 	__u32 max_tx_rate;
+	__u32 mc_promisc;
 };
 #endif /* _LINUX_IF_LINK_H */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d115256..fd15d87 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -870,6 +870,7 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
  * int (*ndo_set_vf_rate)(struct net_device *dev, int vf, int min_tx_rate,
  *			  int max_tx_rate);
  * int (*ndo_set_vf_spoofchk)(struct net_device *dev, int vf, bool setting);
+ * int (*ndo_set_vf_mc_promisc)(struct net_device *dev, int vf, bool setting);
  * int (*ndo_get_vf_config)(struct net_device *dev,
  *			    int vf, struct ifla_vf_info *ivf);
  * int (*ndo_set_vf_link_state)(struct net_device *dev, int vf, int link_state);
@@ -1086,6 +1087,8 @@ struct net_device_ops {
 						   int max_tx_rate);
 	int			(*ndo_set_vf_spoofchk)(struct net_device *dev,
 						       int vf, bool setting);
+	int			(*ndo_set_vf_mc_promisc)(struct net_device *dev,
+							 int vf, bool setting);
 	int			(*ndo_get_vf_config)(struct net_device *dev,
 						     int vf,
 						     struct ifla_vf_info *ivf);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 0deee3e..d7dc39c 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -458,6 +458,7 @@ enum {
 	IFLA_VF_SPOOFCHK,	/* Spoof Checking on/off switch */
 	IFLA_VF_LINK_STATE,	/* link state enable/disable/auto switch */
 	IFLA_VF_RATE,		/* Min and Max TX Bandwidth Allocation */
+	IFLA_VF_MC_PROMISC,	/* Multicast Promiscuous allow/disallow */
 	__IFLA_VF_MAX,
 };
 
@@ -502,6 +503,11 @@ struct ifla_vf_link_state {
 	__u32 link_state;
 };
 
+struct ifla_vf_mc_promisc {
+	__u32 vf;
+	__u32 setting;
+};
+
 /* VF ports management section
  *
  *	Nested layout of set/get msg is:
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5be499b..b668e96 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -818,7 +818,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev,
 			 nla_total_size(sizeof(struct ifla_vf_vlan)) +
 			 nla_total_size(sizeof(struct ifla_vf_spoofchk)) +
 			 nla_total_size(sizeof(struct ifla_vf_rate)) +
-			 nla_total_size(sizeof(struct ifla_vf_link_state)));
+			 nla_total_size(sizeof(struct ifla_vf_link_state)) +
+			 nla_total_size(sizeof(struct ifla_vf_mc_promisc)));
 		return size;
 	} else
 		return 0;
@@ -1111,6 +1112,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			struct ifla_vf_tx_rate vf_tx_rate;
 			struct ifla_vf_spoofchk vf_spoofchk;
 			struct ifla_vf_link_state vf_linkstate;
+			struct ifla_vf_mc_promisc vf_mc_promisc;
 
 			/*
 			 * Not all SR-IOV capable drivers support the
@@ -1119,6 +1121,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			 * report anything.
 			 */
 			ivi.spoofchk = -1;
+			ivi.mc_promisc = -1;
 			memset(ivi.mac, 0, sizeof(ivi.mac));
 			/* The default value for VF link state is "auto"
 			 * IFLA_VF_LINK_STATE_AUTO which equals zero
@@ -1131,7 +1134,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 				vf_rate.vf =
 				vf_tx_rate.vf =
 				vf_spoofchk.vf =
-				vf_linkstate.vf = ivi.vf;
+				vf_linkstate.vf =
+				vf_mc_promisc.vf = ivi.vf;
 
 			memcpy(vf_mac.mac, ivi.mac, sizeof(ivi.mac));
 			vf_vlan.vlan = ivi.vlan;
@@ -1140,6 +1144,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			vf_rate.min_tx_rate = ivi.min_tx_rate;
 			vf_rate.max_tx_rate = ivi.max_tx_rate;
 			vf_spoofchk.setting = ivi.spoofchk;
+			vf_mc_promisc.setting = ivi.mc_promisc;
 			vf_linkstate.link_state = ivi.linkstate;
 			vf = nla_nest_start(skb, IFLA_VF_INFO);
 			if (!vf) {
@@ -1481,6 +1486,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
 								 ivl->link_state);
 			break;
 		}
+		case IFLA_VF_MC_PROMISC: {
+			struct ifla_vf_mc_promisc *ivm;
+			ivm = nla_data(vf);
+			err = -EOPNOTSUPP;
+			if (ops->ndo_set_vf_mc_promisc)
+				err = ops->ndo_set_vf_mc_promisc(dev, ivm->vf,
+								 ivm->setting);
+			break;
+		}
 		default:
 			err = -EINVAL;
 			break;
-- 
2.1.0

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

On Fri, 2015-02-20 at 01:00 +0000, Hiroshi Shimamoto wrote:
> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 
> Add netlink directives and ndo entry to allow VF multicast promiscuous
> mode.
> 
> The administrator wants to allow dedicatedly multicast promiscuous per
> VF.
> 
> Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com>
> CC: Choi, Sy Jong <sy.jong.choi@intel.com>
> ---
>  include/linux/if_link.h      |  1 +
>  include/linux/netdevice.h    |  3 +++
>  include/uapi/linux/if_link.h |  6 ++++++
>  net/core/rtnetlink.c         | 18 ++++++++++++++++--
>  4 files changed, 26 insertions(+), 2 deletions(-)

I have added this to my queue, thanks!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Edward Cree]

On 20/02/15 01:00, Hiroshi Shimamoto wrote:
> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>
> Add netlink directives and ndo entry to allow VF multicast promiscuous mode.
>
> The administrator wants to allow dedicatedly multicast promiscuous per VF.
If I'm properly understanding, this seems to be an ixgbe-specific option
to work around an ixgbe limitation; is it really appropriate to
implement as a generic net_device_op?
What would this ndo mean to a driver which can support thousands of
multicast groups without MC promisc?  Is it expected to limit the number
of MC groups when this is set to disallow?  Or just fulfil the letter of
the option but not its spirit?  The option doesn't seem to have
well-defined semantics outside of ixgbe.
I would suggest that the right place for this sort of driver-specific
device control is in sysfs.

I'm also a little perplexed as to why anyone would need to disallow
this; what security, or even administrative convenience, is gained by
allowing a VF to join 30 multicast groups but not multicast promiscuous
mode?  Especially as, afaik, there are no restrictions on which
multicast groups are joined, so the VF can receive any particular
multicast traffic it cares about.

RE: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Skidmore, Donald C]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3720 bytes --]



-----Original Message-----
From: Edward Cree [mailto:ecree@solarflare.com] 
Sent: Friday, February 20, 2015 5:52 AM
To: Hiroshi Shimamoto
Cc: Skidmore, Donald C; vyasevic@redhat.com; Kirsher, Jeffrey T; Alexander Duyck; Bjørn Mork; e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org; Choi, Sy Jong; linux-kernel@vger.kernel.org; David Laight; Hayato Momma
Subject: Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

On 20/02/15 01:00, Hiroshi Shimamoto wrote:
> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
>
> Add netlink directives and ndo entry to allow VF multicast promiscuous mode.
>
> The administrator wants to allow dedicatedly multicast promiscuous per VF.
If I'm properly understanding, this seems to be an ixgbe-specific option to work around an ixgbe limitation; is it really appropriate to implement as a generic net_device_op?
What would this ndo mean to a driver which can support thousands of multicast groups without MC promisc?  Is it expected to limit the number of MC groups when this is set to disallow?  Or just fulfil the letter of the option but not its spirit?  The option doesn't seem to have well-defined semantics outside of ixgbe.
I would suggest that the right place for this sort of driver-specific device control is in sysfs.

I'm also a little perplexed as to why anyone would need to disallow this; what security, or even administrative convenience, is gained by allowing a VF to join 30 multicast groups but not multicast promiscuous mode?  Especially as, afaik, there are no restrictions on which multicast groups are joined, so the VF can receive any particular multicast traffic it cares about.
The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly 
prohibited.


If a vender specific interface is objectionable maybe a simpler and more generic interface would be for the PF to be able to set a given VF into "trusted" mode.  Then when the VF requested to enter multicast promiscuous mode via the mailbox message the PF would just allow it?  This could then be used to address other issues where we don't want to allow a VF to do something due to isolation or performance concerns.  I admit exactly what 'trusted' meant would vary from vender to vender, but it would be a way for the driver to know it could allow configurations such as this.  Just an idea, since we seem to be getting more requests for things such as this.

As to why someone may want to block a VF from entering multicast promiscuous it has more to do with performance that security.  The issue is this could have a very noticeably effect  on the overall system.  If any other VFs (or the PF) are receiving MC packets these will have to be replicated which will be a performance hit.  When we use the MC hash this is limited vs. when anyone is in MC promiscuous every MC packet used by another pool would be replicated. .  If too many VF's were in this mode you run the risk for flooding the PCIe interface.  I could imagine in some environments (i.e. public clouds) where you don't trust what is running in your VM you might what to block this from happening.

- Don Skidmore <donald.c.skidmore@intel.com>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Edward Cree]

On 20/02/15 21:05, Skidmore, Donald C wrote:
> If a vender specific interface is objectionable maybe a simpler and more generic interface would be for the PF to be able to set a given VF into "trusted" mode... I admit exactly what 'trusted' meant would vary from vender to vender, but it would be a way for the driver to know it could allow configurations such as this.  Just an idea, since we seem to be getting more requests for things such as this.
That's an even worse idea; now you have a generic interface with
completely undefined semantics.
The right way to do this, imho, is to use one of the standard interfaces
for driver-specific gubbins - e.g. sysfs, genetlink or even (whisper it)
ioctls - and put your 'VF promisc mode' setting there.  That way you
have a vendor-specific interface with vendor-specified semantics.
Of those options, I'd recommend sysfs as the best fit.

RE: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Skidmore, Donald C]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2196 bytes --]

> -----Original Message-----
> From: Edward Cree [mailto:ecree@solarflare.com]
> Sent: Monday, February 23, 2015 5:53 AM
> To: Skidmore, Donald C
> Cc: Hiroshi Shimamoto; vyasevic@redhat.com; Kirsher, Jeffrey T; Alexander
> Duyck; Bjørn Mork; e1000-devel@lists.sourceforge.net;
> netdev@vger.kernel.org; Choi, Sy Jong; linux-kernel@vger.kernel.org; David
> Laight; Hayato Momma
> Subject: Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control
> 
> On 20/02/15 21:05, Skidmore, Donald C wrote:
> > If a vender specific interface is objectionable maybe a simpler and more
> generic interface would be for the PF to be able to set a given VF into
> "trusted" mode... I admit exactly what 'trusted' meant would vary from
> vender to vender, but it would be a way for the driver to know it could allow
> configurations such as this.  Just an idea, since we seem to be getting more
> requests for things such as this.
> That's an even worse idea; now you have a generic interface with completely
> undefined semantics.
> The right way to do this, imho, is to use one of the standard interfaces for
> driver-specific gubbins - e.g. sysfs, genetlink or even (whisper it) ioctls - and
> put your 'VF promisc mode' setting there.  That way you have a vendor-
> specific interface with vendor-specified semantics.
> Of those options, I'd recommend sysfs as the best fit.
> The information contained in this message is confidential and is intended for
> the addressee(s) only. If you have received this message in error, please
> notify the sender immediately and delete the message. Unless you are an
> addressee (or authorized to receive for an addressee), you may not use,
> copy or disclose to anyone this message or any information contained in this
> message. The unauthorized use, disclosure, copying or alteration of this
> message is strictly prohibited.

I do see your point, but I thought custom sysfs interfaces (not to even mention new ioctl's :) were frowned upon?  Which is why I didn't even consider sysfs as an option.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

On Mon, 2015-02-23 at 06:29 -0800, Skidmore, Donald C wrote:
> > -----Original Message-----
> > From: Edward Cree [mailto:ecree@solarflare.com]
> > Sent: Monday, February 23, 2015 5:53 AM
> > To: Skidmore, Donald C
> > Cc: Hiroshi Shimamoto; vyasevic@redhat.com; Kirsher, Jeffrey T; Alexander
> > Duyck; Bjørn Mork; e1000-devel@lists.sourceforge.net;
> > netdev@vger.kernel.org; Choi, Sy Jong; linux-kernel@vger.kernel.org; David
> > Laight; Hayato Momma
> > Subject: Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control
> > 
> > On 20/02/15 21:05, Skidmore, Donald C wrote:
> > > If a vender specific interface is objectionable maybe a simpler and more
> > generic interface would be for the PF to be able to set a given VF into
> > "trusted" mode... I admit exactly what 'trusted' meant would vary from
> > vender to vender, but it would be a way for the driver to know it could allow
> > configurations such as this.  Just an idea, since we seem to be getting more
> > requests for things such as this.
> > That's an even worse idea; now you have a generic interface with completely
> > undefined semantics.
> > The right way to do this, imho, is to use one of the standard interfaces for
> > driver-specific gubbins - e.g. sysfs, genetlink or even (whisper it) ioctls - and
> > put your 'VF promisc mode' setting there.  That way you have a vendor-
> > specific interface with vendor-specified semantics.
> > Of those options, I'd recommend sysfs as the best fit.
> > The information contained in this message is confidential and is intended for
> > the addressee(s) only. If you have received this message in error, please
> > notify the sender immediately and delete the message. Unless you are an
> > addressee (or authorized to receive for an addressee), you may not use,
> > copy or disclose to anyone this message or any information contained in this
> > message. The unauthorized use, disclosure, copying or alteration of this
> > message is strictly prohibited.
> 
> I do see your point, but I thought custom sysfs interfaces (not to even mention new ioctl's :) were frowned upon?  Which is why I didn't even consider sysfs as an option.

We discussed this during NetConf last week, and Don is correct that a
custom sysfs interface is not the way we want to handle this.  We agreed
upon a generic interface so that any NIC is able to turn on or off VF
multicast promiscuous mode.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/3] if_link: Add VF multicast promiscuous control

[Or Gerlitz]

On Mon, Feb 23, 2015 at 11:14 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
[...]
> We discussed this during NetConf last week, and Don is correct that a
> custom sysfs interface is not the way we want to handle this.  We agreed
> upon a generic interface so that any NIC is able to turn on or off VF
> multicast promiscuous mode.

Jeff, please make sure to either respond to my comments on the V2
thread (or better) address them for the V3 post.


http://marc.info/?l=linux-netdev&m=142441852518152&w=2
http://marc.info/?l=linux-netdev&m=142441867218183&w=2

Or.