| 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
| ||
|
Message-ID: <877d9spfct.fsf@bang-olufsen.dk>
Date: Fri, 18 Feb 2022 11:46:11 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"andrew@...n.ch" <andrew@...n.ch>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"olteanv@...il.com" <olteanv@...il.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v2 1/2] net: dsa: tag_rtl8_4: add rtl8_4t
trailing variant
Luiz Angelo Daros de Luca <luizluca@...il.com> writes:
> The switch supports the same tag both before ethertype or before CRC.
s/The switch supports/Realtek switches support/?
I think you should update the documentation at the top of the file as
well.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> include/net/dsa.h | 2 +
> net/dsa/tag_rtl8_4.c | 127 +++++++++++++++++++++++++++++++++----------
> 2 files changed, 99 insertions(+), 30 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fd1f62a6e0a8..b688ced04b0e 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -52,6 +52,7 @@ struct phylink_link_state;
> #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
> #define DSA_TAG_PROTO_SJA1110_VALUE 23
> #define DSA_TAG_PROTO_RTL8_4_VALUE 24
> +#define DSA_TAG_PROTO_RTL8_4T_VALUE 25
>
> enum dsa_tag_protocol {
> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> @@ -79,6 +80,7 @@ enum dsa_tag_protocol {
> DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE,
> DSA_TAG_PROTO_SJA1110 = DSA_TAG_PROTO_SJA1110_VALUE,
> DSA_TAG_PROTO_RTL8_4 = DSA_TAG_PROTO_RTL8_4_VALUE,
> + DSA_TAG_PROTO_RTL8_4T = DSA_TAG_PROTO_RTL8_4T_VALUE,
> };
>
> struct dsa_switch;
> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
> index 02686ad4045d..d80357cb74b0 100644
> --- a/net/dsa/tag_rtl8_4.c
> +++ b/net/dsa/tag_rtl8_4.c
> @@ -84,87 +84,133 @@
> #define RTL8_4_TX GENMASK(3, 0)
> #define RTL8_4_RX GENMASK(10, 0)
>
> -static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> - struct net_device *dev)
> +static void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> + void *tag)
> {
> struct dsa_port *dp = dsa_slave_to_port(dev);
> - __be16 *tag;
> -
> - skb_push(skb, RTL8_4_TAG_LEN);
> -
> - dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> - tag = dsa_etype_header_pos_tx(skb);
> + __be16 tag16[RTL8_4_TAG_LEN / 2];
>
> /* Set Realtek EtherType */
> - tag[0] = htons(ETH_P_REALTEK);
> + tag16[0] = htons(ETH_P_REALTEK);
>
> /* Set Protocol; zero REASON */
> - tag[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB));
> + tag16[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB));
>
> /* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */
> - tag[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1));
> + tag16[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1));
>
> /* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
> - tag[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index)));
> + tag16[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index)));
> +
> + memcpy(tag, tag16, RTL8_4_TAG_LEN);
Why not just cast tag to __be16 and avoid an extra memcpy for each xmit?
> +}
> +
> +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + skb_push(skb, RTL8_4_TAG_LEN);
> +
> + dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> +
> + rtl8_4_write_tag(skb, dev, dsa_etype_header_pos_tx(skb));
>
> return skb;
> }
>
> -static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> - struct net_device *dev)
> +static struct sk_buff *rtl8_4t_tag_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + /* Calculate the checksum here if not done yet as trailing tags will
> + * break either software and hardware based checksum
> + */
> + if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
> + return NULL;
> +
> + rtl8_4_write_tag(skb, dev, skb_put(skb, RTL8_4_TAG_LEN));
> +
> + return skb;
> +}
> +
> +static int rtl8_4_read_tag(struct sk_buff *skb, struct net_device *dev,
> + void *tag)
> {
> - __be16 *tag;
> u16 etype;
> u8 reason;
> u8 proto;
> u8 port;
> + __be16 tag16[RTL8_4_TAG_LEN / 2];
nit: Reverse christmas-tree order?
>
> - if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
> - return NULL;
> -
> - tag = dsa_etype_header_pos_rx(skb);
> + memcpy(tag16, tag, RTL8_4_TAG_LEN);
Likewise can you avoid this memcpy?
>
> /* Parse Realtek EtherType */
> - etype = ntohs(tag[0]);
> + etype = ntohs(tag16[0]);
> if (unlikely(etype != ETH_P_REALTEK)) {
> dev_warn_ratelimited(&dev->dev,
> "non-realtek ethertype 0x%04x\n", etype);
> - return NULL;
> + return -EPROTO;
> }
>
> /* Parse Protocol */
> - proto = FIELD_GET(RTL8_4_PROTOCOL, ntohs(tag[1]));
> + proto = FIELD_GET(RTL8_4_PROTOCOL, ntohs(tag16[1]));
> if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
> dev_warn_ratelimited(&dev->dev,
> "unknown realtek protocol 0x%02x\n",
> proto);
> - return NULL;
> + return -EPROTO;
> }
>
> /* Parse REASON */
> - reason = FIELD_GET(RTL8_4_REASON, ntohs(tag[1]));
> + reason = FIELD_GET(RTL8_4_REASON, ntohs(tag16[1]));
>
> /* Parse TX (switch->CPU) */
> - port = FIELD_GET(RTL8_4_TX, ntohs(tag[3]));
> + port = FIELD_GET(RTL8_4_TX, ntohs(tag16[3]));
> skb->dev = dsa_master_find_slave(dev, 0, port);
> if (!skb->dev) {
> dev_warn_ratelimited(&dev->dev,
> "could not find slave for port %d\n",
> port);
> - return NULL;
> + return -ENOENT;
> }
>
> + if (reason != RTL8_4_REASON_TRAP)
> + dsa_default_offload_fwd_mark(skb);
> +
> + return 0;
> +}
> +
> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
> + return NULL;
> +
> + if (unlikely(rtl8_4_read_tag(skb, dev, dsa_etype_header_pos_rx(skb))))
> + return NULL;
> +
> /* Remove tag and recalculate checksum */
> skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
>
> dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
>
> - if (reason != RTL8_4_REASON_TRAP)
> - dsa_default_offload_fwd_mark(skb);
> + return skb;
> +}
> +
> +static struct sk_buff *rtl8_4t_tag_rcv(struct sk_buff *skb,
> + struct net_device *dev)
> +{
I wonder if it's necessary to check pskb_may_pull() here too.
> + if (skb_linearize(skb))
> + return NULL;
> +
> + if (unlikely(rtl8_4_read_tag(skb, dev, skb_tail_pointer(skb) - RTL8_4_TAG_LEN)))
> + return NULL;
> +
> + if (pskb_trim_rcsum(skb, skb->len - RTL8_4_TAG_LEN))
> + return NULL;
>
> return skb;
> }
>
> +/* Ethertype version */
> static const struct dsa_device_ops rtl8_4_netdev_ops = {
> .name = "rtl8_4",
> .proto = DSA_TAG_PROTO_RTL8_4,
> @@ -172,7 +218,28 @@ static const struct dsa_device_ops rtl8_4_netdev_ops = {
> .rcv = rtl8_4_tag_rcv,
> .needed_headroom = RTL8_4_TAG_LEN,
> };
> -module_dsa_tag_driver(rtl8_4_netdev_ops);
>
> -MODULE_LICENSE("GPL");
> +DSA_TAG_DRIVER(rtl8_4_netdev_ops);
> +
> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4);
> +
> +/* Tail version */
> +static const struct dsa_device_ops rtl8_4t_netdev_ops = {
> + .name = "rtl8_4t",
> + .proto = DSA_TAG_PROTO_RTL8_4T,
> + .xmit = rtl8_4t_tag_xmit,
> + .rcv = rtl8_4t_tag_rcv,
> + .needed_tailroom = RTL8_4_TAG_LEN,
> +};
> +
> +DSA_TAG_DRIVER(rtl8_4t_netdev_ops);
> +
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4L);
> +
> +static struct dsa_tag_driver *dsa_tag_drivers[] = {
> + &DSA_TAG_DRIVER_NAME(rtl8_4_netdev_ops),
> + &DSA_TAG_DRIVER_NAME(rtl8_4t_netdev_ops),
> +};
> +module_dsa_tag_drivers(dsa_tag_drivers);
> +
> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists