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

[Horatiu Vultur]

The 08/29/2019 14:18, Jiri Pirko wrote:
> External E-Mail
> 
> 
> Thu, Aug 29, 2019 at 12:56:54PM CEST, horatiu.vultur@xxxxxxxxxxxxx wrote:
> >The 08/29/2019 11:51, Jiri Pirko wrote:
> >> External E-Mail
> >> 
> >> 
> >> Thu, Aug 29, 2019 at 11:22:28AM CEST, horatiu.vultur@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> >> >---
> >> > 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
> 
> If the promisc is on, it is on. Why do you need to know who enabled it?
When a port is added to a bridge, then the port gets in promisc mode.
But in our case the HW has bridge capabilities so it is not required to
set the port in promisc mode.
But if someone else requires the port to be in promisc mode (tcpdump or
any other application) then I would like to set the port in promisc
mode.
In previous emails Andrew came with the suggestion to look at
dev->promiscuity and check if the port is a bridge port. Using this
information I could see when to add the port in promisc mode. He also
suggested to add a new switchdev call(maybe I missunderstood him, or I
have done it at the wrong place) in case there are no callbacks in the
driver to get this information.
> 
> Or do you want to use this to ask driver to ask hw to trap packets to
> kernel? If yes, I don't think it is correct. If you want to "steal" some
> packets from the hw forwarding datapath, use TC action "trap".
No, I just wanted to know when to update the HW to set the port in
promisc mode.

> 
> 
> >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
> 

-- 
/Horatiu