| 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: <CAJq09z7VDR8Dv20MU5mbsAd4Ux1uL+1qofiuJCrLRubV3uKtpQ@mail.gmail.com>
Date: Mon, 21 Feb 2022 21:37:47 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
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
Em sex., 18 de fev. de 2022 às 08:46, Alvin Šipraga
<ALSI@...g-olufsen.dk> escreveu:
>
> 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/?
Thanks!
>
> I think you should update the documentation at the top of the file as
> well.
OK
>
> >
> > 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?
The last version I sent, there was a valid concern about unaligned
access. With ethertype tags, we know the exact position the tag will
be. However, at the end of the packet, the two bytes might fall into
different words depending on the payload. I did test different
payloads without any issues but my big endian system might have
helped.
I checked the machine code and the compiler seems to be doing a good
job here, converting the memcpy to a simple "register to memory" each
byte at a time.
>
> > +}
> > +
> > +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?
Sure! My bad.
>
> >
> > - 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.
I didn't add it because no trailing tag used it. I checked
tag_hellcreek.c, tag_ksz.c, tag_xrs700x.c. tag_sja1105.c seems to use
both head and tail space and it indeed use pskb_may_pull() but only
related to the head space (SJA1110_HEADER_LEN).
>
> > + 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