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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Aug 29, 2019 at 12:56:54PM CEST, horatiu.vultur@microchip.com wrote:
>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

If the promisc is on, it is on. Why do you need to know who enabled it?

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


>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