Am 04.04.21 um 01:21 schrieb Vladimir Oltean:
> On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote:
>> On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:
>>> Some switches (for example ar9331) do not provide enough information
>>> about forwarded packets. If the switch decision was made based on IPv4
>>> or IPv6 header, we need to analyze it and set proper flag.
>>>
>>> Potentially we can do it in existing rcv path, on other hand we can
>>> avoid part of duplicated work and let the dsa framework set skb header
>>> pointers and then use preprocessed skb one step later withing the rcv_post
>>> call back.
>>>
>>> This patch is needed for ar9331 switch.
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
>>> ---
>>
>> I don't necessarily disagree with this, perhaps we can even move
>> Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method
>> implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND
>> and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's
>> rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we
>> can already do:
>>
>> .rcv_post = dsa_untag_bridge_pvid,
>>
>> This should be generally useful for stuff that DSA taggers need to do
>> which is easiest done after eth_type_trans() was called.
>
> I had some fun with an alternative method of parsing the frame for IGMP
> so that you can clear skb->offload_fwd_mark, which doesn't rely on the
> introduction of a new method in DSA. It should also have several other
> advantages compared to your solution such as the fact that it should
> work with VLAN-tagged packets.
>
> Background: we made Receive Packet Steering work on DSA master interfaces
> (echo 3 > /sys/class/net/eth0/queues/rx-1/rps_cpus) even when the DSA
> tag shifts to the right the IP headers and everything that comes
> afterwards. The flow dissector had to be patched for that, just grep for
> DSA in net/core/flow_dissector.c.
>
> The problem you're facing is that you can't parse the IP and IGMP
> headers in the tagger's rcv() method, since the network header,
> transport header offsets and skb->protocol are all messed up, since
> eth_type_trans hasn't been called yet.
>
> And that's the trick right there, you're between a rock and a hard
> place: too early because eth_type_trans wasn't called yet, and too late
> because skb->dev was changed and no longer points to the DSA master, so
> the flow dissector adjustment we made doesn't apply.
>
> But if you call the flow dissector _before_ you call "skb->dev =
> dsa_master_find_slave" (and yes, while the DSA tag is still there), then
> it's virtually as if you had called that while the skb belonged to the
> DSA master, so it should work with __skb_flow_dissect.
>
> In fact I prototyped this idea below. I wanted to check whether I can
> match something as fine-grained as an IGMPv2 Membership Report message,
> and I could.
>
> I prototyped it inside the ocelot tagging protocol driver because that's
> what I had handy. I used __skb_flow_dissect with my own flow dissector
> which had to be initialized at the tagger module_init time, even though
> I think I could have probably just called skb_flow_dissect_flow_keys
> with a standard dissector, and that would have removed the need for the
> custom module_init in tag_ocelot.c. One thing that is interesting is
> that I had to add the bits for IGMP parsing to the flow dissector
> myself (based on the existing ICMP code). I was too lazy to do that for
> MLD as well, but it is really not hard. Or even better, if you don't
> need to look at all inside the IGMP/MLD header, I think you can even
> omit adding this parsing code to the flow dissector and just look at
> basic.n_proto and basic.ip_proto.
>
> See the snippet below. Hope it helps.
>
> -----------------------------[ cut here ]-----------------------------
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index ffd386ea0dbb..4c25fa47637a 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -190,6 +190,20 @@ struct flow_dissector_key_icmp {
> u16 id;
> };
>
> +/**
> + * flow_dissector_key_igmp:
> + * type: indicates the message type, see include/uapi/linux/igmp.h
> + * code: Max Resp Code, the maximum time in 1/10 second
> + * increments before sending a responding report
> + * group: the multicast address being queried when sending a
> + * Group-Specific or Group-and-Source-Specific Query.
> + */
> +struct flow_dissector_key_igmp {
> + u8 type;
> + u8 code; /* Max Resp Time in IGMPv2 */
> + __be32 group;
> +};
> +
> /**
> * struct flow_dissector_key_eth_addrs:
> * @src: source Ethernet address
> @@ -259,6 +273,7 @@ enum flow_dissector_key_id {
> FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
> FLOW_DISSECTOR_KEY_PORTS_RANGE, /* struct flow_dissector_key_ports */
> FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
> + FLOW_DISSECTOR_KEY_IGMP, /* struct flow_dissector_key_igmp */
> FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
> FLOW_DISSECTOR_KEY_TIPC, /* struct flow_dissector_key_tipc */
> FLOW_DISSECTOR_KEY_ARP, /* struct flow_dissector_key_arp */
> @@ -314,6 +329,7 @@ struct flow_keys {
> struct flow_dissector_key_keyid keyid;
> struct flow_dissector_key_ports ports;
> struct flow_dissector_key_icmp icmp;
> + struct flow_dissector_key_igmp igmp;
> /* 'addrs' must be the last member */
> struct flow_dissector_key_addrs addrs;
> };
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 5985029e43d4..8cc8c34ea5cd 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -202,6 +202,30 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
> skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen);
> }
>
> +static void __skb_flow_dissect_igmp(const struct sk_buff *skb,
> + struct flow_dissector *flow_dissector,
> + void *target_container, const void *data,
> + int thoff, int hlen)
> +{
> + struct flow_dissector_key_igmp *key_igmp;
> + struct igmphdr *ih, _ih;
> +
> + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IGMP))
> + return;
> +
> + ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih);
> + if (!ih)
> + return;
> +
> + key_igmp = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_IGMP,
> + target_container);
> +
> + key_igmp->type = ih->type;
> + key_igmp->code = ih->code;
> + key_igmp->group = ih->group;
> +}
> +
> void skb_flow_dissect_meta(const struct sk_buff *skb,
> struct flow_dissector *flow_dissector,
> void *target_container)
> @@ -1398,6 +1422,11 @@ bool __skb_flow_dissect(const struct net *net,
> data, nhoff, hlen);
> break;
>
> + case IPPROTO_IGMP:
> + __skb_flow_dissect_igmp(skb, flow_dissector, target_container,
> + data, nhoff, hlen);
> + break;
> +
> default:
> break;
> }
> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
> index f9df9cac81c5..a2cc824ddeec 100644
> --- a/net/dsa/tag_ocelot.c
> +++ b/net/dsa/tag_ocelot.c
> @@ -2,9 +2,51 @@
> /* Copyright 2019 NXP Semiconductors
> */
> #include <linux/dsa/ocelot.h>
> +#include <linux/igmp.h>
> #include <soc/mscc/ocelot.h>
> #include "dsa_priv.h"
>
> +static const struct flow_dissector_key ocelot_flow_keys[] = {
> + {
> + .key_id = FLOW_DISSECTOR_KEY_CONTROL,
> + .offset = offsetof(struct flow_keys, control),
> + },
> + {
> + .key_id = FLOW_DISSECTOR_KEY_BASIC,
> + .offset = offsetof(struct flow_keys, basic),
> + },
> + {
> + .key_id = FLOW_DISSECTOR_KEY_IGMP,
> + .offset = offsetof(struct flow_keys, igmp),
> + },
> +};
> +
> +static struct flow_dissector ocelot_flow_dissector __read_mostly;
> +
> +static struct sk_buff *ocelot_drop_igmp(struct sk_buff *skb)
> +{
> + struct flow_keys fk;
> +
> + memset(&fk, 0, sizeof(fk));
> +
> + if (!__skb_flow_dissect(NULL, skb, &ocelot_flow_dissector,
> + &fk, NULL, 0, 0, 0, 0))
> + return skb;
> +
> + if (fk.basic.n_proto != htons(ETH_P_IP))
> + return skb;
> +
> + if (fk.basic.ip_proto != IPPROTO_IGMP)
> + return skb;
> +
> + if (fk.igmp.type != IGMPV2_HOST_MEMBERSHIP_REPORT)
> + return skb;
> +
> + skb_dump(KERN_ERR, skb, true);
> +
> + return NULL;
> +}
> +
> static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
> struct sk_buff *clone)
> {
> @@ -84,6 +126,10 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
> u8 *extraction;
> u16 vlan_tpid;
>
> + skb = ocelot_drop_igmp(skb);
> + if (!skb)
> + return NULL;
I probably like this idea, but i need more understanding :)
In case of ocelot_drop_igmp() you wont to really drop it? Or it is just
example on how it may potentially work?
I ask, because IGMP should not be dropped.
--
Regards,
Oleksij
