[PATCH net-next] net: dsa: Allow port mirroring to the CPU port

[PATCH net-next] net: dsa: Allow port mirroring to the CPU port

[Vladimir Oltean]

On a regular netdev, putting it in promiscuous mode means receiving all
traffic passing through it, whether or not it was destined to its MAC
address. Then monitoring applications such as tcpdump can see all
traffic transiting it.

On Ethernet switches, clearly all ports are in promiscuous mode by
definition, since they accept frames destined to any MAC address.
However tcpdump does not capture all frames transiting switch ports,
only the ones destined to, or originating from the CPU port.

To be able to monitor frames with tcpdump on the CPU port, extend the tc
matchall classifier and mirred action to support the DSA master port as
a possible mirror target.

Tested with:
tc qdisc add dev swp2 clsact
tc filter add dev swp2 ingress matchall skip_sw \
	action mirred egress mirror dev eth2
tcpdump -i swp2

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/slave.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 75d58229a4bd..5db0a4f45e7b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -872,7 +872,7 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	__be16 protocol = cls->common.protocol;
 	struct dsa_switch *ds = dp->ds;
 	struct flow_action_entry *act;
-	struct dsa_port *to_dp;
+	const struct dsa_port *to_dp;
 	int err = -EOPNOTSUPP;
 
 	if (!ds->ops->port_mirror_add)
@@ -889,7 +889,11 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 		if (!act->dev)
 			return -EINVAL;
 
-		if (!dsa_slave_dev_check(act->dev))
+		if (dsa_slave_dev_check(act->dev))
+			to_dp = dsa_slave_to_port(act->dev);
+		else if (act->dev == dp->cpu_dp->master)
+			to_dp = dp->cpu_dp;
+		else
 			return -EOPNOTSUPP;
 
 		mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
@@ -900,8 +904,6 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 		mall_tc_entry->type = DSA_PORT_MALL_MIRROR;
 		mirror = &mall_tc_entry->mirror;
 
-		to_dp = dsa_slave_to_port(act->dev);
-
 		mirror->to_local_port = to_dp->index;
 		mirror->ingress = ingress;
 
-- 
2.17.1

Re: [PATCH net-next] net: dsa: Allow port mirroring to the CPU port

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Thu,  3 Oct 2019 02:37:50 +0300

> On a regular netdev, putting it in promiscuous mode means receiving all
> traffic passing through it, whether or not it was destined to its MAC
> address. Then monitoring applications such as tcpdump can see all
> traffic transiting it.
> 
> On Ethernet switches, clearly all ports are in promiscuous mode by
> definition, since they accept frames destined to any MAC address.
> However tcpdump does not capture all frames transiting switch ports,
> only the ones destined to, or originating from the CPU port.
> 
> To be able to monitor frames with tcpdump on the CPU port, extend the tc
> matchall classifier and mirred action to support the DSA master port as
> a possible mirror target.
> 
> Tested with:
> tc qdisc add dev swp2 clsact
> tc filter add dev swp2 ingress matchall skip_sw \
> 	action mirred egress mirror dev eth2
> tcpdump -i swp2
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

Andrew and co., please review.

Re: [PATCH net-next] net: dsa: Allow port mirroring to the CPU port

[Andrew Lunn]

On Thu, Oct 03, 2019 at 12:04:57PM -0700, David Miller wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Thu,  3 Oct 2019 02:37:50 +0300
> 
> > On a regular netdev, putting it in promiscuous mode means receiving all
> > traffic passing through it, whether or not it was destined to its MAC
> > address. Then monitoring applications such as tcpdump can see all
> > traffic transiting it.
> > 
> > On Ethernet switches, clearly all ports are in promiscuous mode by
> > definition, since they accept frames destined to any MAC address.
> > However tcpdump does not capture all frames transiting switch ports,
> > only the ones destined to, or originating from the CPU port.
> > 
> > To be able to monitor frames with tcpdump on the CPU port, extend the tc
> > matchall classifier and mirred action to support the DSA master port as
> > a possible mirror target.
> > 
> > Tested with:
> > tc qdisc add dev swp2 clsact
> > tc filter add dev swp2 ingress matchall skip_sw \
> > 	action mirred egress mirror dev eth2
> > tcpdump -i swp2
> > 
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> 
> Andrew and co., please review.

Yes, i thinking about this. Not reached a conclusion yet.

     Andrew

Re: [PATCH net-next] net: dsa: Allow port mirroring to the CPU port

[Andrew Lunn]

On Thu, Oct 03, 2019 at 02:37:50AM +0300, Vladimir Oltean wrote:
> On a regular netdev, putting it in promiscuous mode means receiving all
> traffic passing through it, whether or not it was destined to its MAC
> address. Then monitoring applications such as tcpdump can see all
> traffic transiting it.
> 
> On Ethernet switches, clearly all ports are in promiscuous mode by
> definition, since they accept frames destined to any MAC address.
> However tcpdump does not capture all frames transiting switch ports,
> only the ones destined to, or originating from the CPU port.
> 
> To be able to monitor frames with tcpdump on the CPU port, extend the tc
> matchall classifier and mirred action to support the DSA master port as
> a possible mirror target.
> 
> Tested with:
> tc qdisc add dev swp2 clsact
> tc filter add dev swp2 ingress matchall skip_sw \
> 	action mirred egress mirror dev eth2
> tcpdump -i swp2

Humm.

O.K, i don't like this for a few reasons.

egress mirror dev eth2

Frames are supported to egress eth2. But in fact they will ingress on
eth2. That is not intuitive.

