| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 17 Mar 2021 10:16:16 +0100
From: Álvaro Fernández Rojas <noltari@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Jonas Gorski <jonas.gorski@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: dsa: tag_brcm: add support for legacy
tags
Hi Vladimir,
> El 15 mar 2021, a las 22:28, Vladimir Oltean <olteanv@...il.com> escribió:
>
> On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:
>> Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
>> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
>> ---
>> include/net/dsa.h | 2 +
>> net/dsa/Kconfig | 7 ++++
>> net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 105 insertions(+)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 83a933e563fe..dac303edd33d 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -49,10 +49,12 @@ struct phylink_link_state;
>> #define DSA_TAG_PROTO_XRS700X_VALUE 19
>> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
>> #define DSA_TAG_PROTO_SEVILLE_VALUE 21
>> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
>>
>> enum dsa_tag_protocol {
>> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
>> DSA_TAG_PROTO_BRCM = DSA_TAG_PROTO_BRCM_VALUE,
>> + DSA_TAG_PROTO_BRCM_LEGACY = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
>
> Is there no better qualifier for this tagging protocol name than "legacy"?
It’s always referred to as “legacy”, so that’s what I used.
Maybe @Florian can suggest a better name for this...
>
>> DSA_TAG_PROTO_BRCM_PREPEND = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
>> DSA_TAG_PROTO_DSA = DSA_TAG_PROTO_DSA_VALUE,
>> DSA_TAG_PROTO_EDSA = DSA_TAG_PROTO_EDSA_VALUE,
>> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
>> index 58b8fc82cd3c..aaf8a452fd5b 100644
>> --- a/net/dsa/Kconfig
>> +++ b/net/dsa/Kconfig
>> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
>> Say Y if you want to enable support for tagging frames for the
>> Broadcom switches which place the tag after the MAC source address.
>>
>> +config NET_DSA_TAG_BRCM_LEGACY
>> + tristate "Tag driver for Broadcom legacy switches using in-frame headers"
>
> Aren't all headers in-frame?
I copied that from NET_DSA_TAG_BRCM:
https://github.com/torvalds/linux/blob/1df27313f50a57497c1faeb6a6ae4ca939c85a7d/net/dsa/Kconfig#L45
Do you want me to change it to "Tag driver for Broadcom legacy switches” or “Legacy tag driver for Broadcom switches"?
>
>> + select NET_DSA_TAG_BRCM_COMMON
>> + help
>> + Say Y if you want to enable support for tagging frames for the
>> + Broadcom legacy switches which place the tag after the MAC source
>> + address.
>>
>> config NET_DSA_TAG_BRCM_PREPEND
>> tristate "Tag driver for Broadcom switches using prepended headers"
>> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
>> index e2577a7dcbca..9dbff771c9b3 100644
>> --- a/net/dsa/tag_brcm.c
>> +++ b/net/dsa/tag_brcm.c
>> @@ -9,9 +9,23 @@
>> #include <linux/etherdevice.h>
>> #include <linux/list.h>
>> #include <linux/slab.h>
>> +#include <linux/types.h>
>>
>> #include "dsa_priv.h"
>>
>> +struct bcm_legacy_tag {
>> + uint16_t type;
>> +#define BRCM_LEG_TYPE 0x8874
>> +
>> + uint32_t tag;
>> +#define BRCM_LEG_TAG_PORT_ID (0xf)
>> +#define BRCM_LEG_TAG_MULTICAST (1 << 29)
>> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
>> +#define BRCM_LEG_TAG_INGRESS (3 << 29)
>> +} __attribute__((packed));
>> +
>> +#define BRCM_LEG_TAG_LEN sizeof(struct bcm_legacy_tag)
>> +
>
> As Florian pointed out, tagging protocol parsing should be
> endian-independent, and mapping a struct over the frame header is pretty
> much not that.
Ok, I will change that in v2.
>
>> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
>> * handle them
>> */
>> @@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);
>> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
>> #endif
>>
>> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
>> +static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
>> + struct net_device *dev)
>> +{
>> + struct dsa_port *dp = dsa_slave_to_port(dev);
>> + struct bcm_legacy_tag *brcm_tag;
>> +
>> + if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
>> + return NULL;
>
> This is not needed since commit 2f0d030c5ffe ("net: dsa: tag_brcm: let
> DSA core deal with TX reallocation").
I’m testing this on v5.10 and I forgot to remove it, sorry :$.
>
>> + /* The Ethernet switch we are interfaced with needs packets to be at
>> + * least 64 bytes (including FCS) otherwise they will be discarded when
>> + * they enter the switch port logic. When Broadcom tags are enabled, we
>> + * need to make sure that packets are at least 70 bytes
>> + * (including FCS and tag) because the length verification is done after
>> + * the Broadcom tag is stripped off the ingress packet.
>> + *
>> + * Let dsa_slave_xmit() free the SKB
>> + */
>> + if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
>> + return NULL;
>
> Are you sure the switches you're working on need this, or is it just
> another copy-pasta?
Yes, I need that.
>
>> + skb_push(skb, BRCM_LEG_TAG_LEN);
>> +
>> + memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
>> +
>> + brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
>> +
>> + brcm_tag->type = BRCM_LEG_TYPE;
>> + brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
>> + brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
>> +
>> + return skb;
>> +}
>> +
>> +
>
> Please remove the extra newline.
Ok.
>
>> +static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
>> + struct net_device *dev,
>> + struct packet_type *pt)
>> +{
>> + int source_port;
>> + struct bcm_legacy_tag *brcm_tag;
>
> Please declare the local variables in the order of decreasing line length.
Ok.
>
>> +
>> + if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
>> + return NULL;
>> +
>> + brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);
>
> Nitpick: the space between the *) and the (skb-> is not needed.
Ok, but this is going to disappear when I remove the struct usage.
>
>> +
>> + source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
>> +
>> + skb->dev = dsa_master_find_slave(dev, 0, source_port);
>> + if (!skb->dev)
>> + return NULL;
>> +
>> + /* Remove Broadcom tag and update checksum */
>> + skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
>> +
>> + skb->offload_fwd_mark = 1;
>> +
>> + /* Move the Ethernet DA and SA */
>> + memmove(skb->data - ETH_HLEN,
>> + skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
>> + 2 * ETH_ALEN);
>> +
>> + return skb;
>> +}
>> +
>> +static const struct dsa_device_ops brcm_legacy_netdev_ops = {
>> + .name = "brcm-legacy",
>> + .proto = DSA_TAG_PROTO_BRCM_LEGACY,
>> + .xmit = brcm_leg_tag_xmit,
>> + .rcv = brcm_leg_tag_rcv,
>> + .overhead = BRCM_LEG_TAG_LEN,
>> +};
>> +
>> +DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
>> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
>> +#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
>> +
>> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
>> static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
>> struct net_device *dev)
>> @@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] = {
>> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
>> &DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
>> #endif
>> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
>> + &DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
>> +#endif
>> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
>> &DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
>> #endif
>> --
>> 2.20.1
Best regards,
Álvaro.
Powered by blists - more mailing lists