Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 08/29/2019 11:51, Jiri Pirko wrote:
> External E-Mail
> 
> 
> Thu, Aug 29, 2019 at 11:22:28AM CEST, horatiu.vultur@microchip.com wrote:
> >Add the SWITCHDEV_ATTR_ID_PORT_PROMISCUITY switchdev notification type,
> >used to indicate whenever the dev promiscuity counter is changed.
> >
> >The notification doesn't use any switchdev_attr attribute because in the
> >notifier callbacks is it possible to get the dev and read directly
> >the promiscuity value.
> >
> >Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> >---
> > include/net/switchdev.h | 1 +
> > net/core/dev.c          | 9 +++++++++
> > 2 files changed, 10 insertions(+)
> >
> >diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> >index aee86a1..14b1617 100644
> >--- a/include/net/switchdev.h
> >+++ b/include/net/switchdev.h
> >@@ -40,6 +40,7 @@ enum switchdev_attr_id {
> > 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
> > 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> > 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
> >+	SWITCHDEV_ATTR_ID_PORT_PROMISCUITY,
> > };
> > 
> > struct switchdev_attr {
> >diff --git a/net/core/dev.c b/net/core/dev.c
> >index 49589ed..40c74f2 100644
> >--- a/net/core/dev.c
> >+++ b/net/core/dev.c
> >@@ -142,6 +142,7 @@
> > #include <linux/net_namespace.h>
> > #include <linux/indirect_call_wrapper.h>
> > #include <net/devlink.h>
> >+#include <net/switchdev.h>
> > 
> > #include "net-sysfs.h"
> > 
> >@@ -7377,6 +7378,11 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
> > static int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
> > {
> > 	unsigned int old_flags = dev->flags;
> >+	struct switchdev_attr attr = {
> >+		.orig_dev = dev,
> >+		.id = SWITCHDEV_ATTR_ID_PORT_PROMISCUITY,
> >+		.flags = SWITCHDEV_F_DEFER,
> 

Hi Jiri,

> NACK
> 
> This is invalid usecase for switchdev infra. Switchdev is there for
> bridge offload purposes only.
> 
> For promiscuity changes, the infrastructure is already present in the
> code. See __dev_notify_flags(). it calls:
> call_netdevice_notifiers_info(NETDEV_CHANGE, &change_info.info)
> and you can actually see the changed flag in ".flags_changed".
Yes, you are right. But in case the port is part of a bridge and then
enable promisc mode by a user space application(tpcdump) then the drivers
will not be notified. The reason is that the dev->flags will still be the
same(because IFF_PROMISC was already set) and only promiscuity will be
changed.

One fix could be to call __dev_notify_flags() no matter when the
promisc is enable or disabled.

> 
> You just have to register netdev notifier block in your driver. Grep
> for: register_netdevice_notifier
> 
> 
> >+	};
> > 	kuid_t uid;
> > 	kgid_t gid;
> > 
> >@@ -7419,6 +7425,9 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
> > 	}
> > 	if (notify)
> > 		__dev_notify_flags(dev, old_flags, IFF_PROMISC);
> >+
> >+	switchdev_port_attr_set(dev, &attr);
> >+
> > 	return 0;
> > }
> > 
> >-- 
> >2.7.4
> >
> 

-- 
/Horatiu