I'm also no sure how safe this it is to ingress mirror packets on the
master interface. Will they have DSA tags? I think that will vary from
device to device. Are we going to see some packets twice? Once for the
mirror, and a second time because they are destined to the CPU? Do we
end up processing the packets twice?

For your use case of wanting to see packets in tcpdump, i think we are
back to the discussion of what promisc mode means. I would prefer that
when a DSA slave interface is put into promisc mode for tcpdump, the
switch then forwards a copy of frames to the CPU, without
duplication. That is a much more intuitive model.

	  Andrew

Re: [PATCH net-next] net: dsa: Allow port mirroring to the CPU port

[Vladimir Oltean]

On Thu, 3 Oct 2019 at 22:24, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Oct 03, 2019 at 02:37:50AM +0300, Vladimir Oltean wrote:
> > On a regular netdev, putting it in promiscuous mode means receiving all
> > traffic passing through it, whether or not it was destined to its MAC
> > address. Then monitoring applications such as tcpdump can see all
> > traffic transiting it.
> >
> > On Ethernet switches, clearly all ports are in promiscuous mode by
> > definition, since they accept frames destined to any MAC address.
> > However tcpdump does not capture all frames transiting switch ports,
> > only the ones destined to, or originating from the CPU port.
> >
> > To be able to monitor frames with tcpdump on the CPU port, extend the tc
> > matchall classifier and mirred action to support the DSA master port as
> > a possible mirror target.
> >
> > Tested with:
> > tc qdisc add dev swp2 clsact
> > tc filter add dev swp2 ingress matchall skip_sw \
> >       action mirred egress mirror dev eth2
> > tcpdump -i swp2
>
> Humm.
>
> O.K, i don't like this for a few reasons.
>
> egress mirror dev eth2
>
> Frames are supported to egress eth2. But in fact they will ingress on
> eth2. That is not intuitive.
>

But you are just arguing that the tc mirred syntax is confusing.
'ingress'/'egress' has nothing to do with 'eth2'. You just specify the
direction of the frames transiting swp2 that you want to capture. And
the destination port, as a net device. Because there is no net device
for the CPU port, 'eth2' acts as substitute. Florian's br0 could have
acted as substitute as well, but then there may not be a br0...
But that is only to fit the existing tc mirred command pattern. I'm
not using 'tcpdump -i eth2' to capture the mirrored traffic, but still
'tcpdump -i swp2'.

> I'm also no sure how safe this it is to ingress mirror packets on the
> master interface. Will they have DSA tags? I think that will vary from

Generally speaking, I would say that a device which does not push DSA
tags towards the CPU port is broken. Yes, I know, I don't need to be
reminded about cc1939e4b3aa ("net: dsa: Allow drivers to filter
packets they can decode source port from").
But there might be other exceptions too: maybe some switches support
cascaded setups, but don't stack DSA tags, and they need an awareness
of the switches beneath them, case in which it's reasonable for them
not to push a second tag. But then it isn't possible to enable port
mirroring on a DSA port anyway, due to lack of both net devices in
this case.

> device to device. Are we going to see some packets twice? Once for the
> mirror, and a second time because they are destined to the CPU? Do we
> end up processing the packets twice?
>

Does it matter?
FWIW, my device does not duplicate frames which already had the CPU in
the destination ports mask. But you will, nonetheless, see as
duplicated the frames transmitted from the CPU towards a port with
egress mirroring enabled towards the CPU. But then you could just keep
only ingress mirroring enabled, if that bothered you.

> For your use case of wanting to see packets in tcpdump, i think we are
> back to the discussion of what promisc mode means. I would prefer that
> when a DSA slave interface is put into promisc mode for tcpdump, the
> switch then forwards a copy of frames to the CPU, without
> duplication. That is a much more intuitive model.
>

So I'm not disagreeing that the patch I'm proposing isn't very
intuitive. But I think the reasons you pointed out are not the real
ones why.
I would like to see DSA switch net devices (and not only) as
'offloaded net devices', some of the traffic not reaching the CPU for
whatever reason. And have a switch to copy the offloaded traffic
towards the CPU as well. Then it would not matter that it's DSA or
switchdev or capable of offloaded IP routing or promiscuous or
whatever.
Would I want that switch to get flipped by default by the driver when
I run tcpdump? Not so sure. I mean there are already switches like
'--monitor-mode' in tcpdump specifically for Wi-Fi, so it's not as
though users who want to 'see everything' aren't able to understand a
new concept (in this case an 'offloaded net device').
And piggybacking on top of the promiscuity concept maybe isn't the
most intuitive way to get this solved either: you can already be
promiscuous and still not 'see everything'. And there's a second
reason too: mirroring (or copy-to-cpu) in many devices is a lot more
configurable than promiscuity is. Even 'dumb' devices like sja1105
support port-based mirroring and flow-based mirroring (classification
done at least by {DMAC, VLAN} keys), configured separately for the RX
and TX direction of each port. I suppose you want the driver to just
enable something really simple, like egress and ingress port-based
mirroring? Maybe that is less useful in a real debugging scenario than
just copying what you're interested in.
And this slowly glides towards the idea that if there's already this
much degree of configurability in what you want to mirror, then maybe
it doesn't make much sense at all in even having that switch put in
tcpdump (where it would only be something trivial, if not implicit),
and not in a more dedicated place for this kind of stuff, like tc.
Maybe the discussion should be about how to represent traffic destined
towards the CPU in a more abstract way in the tc mirred command?

>           Andrew

Thanks,
-Vladimir